Skip to content

Conversation

@ychin
Copy link
Contributor

@ychin ychin commented Feb 8, 2025

setreg() will automatically calculate the width when a blockwise mode is specified, but it does not properly calculate the line widths of mbyte characters when value is passed as newline-terminated string. It does work when value is passed as a list of lines though.

Fix this by properly using the mbyte utiilty to increment the loop counter.

~~Update: Seems like it's failing some terminal tests in test_terminal2. Looking.~ Update 2: Nevermind the test failures are unrelated to my change.

setreg() will automatically calculate the width when a blockwise mode is
specified, but it does not properly calculate the line widths of mbyte
characters when value is passed as newline-terminated string. It does
work when value is passed as a list of lines though.

Fix this by properly using the mbyte utiilty to increment the loop
counter.
@ychin
Copy link
Contributor Author

ychin commented Feb 8, 2025

Just to demonstrate this: Call :call setreg('a', "🧐🧐\n1234", 'b').

Then in the following text, do "aP between "foo" and "bar":

foobar
xxxyyy

You can see that the pasted value is way wider that it should be:

foo🧐🧐                        bar
xxx1234                        yyy

@chrisbra
Copy link
Member

chrisbra commented Feb 8, 2025

thanks

@chrisbra chrisbra closed this in a17f8bf Feb 8, 2025
@ychin ychin deleted the fix-setreg-mbyte-blockwise-width branch February 8, 2025 20:40
zeertzjq added a commit to zeertzjq/neovim that referenced this pull request Feb 9, 2025
…blockwise mode

Problem:  setreg() doesn't correctly handle mbyte chars in blockwise
          mode
Solution: use mb_ptr2len_len function pointer (Yee Cheng Chin)

setreg() will automatically calculate the width when a blockwise mode is
specified, but it does not properly calculate the line widths of mbyte
characters when value is passed as newline-terminated string. It does
work when value is passed as a list of lines though.

Fix this by properly using the mbyte function pointer to increment the
loop counter.

closes: vim/vim#16596

vim/vim@a17f8bf

Co-authored-by: Yee Cheng Chin <[email protected]>
zeertzjq added a commit to zeertzjq/neovim that referenced this pull request Feb 9, 2025
…blockwise mode

Problem:  setreg() doesn't correctly handle mbyte chars in blockwise
          mode
Solution: use mb_ptr2len_len function pointer (Yee Cheng Chin)

setreg() will automatically calculate the width when a blockwise mode is
specified, but it does not properly calculate the line widths of mbyte
characters when value is passed as newline-terminated string. It does
work when value is passed as a list of lines though.

Fix this by properly using the mbyte function pointer to increment the
loop counter.

closes: vim/vim#16596

vim/vim@a17f8bf

Co-authored-by: Yee Cheng Chin <[email protected]>
ychin added a commit to ychin/macvim that referenced this pull request Feb 11, 2025
Apple "Intelligence" Writing Tools was previously not working correctly
with MacVim. When the user chooses to replace the original selection
with the updated texts, MacVim mistreats the input and treat them as
commands instead of raw texts. The reason was that even though this
service uses the NSServicesMenuRequestor API to obtain the selected
texts, it does not use it to send over the replacement. Instead, it uses
NSTextInput's `insertText:replacementRange` to do so instead, which we
never implemented properly. The reason behind this choice was probably
because Writing Tools first shows a UI with user interaction and has a
delay between obtaining the texts and replacing them, like a regular
Services menu. This means the selection may already be invalid by the
time it requests a replacement.

To fix this, add a new IPC API `replaceSelectedText` to replace the
selected texts and redirect `insertText:replacementRange` to use it if
the replacement range is non-empty. This isn't the most correct
implementation of the protocol but should work in most cases. We don't
have a way to implement it "correctly" as MacVim does not have easy
access to Vim's internal text storage. Also make sure the Service
menu uses this API (for things like "convert to full width" and
Traditional/Simplified Chinese conversions). The old method of simple
injecting a normal mode command `s` before the text was horribly buggy.
It also works with visual block selection properly now.

The implementation uses Vim's register put functionality because Vim
doesn't have an API to simply replace a block of text, and everything
has to go through registers. At the same time, replace the
implementation for the old `selectedText` IPC API to not do this,
because Vim *did* end an API to do so for obtaining texts (via
`getregion()`) and it's more stable to use this than to manually
cache/restore registers.

Related: vim/vim#16596 (fixes `setreg()` which this uses)

Fix macvim-dev#1512
ychin added a commit to ychin/macvim that referenced this pull request Feb 11, 2025
Apple "Intelligence" Writing Tools was previously not working correctly
with MacVim. When the user chooses to replace the original selection
with the updated texts, MacVim mistreats the input and treat them as
commands instead of raw texts. The reason was that even though this
service uses the NSServicesMenuRequestor API to obtain the selected
texts, it does not use it to send over the replacement. Instead, it uses
NSTextInput's `insertText:replacementRange` to do so instead, which we
never implemented properly. The reason behind this choice was probably
because Writing Tools first shows a UI with user interaction and has a
delay between obtaining the texts and replacing them, like a regular
Services menu. This means the selection may already be invalid by the
time it requests a replacement.

To fix this, add a new IPC API `replaceSelectedText` to replace the
selected texts and redirect `insertText:replacementRange` to use it if
the replacement range is non-empty. This isn't the most correct
implementation of the protocol but should work in most cases. We don't
have a way to implement it "correctly" as MacVim does not have easy
access to Vim's internal text storage. Also make sure the Service
menu uses this API (for things like "convert to full width" and
Traditional/Simplified Chinese conversions). The old method of simple
injecting a normal mode command `s` before the text was horribly buggy.
It also works with visual block selection properly now.

The implementation uses Vim's register put functionality because Vim
doesn't have an API to simply replace a block of text, and everything
has to go through registers. At the same time, replace the
implementation for the old `selectedText` IPC API to not do this,
because Vim *did* end an API to do so for obtaining texts (via
`getregion()`) and it's more stable to use this than to manually
cache/restore registers.

Related: vim/vim#16596 (fixes `setreg()` which this uses)

Fix macvim-dev#1512
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants