Revert SpawnMultiCursor{Up,Down} honoring softwrap + overhaul LastVisualX usage

[dmaluka]

In #3145, namely in 9f36c57, we changed the behavior of SpawnMultiCursor{Up,Down} to spawn cursor in the next visual line within a wrapped line when softwrap is enabled. That was done for "consistency" with cursor movements (Cursor{Up,Down} etc). However it seems there are no actual use cases for that. Whereas at least some users (see issue #3499) prefer to spawn cursor in the next logical line, regardless of the softwrap setting.

So restore the old behavior.

In the future we may also consider adding actions like CursorUpLogical, CursorDownLogical etc, for moving cursor by logical lines even if softwrap is enabled, if there are users that want it. For now let's just fix multicursor spawning.

...That is just a part of the story. Simply reverting 9f36c57 is not enough, since it would make SpawnMultiCursor{Up,Down} broken, due to another change from #3145, namely 93b5326, due to the fact that LastVisualX currently assumes visual lines, not logical lines, so mixing logical lines usage with LastVisualX usage would cause SpawnMultiCursor{Up,Down} spawn cursors at unexpected locations in some cases.

The easy way would be to revert 93b5326 as well, but that means bringing back the issues fixed by 93b5326, which would be unfortunate.

So instead rework LastVisualX and GetVisualX() usage: restore the original semantics of LastVisualX that it used to have long ago, before #2076: last visual x cursor location in a logical line, not in a visual line. And add a separate LastWrappedVisualX field for last visual x cursor location in a visual line. So we can track last x position at the same time for both cases when we care about softwrap (e.g. cursor movements) and when we don't care about it (e.g. for spawning multicursors, in order to fix #3499, and possibly in the future for Cursor{Up,Down}Logical etc).

Also this fixes a minor bug: in InsertTab(), when tabstospaces is enabled and we insert a tab, the amount of inserted spaces depends on the visual line wrapping (i.e. on the window width), which is probably not a good idea.

Fixes #3499

[JoeKar]

Missed these both or ignored by intention?

https://github.com/zyedidia/micro/blob/d60413f03c3ac0bf0c0e557ed37bfbee43472ce8/internal/display/bufwindow.go#L61

https://github.com/zyedidia/micro/blob/d60413f03c3ac0bf0c0e557ed37bfbee43472ce8/internal/display/bufwindow.go#L163

[dmaluka]

By intention: the next commit 6214abb changes those 2 places to set c.LastVisualX instead of c.LastWrappedVisualX. In those 2 places we don't need to update c.LastVisualX, since c.LastVisualX, unlike c.LastWrappedVisualX, doesn't depend on the window width or on whether softwrap is enabled.

Hmm... it just occurred to me that it means that in those 2 places, i.e. when we resize the window or enable/disable softwrap, we "reset" cursor up/down movements if softwrap is enabled (i.e. when LastWrappedVisualX is used) but don't "reset" them anymore if softwrap is disabled (i.e. if LastVisualX is used). So it introduces a small "inconsistency" here. OTOH, perhaps this inconsistency is a rather good thing: this "resetting" of cursor up/down movements is generally an undesirable thing to do, we do it out of necessity and for simplicity, so since we indeed can avoid doing it as least if softwrap is disabled (and we achieve that for free, just as a side effect of this PR), it's a good thing?

[JoeKar]

By intention: the next commit 6214abb changes those 2 places to set c.LastVisualX instead of c.LastWrappedVisualX.

Ah, I see. Was interrupted for a long time right after the review of the first commit and thus didn't realize that.

this "resetting" of cursor up/down movements is generally an undesirable thing to do, we do it out of necessity and for simplicity, so since we indeed can avoid doing it as least if softwrap is disabled [...], it's a good thing?

Is it worth documenting this there, just for faster realization?
Besides that it should be right.

[dmaluka]

Is it worth documenting this there, just for faster realization?

At the moment I'm inclined to think it's better to leave this without documenting, this is of too little importance to draw attention to it with a comment in the code (perhaps no real user will ever pay attention to this subtle difference in behavior), and if we ever need to think about this again, it shouldn't be difficult to figure out from the code what's going on.

[JoeKar]

Ok, does work so far as intended and SpawnMultiCursorUpN() does the job.
I tried it with MouseMultiCursor too, but this does respect the visual lines only and doesn't care about softwrap. Now I'm undecided if this fits into the picture.

PS:
Why exactly was it named SpawnMultiCursorUpN() instead of SpawnMultiCursorDirection(N)()?

[dmaluka]

Why exactly was it named SpawnMultiCursorUpN()

Just because we also have UpN().

I tried it with MouseMultiCursor too, but this does respect the visual lines only and doesn't care about softwrap. Now I'm undecided if this fits into the picture.

What does MouseMultiCursor have to do with this?

[JoeKar]

Just because we also have UpN().

I understand, just because of the consistency. Well, then we go again with this ugly name.

What does MouseMultiCursor have to do with this?

It spawns multiple cursors by clicks and does this in soft-wrapped visual lines too, which is what we prevent, when we do it with the keyboard only in the same scenario. For me it's inconsistent, but maybe I don't get it again...as so often.

[dmaluka]

I understand, just because of the consistency. Well, then we go again with this ugly name.

I don't find it particularly ugly. It is "move N lines up", where N may be negative.

It spawns multiple cursors by clicks and does this in soft-wrapped visual lines too, which is what we prevent, when we do it with the keyboard only in the same scenario.

We don't prevent spawning multicursors in soft-wrapped visual lines (why would we?), we just prevent spawning multicursors in the same logical line, when using SpawnMultiCursorUp and SpawnMultiCursorDown.

In other words, we just ensure that in cases like the following:

when the cursor is on the slash after github.com, SpawnMultiCursorDown will spawn a cursor on the slash after github.com in the next line, not on the character in the current line that is visually below the slash (the first f letter in buffer in this case).

[JoeKar]

we just prevent spawning multicursors in the same logical line, when using SpawnMultiCursorUp and SpawnMultiCursorDown.

Ok, then just for the completeness what I was referring to...

Without this PR with SpawnMultiCursorUp and MouseMultiCursor:

With this PR with SpawnMultiCursorUp and MouseMultiCursor:

I would expect to adapt the behavior of MouseMultiCursor to the one of SpawnMultiCursorUp resp. SpawnMultiCursorDown and don't create a cursor on the first o of bool.
But in case this isn't the common sense, we can add it as is too. Maybe I search analogy where non is and non shall be.

[dmaluka]

MouseMultiCursor just spawns a cursor wherever you click with the mouse, without any relation of to the previous cursor's position, right? So what does MouseMultiCursor have to do with anything here?

[JoeKar]

MouseMultiCursor just spawns a cursor wherever you click with the mouse, without any relation of to the previous cursor's position, right?

Hm, yes. I was more thinking about the block-selection scenario (with the mouse), which MouseMultiCursor doesn't realize.
In a real block-/row-selection scenario selected with the mouse we should have the same behavior, but not in this random selection case.

So what does MouseMultiCursor have to do with anything here?

You're right, nothing. Forget about it.