Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cursor moving down when selection exist. Solves (#3087) #3091

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

dustdfg
Copy link
Contributor

@dustdfg dustdfg commented Dec 13, 2023

Previously CursorDown function called Deselect with a wrong argument which lead to the situation when cursor was moved to the start instead of the end of the selection. Solves (#3087)

Previously `CursorDown` function called `Deselect` with a wrong
argument which lead to the situation when cursor was moved to the
start instead of the end of the selection

Signed-off-by: Yevhen Babiichuk (DustDFG) <dfgdust@gmail.com>
@dustdfg
Copy link
Contributor Author

dustdfg commented Dec 13, 2023

@semirke I think it would be interesting for you because of semirke@b345ec7

@semirke
Copy link

semirke commented Dec 13, 2023

@semirke I think it would be interesting for you because of semirke@b345ec7

hi,
ye, your solution seems much more on spot (y) (y)
thanks :)

@dmaluka
Copy link
Collaborator

dmaluka commented Apr 15, 2024

What if the user selects 1000 lines from bottom up and then presses the down key?

It seems we need a proper fix instead. Simply passing a constant value to Deselect(), be it true or false, doesn't cut it.

@dustdfg
Copy link
Contributor Author

dustdfg commented Apr 16, 2024

What if the user selects 1000 lines from bottom up and then presses the down key?

Are you about mouse selection? If yes, so it is not a problem of deselection logic it is a problem of proper storing start and end positions when you select with mouse (which can be solved with several lines... When we select with mouse we just need to check if start is smaller than end and if not just exchange values...)

Everything will be ok.... We have two points for selection. start and end. Deselect is designed to set cursor in one of those position depending on what it received true or false... Just also look at CursorRight and CursorLeft

@semirke
Copy link

semirke commented Apr 16, 2024

What if the user selects 1000 lines from bottom up and then presses the down key?

It seems we need a proper fix instead. Simply passing a constant value to Deselect(), be it true or false, doesn't cut it.

Hi,
Im not sure if we are on the same page or why the number of lines matter (?), but I believe the cursor should always exit a selection on the bottom if down key was pressed.
Isn't that the actual behaviour after patch?

Thanks,
Semirke

@dustdfg
Copy link
Contributor Author

dustdfg commented Apr 16, 2024

I don't know about what @dmakula says but no, this pr isn't enough for "always going down". There is also one problem: If you select with keyboard it doesn't matter from where you started from the end or from the start, but if you select with mouse the place from where you started is considered as start of selection even if after that you will drag mouse higher... So when you select with mouse from the end to start micro think that start is end and that end is start...

@semirke
Copy link

semirke commented Apr 16, 2024

this pr isn't enough for "always going down".
Wdym?
Sorry, I checked my branch and that does always go down, which is the expected behav I guess. (Most editors do that if not all.)

However, Im not using mouse bc it messes up my clipboard, so I cannot confirm or deny that.

@dmaluka
Copy link
Collaborator

dmaluka commented Apr 16, 2024

Hold Shift, press PageUp several times and release Shift. You now have a large selection (several screens) and you are at the top of this selection, not at the bottom of it. Now press Down key. You probably expect the cursor to remain at the top of the selection, just move one line down, right? (And that is what most editors do, if not all.) But with this PR the cursor will suddenly jump to the bottom of the selection instead.

Deselect is designed to set cursor in one of those position depending on what it received true or false... Just also look at CursorRight and CursorLeft

Yes, and my point is that this design is broken.

So, merging this PR would not make things worse, but rather would not make things better either. (To be completely precise: it fixes cursor down behavior after a "top to down" selection, but breaks cursor down behavior after a "bottom to up" selection, so "on average" it doesn't make things better or worse.) It just implements following this broken Deselect design in a more "consistent" way. I think we should fix this broken design instead.

In other words, we should make it take into account the direction of selection.

CursorRight and CursorLeft are broken too, for the same reason.

And yes, with mouse selection it is also broken, although in a different way.

I see @dustdfg already reported both issues in #3055 (I wasn't aware of that).

@semirke
Copy link

semirke commented Apr 16, 2024

You probably expect the cursor to remain at the top of the selection, just move one line down, right?

Hi,
no I dont. If you check even in github's editor you will end up below the selection, and all other editors work like that.

@dmaluka
Copy link
Collaborator

dmaluka commented Apr 16, 2024

Indeed, github's editor behaves like you expect (but who uses it?).

I've just tried emacs, nano, gedit and geany. Gedit is the only one that behaves as you'd expect. Emacs, nano and geany all behave as I'd expect.

When it comes to the behavior with mouse selection, gedit is consistent with its own behavior with keyboard selection, i.e. it doesn't care about the direction of selection (unlike micro which, in the case of mouse selection, does care about the direction, but for some reason incorrectly gets it backwards).

Ok, since there are some editors with such "direction agnostic" behavior, and perhaps there even some users that like this behavior, maybe for now we should just merge this PR, just to make the present micro's "direction agnostic" direction agnostic behavior for keyboard selections work consistently, in the short term.

Still, we should also address the inconsistency of keyboard vs mouse selections (#3055), so we should decide in favor of which behavior we should address it, i.e. should we make both keyboard and mouse selections "direction agnostic" (like github, gedit) or both "direction aware" (like emacs, nano, geany). Perhaps the former is just much easier to implement (much less rework needed in the existing code). But what is better for the users?

@semirke
Copy link

semirke commented Apr 16, 2024

I've just tried emacs, nano, gedit and geany. Gedit is the only one that behaves as you'd expect. Emacs, nano and geany all behave as I'd expect.

Then I'd propose to add a setting parameter when it's being fixed, bc it seems like a personal preference.
Most (non-cli) editors I used behave like github's and I do see the logic in it.

@dmaluka
Copy link
Collaborator

dmaluka commented Apr 16, 2024

Then I'd propose to add a setting parameter

I'd propose not to, unless really needed.

I'd personally would be fine with either behavior, as a user I've never paid attention to this particular aspect of the cursor behavior.

@dustdfg
Copy link
Contributor Author

dustdfg commented Apr 16, 2024

Nano uses the go to the end approach...

It is very expected and consistent in my opinion. If you selected text and press right or left, you expect to go to the "edge" of the selection.

One more example: Do you remember the filemanager plugin? So the author expected the behavior of this pr and considered current behavior as bug:

https://github.com/micro-editor/updated-plugins/blob/216ec3adaf3adec78665614402ece56cf60ae713/filemanager-plugin/filemanager.lua#L1024-L1037

@dustdfg
Copy link
Contributor Author

dustdfg commented Apr 16, 2024

Nano works as current mouse selection in micro with messing the start and the end edges...

@dmaluka
Copy link
Collaborator

dmaluka commented Apr 25, 2024

Ok, let's merge this PR then. I've also uploaded PR #3268 which fixes the inconsistent mouse selection behavior. With both PRs together, both keyboard and mouse selections should work as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants