[patch] visual <C-A>/<C-X> is refactored to support vcol.

[h_east]

To: Bram (As a Vimboss)
To: Christian Brabandt (As a visual <C-A>/<C-X> first patch author)
To: Jason Schulz (As a support for bin 'nrformats' patch author)

Hi,

I refactored visual <C-A>/<C-X> to support vcol et al.
This mean is <TAB> code free!

Contents of patch.
- visual <C-A>/<C-X> support vcol. (<TAB> code free)
- 'test_increment' convert from old style test to new style test. and added some test items.
- Processing was allowed to separate.
(line loop process and add/subtract process)
(We have to use the existing function block_prep() to process the block-wise)
- We removed the halfway right-to-left processing.
(Remove RLADDSUBFIX() macro)
(This is causing the actual problem)
$ vim -Nu NONE -c "set rightleft"
i123 45<Esc>
<C-A> " Unexpected swap the numbers of strings occurred.

Christian Brabandt and Jason Schulz and List>
I was wondering if you could review this patch.

Jason Schulz>
Sorry to such just your patch was included.
I have just completed the doing has been working since last fall :-)

Thanks.
--
Best regards,
Hirohito Higashi (a.k.a h_east)

[Bram Moolenaar]

That's a big change. Can you give an example of what didn't work before
and works now?

To make reviewing easier, it would be good to first make a patch to
change the test from old to new style. Then we know the test works with
the old code.

Then change the code and extend the test with parts that didn't
work before. So we can clearly see what's fixed. And then if one would
try to only include the change to the tests would require the test to
fail.

The docs are not updated, thus for the user there is no change?


--
Biting someone with your natural teeth is "simple assault," while biting
someone with your false teeth is "aggravated assault."
[real standing law in Louisana, United States of America]

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

[h_east]

Hi Bram,

2016-1-7(Thu) 6:22:49 UTC+9 Bram Moolenaar:

Please see Test_visual_increment_27() ~ Test_visual_increment_34().
Below is ather exsample.

Case 1 (Visual blockwise <C-A> with TAB and SPACE mixed)
- Manipulate
$ vim -Nu NONE
:call setline(1, ["1234 56", "\<TAB>78"])
:exec "norm! ggw\<C-V>jl\<C-A>"
- Expect result
"1234 57"
"\<TAB>79"
- Unpatched result
"1235 56"
"\<TAB>79"

>
> To make reviewing easier, it would be good to first make a patch to
> change the test from old to new style. Then we know the test works with
> the old code.

Okay. I attached simple patch that only convert to new style test of test_increment.

>
> Then change the code and extend the test with parts that didn't
> work before. So we can clearly see what's fixed. And then if one would
> try to only include the change to the tests would require the test to
> fail.

Sure.
Send by dividing into several the first patch at the end of week.

>
> The docs are not updated, thus for the user there is no change?

For now, it is yes.
The following help does not touch the <TAB> code in this article.
:h v_CTRL-A

Of course, I think that it may be to append an explanation.
But My English is not skillful :-)

[h_east]

Hi Bram,

2016-1-7(Thu) 16:04:39 UTC+9 h_east:
snip...

> > That's a big change. Can you give an example of what didn't work before
> > and works now?
>
> Please see Test_visual_increment_27() ~ Test_visual_increment_34().
> Below is ather exsample.
>
> Case 1 (Visual blockwise <C-A> with TAB and SPACE mixed)
> - Manipulate
> $ vim -Nu NONE
> :call setline(1, ["1234 56", "\<TAB>78"])
> :exec "norm! ggw\<C-V>jl\<C-A>"
> - Expect result
> "1234 57"
> "\<TAB>79"
> - Unpatched result
> "1235 56"
> "\<TAB>79"

snip...

Case 2 (Redo after visual characterwise/blockwise <C-A>)

- Manipulate
$ vim -Nu NONE

:call setline(1, ["1234\<TAB>56"])
:exec "norm! ggw\<C-V>l\<C-A>."
- Expect result
"1234\<TAB>58"
- Unpatched result
"1235\<TAB>57"

Above behavior will be fixed in my patch or the following patch.
https://groups.google.com/d/topic/vim_dev/OdcI-cHv1dw/discussion

Thanks

[Bram Moolenaar]

I see, thanks for fixing that.

> > To make reviewing easier, it would be good to first make a patch to
> > change the test from old to new style. Then we know the test works with
> > the old code.
>
> Okay. I attached simple patch that only convert to new style test of
> test_increment.

Thanks. The original test had a nice explanation of what it was doing.
Although the new style test do have the assert_equal() calls that make
it easier to see what is going on, the commands themselves are still a
bit of a puzzle. Since the explanations were already written, we can
keep them. Using the comment above the test function should work well.


--
A parent can be arrested if his child cannot hold back a burp during a church
service.
[real standing law in Nebraska, United States of America]

[h_east]

Hi Bram and List,

2016-1-8(Fri) 2:27:17 UTC+9 Bram Moolenaar:

Indeed. I did it. Please check and include attached patch.

BTW, The following changes I thought happy for test_increment.vim.
How do you like it?

diff --git a/src/testdir/runtest.vim b/src/testdir/runtest.vim
index 1c4cead..de1bda1 100644
--- a/src/testdir/runtest.vim
+++ b/src/testdir/runtest.vim
@@ -66,7 +66,7 @@ endif
redir @q
function /^Test_
redir END
-let tests = split(substitute(@q, 'function \(\k*()\)', '\1', 'g'))
+let tests = sort(split(substitute(@q, 'function \(\k*()\)', '\1', 'g')))

for test in tests
if exists("*SetUp")

[Bram Moolenaar]

Thanks!

> BTW, The following changes I thought happy for test_increment.vim.
> How do you like it?

Yes, that's better than the arbitrary order we have now.
Unfortunately it break test_quickfix, it makes an assumption about test
function ordering. That needs to be fixed.


--
INSPECTOR END OF FILM: Move along. There's nothing to see! Keep moving!
[Suddenly he notices the cameras.]
INSPECTOR END OF FILM: (to Camera) All right, put that away sonny.
[He walks over to it and puts his hand over the lens.]
"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

[h_east]

Hi,

2016-1-10(Sun) 4:15:14 UTC+9 Bram Moolenaar:

Thanks for including this.
Patch 7.4.1072
https://groups.google.com/d/topic/vim_dev/_K0eQkIB5aY/discussion

>
> > BTW, The following changes I thought happy for test_increment.vim.
> > How do you like it?
>
> Yes, that's better than the arbitrary order we have now.
> Unfortunately it break test_quickfix, it makes an assumption about test
> function ordering. That needs to be fixed.

Thanks for including this too.
Patch 7.4.1071
https://groups.google.com/d/topic/vim_dev/p6IAS6bPFDU/discussion

Well, the next simple patch is about this.

> - We removed the halfway right-to-left processing.
> (Remove RLADDSUBFIX() macro)
> (This is causing the actual problem)
> $ vim -Nu NONE -c "set rightleft"
> i123 45<Esc>
> <C-A> " Unexpected swap the numbers of strings occurred.

Investigation result:
Reverse line process of 'rightleft' is performed by the display part. therefore it doesn't need in do_addsub().

I've attached a patch containing the test.
Please check it.

[Bram Moolenaar]

Thanks. Now I could check that the test fails before including the
change in ops.c.

--
God made machine language; all the rest is the work of man.

[h_east]

Hi Bram,

2016-1-10(Sun) 22:14:01 UTC+9 Bram Moolenaar:

Thanks for include this quickly.
Patch 7.4.1076
https://groups.google.com/d/topic/vim_dev/TOHFHDxek34/discussion

The last patch fixing this.

- visual <C-A>/<C-X> support vcol. (<TAB> code free)

- Processing was allowed to separate.
(line loop process and add/subtract process)
(We have to use the existing function block_prep() to process the block-wise)

Sorry, more than this is difficult to separate the patch...
Please include this.

[Bram Moolenaar]

I have included it now. Unfortunately there was a merge conflict with
patch 7.4.1085. I solved that. Then it turns out that the marks are
set differently. Patch 7.4.1085 sets then before the first changed
number and at the end of the last changed number. Your patch put them
on the Visual area. I think the first solution is more accurate, thus I
kept that.

I found that OP_ADD and OP_SUBTRACT are also used in a Perl header file.
Thus I changed them to OP_NR_ADD and OP_NR_SUB. Not so nice...


--
SUPERIMPOSE "England AD 787". After a few more seconds we hear hoofbeats in
the distance. They come slowly closer. Then out of the mist comes KING
ARTHUR followed by a SERVANT who is banging two half coconuts together.

"Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD

[h_east]

Hi Bram and List,

2016-1-11(Mon) 6:13:31 UTC+9 Bram Moolenaar:

Thanks you verrry much. I think so.

However, I think, '[ and '] marks process is not consistent with other operators.
Of course, it should be modified other operator processing of the '[ and '] marks.
(e.g. op_tilde() in ops.c : 2387)

The report message is required modify too.
(e.g. op_tilde() in ops.c : 2390)

I will write the patch this weekend if you are okay.

--
Best regards,
Hirohito Higashi (a.k.a h_east)

>

[Bram Moolenaar]

Hirohito Higashi wrote:

[...]

> > I have included it now. Unfortunately there was a merge conflict with
> > patch 7.4.1085. I solved that. Then it turns out that the marks are
> > set differently. Patch 7.4.1085 sets then before the first changed
> > number and at the end of the last changed number. Your patch put them
> > on the Visual area. I think the first solution is more accurate, thus I
> > kept that.
>
> Thanks you verrry much. I think so.
>
> However, I think, '[ and '] marks process is not consistent with other operators.
> Of course, it should be modified other operator processing of the '[ and '] marks.
> (e.g. op_tilde() in ops.c : 2387)
>
> The report message is required modify too.
> (e.g. op_tilde() in ops.c : 2390)
>
> I will write the patch this weekend if you are okay.

It depends on how you look at it. The CTRL-A command does not apply to
the whole selected area, it finds the first number in each line.
While the tilde operator does work on the whole area. Although some
character may end up unchanged because they were already uppper or lower
case.

I think for the user it can be useful to move to the last changed
number, while it's not so useful to move to some position after it.

--
SOLDIER: What? A swallow carrying a coconut?
ARTHUR: It could grip it by the husk ...