For a reversed Visual selection, getpos("'<") reports the buffer-relative start while setpos("'<", ...) updates the endpoint associated with the selection direction. setcharpos() follows the same path, so the get/set APIs are inconsistent. Fixes #19049.
Resolve '< and '> to the logical buffer-relative endpoints before updating them. When a new endpoint crosses the other, clamp the range; when one endpoint is deleted, preserve the remaining endpoint so the range can be recreated. Update builtin.txt and add regression coverage for forward and reverse selections, crossing, deletion and recreation, independent initialization, and setcharpos().
make -C srcmake -C src/testdir -W test_marks.vim -W test_visual.vim -W test_codestyle.vim test_marks.res test_visual.res test_codestyle.resgit diff --checkFocused test counts: test_marks 14/14, test_visual 81/81, test_codestyle 5/5.
AI-assisted: Codex
https://github.com/vim/vim/pull/21023
(3 files)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
Thanks, but that visual marks are clamped is not correct I think. Take the following test:
diff --git a/src/testdir/test_visual.vim b/src/testdir/test_visual.vim index beea4c431..f2e7a1489 100644 --- a/src/testdir/test_visual.vim +++ b/src/testdir/test_visual.vim @@ -3065,4 +3065,14 @@ func Test_visual_ended_in_unloaded_buffer() %bw! endfunc +func Test_visual_clamp() + new + call setline(1, ['one', 'two', 'three']) + normal! 2GVjy + call setpos("'>", [0, 1, 1, 0]) + '<,'>d + call assert_equal(['three'], getline(1,'$')) + bw! +endfunc + " vim: shiftwidth=2 sts=2 expandtab
clamping here means after setting the '> mark both visual marks point to line 1 and we suddenly lost the original visual selection, which means we are deleting an unexpected range. That is too much surprising and I think we should not do this.
Is there a reason we need to do the clamping on setting the mark? I believe we don't need to do it and leave it to getmark() to return the marks in the correct order.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
You're right. Your example exposed a bug in the clamping logic: moving '> before '< discarded the previous endpoint. I removed the clamping. setpos() now resolves the requested logical mark and updates only that endpoint, while getmark() handles the ordering.
I added your linewise case and the inverse crossing case; both now preserve the full range.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
I found an existing compatibility constraint while testing the revised approach. Consider a reversed stored selection with endpoints 7,1:
setpos("\x27<", 2) followed by setpos("\x27>", 8) would ideally produce logical range 2..8.setpos("\x27<", 8), then setpos("\x27>", 10)) must produce 8..10.After the first call, both cases are reversed stored endpoints followed by setting \x27> beyond the high endpoint. Without tracking call history, choosing the logical endpoint preserves the first case but breaks the existing pair-replacement behavior; choosing the stored endpoint does the reverse.
Which behavior should take precedence when a call crosses the old range? I can keep this fix limited to non-crossing updates (the original report), or make logical endpoint naming take precedence across crossings.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
Could you share the first failing test or assertion from the normal GCC Linux job? Its check annotation only reports exit code 2. That would help me separate the CI failure from the crossing-semantics question above before revising the patch.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
This is the error:
Failures:
From test_visual.vim:
Found errors in Test_visual_oper_pending_mode_maps():
command line..script /home/runner/work/vim/vim/src/testdir/runtest.vim[636]..function RunTheTest[63]..Test_visual_oper_pending_mode_maps line 33: Expected 'LemonNewNewZ' but got 'LemoNewNewZ'
you should be able to reproduce this locally when running (from the testdir): rm -f test_visual.res; make test_visual.res
This is the function, the error seems to be on the last assert:
" Operator-pending mode maps (movement and text object) " - Simple " - With Ex command moving the cursor " - With Ex command and Visual selection (custom text object) func Test_visual_oper_pending_mode_maps() new call append(0, '') func MoveToCap() call search('\u', 'W') endfunction func SelectInCaps() let [line1, col1] = searchpos('\u', 'bcnW') let [line2, col2] = searchpos('.\u', 'nW') call setpos("'<", [0, line1, col1, 0]) call setpos("'>", [0, line2, col2, 0]) normal! gv endfunction onoremap W /\u/<CR> onoremap <Leader>W :<C-U>call MoveToCap()<CR> onoremap iW :<C-U>call SelectInCaps()<CR> call setline(1, 'PineappleQuinceLoganberryOrangeGrapefruitKiwiZ') call cursor(1, 1) exe "normal cW-\<Esc>l.l2.l." call assert_equal('----Z', getline(1)) call setline(1, 'JuniperDurianZ') call cursor(1, 1) exe "normal g?\WfD." call assert_equal('WhavcreQhevnaZ', getline(1)) call setline(1, 'LemonNectarineZ') call cursor(1, 1) exe "normal yiWPlciWNew\<Esc>fr." call assert_equal('LemonNewNewZ', getline(1)) ounmap W ounmap <Leader>W ounmap iW bwipe! delfunc MoveToCap delfunc SelectInCaps endfunc
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
I reproduced the line-33 failure at cc83269. Using fresh builds with the same configuration, Test_visual_oper_pending_mode_maps() passes on the base ecfea491 and fails on the PR head (LemoNewNewZ).
The first ciW already selects an extra character, before dot-repeat. From the previous range [1,5], setting '< to 6 gives [5,6]. Setting '> to 10 then selects the same storage slot again, yielding [5,10] rather than [6,10].
This also occurs outside an operator mapping, including after a reversed selection (v4lo instead of v4l). Source this script in vim --clean:
new call setline(1, 'LemonLemonNectarineZ') execute "normal! gg0v4l\<Esc>" call setpos("'<", [0, 1, 6, 0]) call setpos("'>", [0, 1, 10, 0]) normal! gvy echo getreg('"')
The base yanks Lemon; the head yanks nLemon.
Would you prefer preserving consecutive setters that construct [6,10] through stable endpoint identity, or following the current sorted marks on every setter, producing [5,10] and changing existing mapping usage? I recommend preserving the existing workflow. Would a broader change with explicit endpoint-identity metadata be acceptable?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
I've updated the setters to keep the endpoint identities established by the saved Visual selection. Consecutive writes now preserve both endpoints, and the unchanged Test_visual_oper_pending_mode_maps() passes with LemonNewNewZ.
Getters remain ordered by position, so after endpoints cross, setpos(name, getpos(name)) can change the marks; that tradeoff is documented. Local marks, Visual, undo, and codestyle tests finished with 156 passes and 2 clipboard-feature skips. Actual old/new undo writers and all four reader combinations also passed, with legacy behavior preserved for older readers and files.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()