fix(align): blockwise align with wide and combining characters #1214
Conversation
This affects ['Align with preview']['works']. The operator blockwise and visual blockwise forms of this test should affect the text in the same way, but the reference screenshots didn't reflect this. The old screenshots showed the result of an erroneously exclusive gA<C-v>`a motion. If d<C-v>`a is used instead in the same situation, the characters after the first _ (in the same column as the 'a mark) are deleted. Forced-blockwise motions behave the same as visual-blockwise motions, except the cursor is placed at the start of the affected region. This means the motion is inclusive or exclusive depending on the 'selection' option.
|
Thanks for the PR! Code change at glance looks a bit complicated, maybe even to the point of opting for not supporting these cases. And definitely uses functionality present only in Neovim>=0.10. Currently there is a "code freeze" period before the next 0.14.0 release, so this has to wait a bit. After that I'll have a closer look. |
|
It looks like the To my eye, the new implementation looks about as complex as the old implementation. The current handling of virtual columns causes text to be deleted or duplicated, and causes a You can run the tests at c2078cb to see the problems with the current implementation. Notable test failures @ c2078cbThese also fail in the same way for |
Diomendius
force-pushed
the
align_fix
branch
from
8f576af to
735c0bb
Compare
When the blockwise selection is in to-EOL mode (when the last horizontal
motion extends to EOL, like $) the selection on each line extends to EOL
regardless of the position of the rightmost selection corner. With
'virtualedit', the selection extends to the column one character past
the end of the longest line in the region.
getcurpos() seems to be the only way to detect if the blockwise
selection is in to-EOL mode, as the last field (curswant) is v:maxcol if
the cursor is in to-EOL mode, which is normally true when the active
blockwise selection is to-EOL. Swapping to one of the left corners with
o or O deactivates to-EOL mode, so this isn't a problem. This is fine
for visual mode, but requires an awkward workaround in operator mode:
1. Save cursor position
2. Restore visual selection
3. Call getcurpos()
4. Exit visual mode
5. Restore cursor position
There is one exception where the cursor position doesn't match the
to-EOL-ness of the selection, which is after the '> mark has been
manually set, such as via m>.
See vim/vim#15587
Subtracts 1 character column not virtual column when adjusting for exclusive selection. Passes list = true to virtcol() to get both the left and right virtcols for wide characters. Renames region_virtcols() to block_region_virtcols() as it currently isn't used for other modes and needs slightly different logic to handle exclusive non-blockwise regions.
Diomendius
force-pushed
the
align_fix
branch
from
735c0bb to
64561d4
Compare
Adds a fallback for v:maxcol for < 0.9.0. Adds workaround for getting both left and right virtcol bounds in pos_to_virtcol() for < 0.10.0 as the list flag for virtcol() requires 0.10.0. Also replaces next_char_boundary() with byte_range_from_virtcols(). This allows both call sites to be cleaner, and the new implementation is robust against virtcol2col() returning different byte positions for multibyte characters on different versions. On 0.10.0 and later virtcol2col() returns the index of the first byte of the character at the specified position, but it returns the index of the last byte on older versions. It would actually save a lot of hassle if it were possible to get both these values, as the weird workaround with byteidx() and charidx() wouldn't be necessary.
Diomendius
force-pushed
the
align_fix
branch
from
64561d4 to
ce114e8
Compare
|
@Diomendius, thanks for the work! I'll take it from here after the next release of 'mini.nvim'. |
Resolve #1214 Co-authored-by: Diomendius <[email protected]>
|
@Diomendius, thanks again for this huge amount of meticulous work in the PR! After reading into this more closely, here is the final result of what I think is reasonable to have:
In the future I highly suggest first opening an issue to discuss the problem before making the PR. This will most certainly save time for both parties. |
|
I skimmed your comment and commit when you made them, but didn't reply at the time. Thanks for what you've done here. I don't blame you for omitting support for I happened to revisit your comment today and noticed your comments on workarounds for blockwise to-EOL selection. Using
I think what this really highlights is the deficiencies in the Nvim API regarding selections. Having a single representation of a visual selection that unambiguously indicates which cells are included, regardless of selection order or |
2 participants
Blockwise align is currently broken when lines contain multi-cell characters such as:
^*)<xxxx>)Single display cells containing multiple Unicode code points also cause problems, such as when
éis decomposed toe<U+0301 COMBINING ACUTE ACCENT>.This is mainly caused by passing virtual column positions to
strcharpart()andbyteidx()as if they were character indices.When the leftmost selection corner sits directly on a wide character, only the last virtual column the character occupies is included in the selection, as
virtcol()is called without thelistargument; without this argument,virtcol()only returns the position of the rightmost column containing the character.Exclusive block selections also aren't handled correctly, as the right vcol is always shifted left. An exclusive block selection extends from and including the corner with the lowest byte offset, to the left edge of the corner with the highest. This means that the selected region is the same as for an inclusive selection unless the corner with the highest byte offset is at least one virtual column to the right of the other corner.
This PR fixes all these issues and adds regression tests. I've added
T['Align']['all modes']andT['Align']['blockwise mode']sets, which many of the existing tests could be adapted to use. This should be applicable to any tests which operate over the entire text, as all modes should behave identically in this case.One issue I encountered which this PR does not fix is that tabs not at the start of the line are aligned incorrectly. I don't think this is worth fixing as using tabs in the middle of lines is a terrible idea in general, and it's not obvious what the correct behaviour should even be.