Add getregionpos() #14617

Shougo · 2024-04-22T09:56:59Z

zeertzjq · 2024-04-22T10:01:31Z

When the selection is blockwise, shouldn't there be a pair of positions for every line? They may have different col and off values when there are TABs or multibyte chars.

yegappan · 2024-04-22T15:39:02Z

src/evalfunc.c

+ * "getregionpos()" function
+ */
+    static void
+f_getregionpos(typval_T *argvars, typval_T *rettv)


Can you add tests for the new getregionpos() function?

Yes. It should. But current tests are very many. It is hard.

Without tests, this cannot be merged.

Can you add one more test similar to def Test_getregion() in test_vim9_builtin.vim, say Test_getregionpos() and check for E1209 and E1211?

OK. I have added them.

Shougo · 2024-04-23T00:34:38Z

When the selection is blockwise, shouldn't there be a pair of positions for
every line? They may have different col and off values when there are TABs or
multibyte chars.

I have implemented it. Please check the implementation.

Shougo · 2024-04-24T04:52:49Z

I have added tests.

NOTE: I cannot test for list_alloc() and list_append_list() though...

runtime/doc/builtin.txt

justinmk · 2024-04-24T10:12:12Z

runtime/doc/builtin.txt

+		Note: When the selection is blockwise, it returns a pair of
+		positions for every line:
+                    [[{pos_start}, {pos_end}], ...]


this note is not needed now, right? because the same structure is always returned

The same structure is returned. But it may be hard to understand why the positions are list.

Can you say instead:

Same as |getregion()|, but returns a list of positions describing the buffer text segments bound by {pos1} and {pos2}.

You can remove:

See |getregion()| for the arguments and notes.

OK. I have fixed it.

chrisbra · 2024-04-29T18:34:47Z

So, is this ready? Have a few people tried it out?

jessepav · 2024-04-29T19:02:20Z

I've been running with this PR applied, and it works as well as getregion() for me.

The only issue I've faced with both of these functions is that they error out (E964: Invalid column number: 2147483647) when you make a linewise visual selection and use the '< and '> positions, since the column of pos2 is set to v:maxcol.

I don't know if that is unintended behavior, though, and is easy enough to check for in scripts.

girishji · 2024-04-29T19:39:46Z

So, is this ready? Have a few people tried it out?

I tried this test, and it gave error E964: Invalid column number: 2147483647

./vim -Nu NONE -S <(cat <<EOF
    vim9script
    :set shortmess=I
    autocmd TextYankPost * {
        if v:event.operator ==? 'y'
            :echom getregionpos(getpos("'["), getpos("']"))
        endif
    }
    setline(1, ['abcd', 'efghij'])
    :normal yy
EOF
)

Please check if my test itself is wrong. I wrote it in a hurry.

It seems it is not exactly like getregion(). If I were to select a visual region using v or V, I think it should return {pos} of all intermediate lines, not just first and last line. It does this for ctrl-v. I understood this from reading the doc. I have not tested this.

Shougo · 2024-04-29T23:49:42Z

I have fixed columns error. It already exists when getregion() implementation though.

It seems it is not exactly like getregion(). If I were to select a visual region using v or V, I think it should return {pos} of all intermediate lines, not just first and last line. It does this for ctrl-v. I understood this from reading the doc. I have not tested this.

I know the feature. I can change the behavior. But I think it is not useful.
<c-v> behavior is changed because of #14617 (comment).
Please fix my documentation if it is not exact.

jessepav · 2024-04-30T02:43:43Z

After the most recent commit, this PR works great for me – and now getregion() works for 'V' mode too.

girishji · 2024-04-30T03:23:34Z

The test I mentioned above still fails. Same error (E964).

Somewhere it is mentioned that the motivation for this API is this, which roughly translates to this test:

./vim -Nu NONE -S <(cat <<EOF
    vim9script
    :set shortmess=I
    autocmd TextYankPost * {
        if v:event.operator ==? 'y'
            var pos = getregionpos(getpos("'["), getpos("']"))->mapnew((_, v) => [v[0][1], v[0][2], v[1][2] - v[0][2] + 1])
            var m = matchaddpos('IncSearch', pos)
            timer_start(300, (_) => m->matchdelete())
        endif
    }
    setline(1, ['abcd', 'efghij'])
    :normal yy
EOF
)

This test does not pass. I'm afraid this PR is not well thought through.

jessepav · 2024-04-30T03:43:04Z

The test given above doesn't fail for me with the latest commit.

pos is set to [[1, 1, 5]] by the autocmd when I test it.

Shougo · 2024-04-30T03:44:41Z

Yes. It works for me. I think you don't build the latest version.

girishji · 2024-04-30T05:01:33Z

The second test fails though. There is no value to getregionpos() if you are just returning two getpos(), which are the exact arguments I am supplying!

Try the following. It does not highlight both lines, but only the first line. This is because getregionpos is returning [[[1, 1, 1, 0], [1, 2, 7, 0]]] instead of [[[1, 1, 1, 0], [1, 1, 5, 0]], [[1, 2, 1, 0], [1, 2, 7, 0]]].

./vim -Nu NONE -S <(cat <<EOF
    vim9script
    :set shortmess=I
    autocmd TextYankPost * {
        if v:event.operator ==? 'y'
            var pos = getregionpos(getpos("'["), getpos("']"))->mapnew((_, v) => [v[0][1], v[0][2], v[1][2] - v[0][2] + 1])
            var m = matchaddpos('IncSearch', pos)
            timer_start(300, (_) => m->matchdelete())
        endif
    }
    setline(1, ['abcd', 'efghij'])
    :normal yj
EOF
)

I am just a little weary of this PR.

girishji · 2024-04-30T05:26:27Z

I found another bug. <c-v> does not work. The values returned are incorrect. It just returns first and last positions, instead of every line in the visual block.

./vim -Nu NONE -S <(cat <<EOF
    vim9script
    :set shortmess=I
    autocmd TextYankPost * {
        if v:event.operator ==? 'y'
            :echom getregionpos(getpos("'["), getpos("']"))
        endif
    }
    setline(1, ['abcd', 'efghij'])
    :exec "normal \<c-v>"
    :normal 3ljy
EOF
)

Shougo · 2024-04-30T05:46:06Z

I found another bug. <c-v> does not work. The values returned are incorrect. It just returns first and last positions, instead of every line in the visual block.

It is not bug. Please see getregion() documentation.

		The optional argument {opts} is a Dict and supports the
		following items:

			type		Specify the region's selection type
					(default: "v"):
			    "v"		for |characterwise| mode
			    "V"		for |linewise| mode
			    "<CTRL-V>"	for |blockwise-visual| mode

			exclusive	If |TRUE|, use exclusive selection
					for the end position
					(default: follow 'selection')

The default type is v. You need to use visualmode().

It works for me.

vim9script
:set shortmess=I
autocmd TextYankPost * {
    if v:event.operator ==? 'y'
      :echom getregionpos(getpos("'["), getpos("']"), { type: visualmode() })
    endif
}
setline(1, ['abcd', 'efghij'])
:exec "normal \<c-v>"
:normal 3ljy

girishji · 2024-04-30T06:12:18Z

Thanks, now I get E475: Invalid value for argument type: when I do this:

./vim -Nu NONE -S <(cat <<EOF
    vim9script
    :set shortmess=I
    autocmd TextYankPost * {
        if v:event.operator ==? 'y'
            var pos = getregionpos(getpos("'["), getpos("']"), { type: visualmode() })
            var m = matchaddpos('IncSearch', pos->mapnew((_, v) => [v[0][1], v[0][2], v[1][2] - v[0][2] + 1]))
            timer_start(300, (_) => m->matchdelete())
        endif
    }
    setline(1, ['abcd', 'efghij'])
    :normal yj
EOF
)

Also, my other concern (see post above) about this api not adding additional value by not returning intermediate lines during 'v' and 'V' still stands.

jessepav · 2024-04-30T06:29:42Z

Visual mode was never entered in your test script, so visualmode() is returning an empty string.

Shougo · 2024-04-30T06:30:48Z

Try the following. It does not highlight both lines, but only the first line. This is because getregionpos is returning [[[1, 1, 1, 0], [1, 2, 7, 0]]] instead of [[[1, 1, 1, 0], [1, 1, 5, 0]], [[1, 2, 1, 0], [1, 2, 7, 0]]].

OK. I get the problem. I will change the result value.

girishji · 2024-04-30T08:07:44Z

Visual mode was never entered in your test script, so visualmode() is returning an empty string.

Thanks. Modified the script.

I see 2 issues:

<c-v> is reporting one additional character (to the right) than what was selected. Is this intended? How are the tests passing?
intermediate lines are ignored in v and V

You can use the script, visual select, and verify.

/vim -Nu NONE -S <(cat <<EOF                                                                                                        [getregion_pos]
    vim9script
    :set shortmess=I
    autocmd TextYankPost * {
        if v:event.operator ==? 'y'
            var [begin, end, vmode] = [getpos("'["), getpos("']"), visualmode()]
            var pos = getregionpos(begin, end, vmode == null_string ? {} : {type: vmode})
            :echom pos
            var m = matchaddpos('IncSearch', pos->mapnew((_, v) => [v[0][1], v[0][2], v[1][2] - v[0][2] + 1]))
            timer_start(300, (_) => m->matchdelete())
        endif
    }
    setline(1, ['abcd', 'efghij'])
    :normal yj
EOF
)

Shougo · 2024-04-30T08:36:21Z

<c-v> is reporting one additional character (to the right) than what was selected. Is this intended? How are the tests passing?

Hm... It seems broken. I will fix.

Shougo · 2024-04-30T10:16:23Z

I have fixed the problems. Please test.

src/testdir/test_visual.vim

chrisbra · 2024-04-30T18:47:49Z

@justinmk does that do what you want?

girishji · 2024-05-01T08:21:35Z

I have fixed the problems. Please test.

I found many problems:

Do reverse selection in v (begin position after end position), and it gives wrong results.
Have two words foo bar, and cursor on b, and then yank the word ye, it returns wrong results (highlights foo instead of bar).
v does not work unless cursor is on first column

I updated the test to include the offset and exclusive. If anyone has time, please try the script below. Yank some text (with and without visual selection) and verify the values printed.

./vim -Nu NONE -S <(cat <<EOF
    vim9script
    def HighlightYank()
        if v:event.operator ==? 'y'
            var [beg, end, vmode] = [getpos("'["), getpos("']"), visualmode()]
            var pos = getregionpos(beg, end, vmode != null_string ? {type: vmode} : {})
            :echom pos
            var m = matchaddpos('IncSearch', pos->mapnew((_, v) => {
                var col_beg = v[0][2] + v[0][3]
                var col_end = v[1][2] + v[1][3] + (!v:event.inclusive ? 1 : 0)
                return [v[0][1], col_beg, col_end - col_beg + 1]
            }))
            timer_start(300, (_) => m->matchdelete())
        endif
    enddef
    autocmd TextYankPost * HighlightYank()
    setline(1, ['abcd', 'efghij', 'foo bar', '	baz	 bza', 'ぬfooぼ barі'])
    :set ruler
    :normal 2lv2jy
EOF
)

Co-authored-by: Justin M. Keyes <justinkz@gmail.com>

Shougo · 2024-05-03T01:36:53Z

I have rebased it and fixed the conflict.

src/testdir/test_visual.vim

chrisbra · 2024-05-07T18:58:44Z

Thanks all. I have made one minor change using mb_ptr2len() instead of utfc_ptr2len().

Problem: Cannot get a list of positions describing a region (Justin M. Keyes, after v9.1.0120) Solution: Add the getregionpos() function (Shougo Matsushita) fixes: vim/vim#14609 closes: vim/vim#14617 vim/vim@b4757e6 Co-authored-by: Shougo Matsushita <Shougo.Matsu@gmail.com> Co-authored-by: Justin M. Keyes <justinkz@gmail.com>

yegappan reviewed Apr 22, 2024

View reviewed changes