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

actions: Perform Cursor(Page)Down with selection like GUI editors do #3540

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JoeKar
Copy link
Collaborator

@JoeKar JoeKar commented Nov 20, 2024

#3091 introduced a small regression in case the actual selection included the new line character, which was ignored on CursorDown.
This PR shall fix this by moving the cursor to the start of the line.

Fixes #3476

downN := 1
if h.Cursor.HasSelection() {
if h.Cursor.CurSelection[1].X == 0 {
h.Cursor.Deselect(true)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks completely wrong. Try selecting any number of lines other than 2 and pressing Down and see what happens.

How about something straightforward like:

--- a/internal/action/actions.go
+++ b/internal/action/actions.go
@@ -254,8 +254,12 @@ func (h *BufPane) CursorUp() bool {
 
 // CursorDown moves the cursor down
 func (h *BufPane) CursorDown() bool {
+	selectionEndNewline := h.Cursor.HasSelection() && h.Cursor.CurSelection[1].X == 0
 	h.Cursor.Deselect(false)
 	h.MoveCursorDown(1)
+	if selectionEndNewline {
+		h.Cursor.Start()
+	}
 	h.Relocate()
 	return true
 }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...Also probably CursorPageDown() also needs a similar fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks completely wrong. Try selecting any number of lines other than 2 and pressing Down and see what happens.

Hm, damn! Thanks.

How about something straightforward like:

But the cursor will be placed at the same line which was already highlighted by the ruler, which is kind of strange.
gnome-text-editor moves the cursor below the line the in the same scenario and this was the intention to increase the down-move by one in case we run into "selectionEndNewline"...but we can perform a second h.MoveCursorDown(1) too, if you agree.
What does your GUI editor do in this case?

...Also probably CursorPageDown() also needs a similar fix.

Yep, indeed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference from GUI editors is that micro doesn't display the cursor and doesn't highlight the current line when there is a selection. So when there is a selection, from the user perspective there is no such thing as "cursor position" or "current line", there is only "selection position". (Especially since Up and Down keys then move the cursor respectively above the first line and below the last line of the selection, regardless of whatever micro internally tracks as "current cursor position".)

If micro still highlights the line number in the ruler when there is a selection, perhaps that in itself is a problem to be fixed? I.e. since micro doesn't highlight the current line if there is a selection, it shouldn't highlight its number in the ruler either?

I'm using a colorscheme that doesn't highlight the current line number in the ruler at all (i.e. current-line-number has the same colors as line-number). Some other users may just completely disable the ruler.

So I don't think extra MoveCursorDown(1) is a good idea.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] and doesn't highlight the current line when there is a selection. [...] I.e. since micro doesn't highlight the current line if there is a selection, it shouldn't highlight its number in the ruler either?

I didn't realize this so far or wasn't really aware of. For the sake of consistency this should be aligned to each other. Remove both or display both, but not just one of both.

BTW: gnome-text-editor does something similar. Without selection it highlights the ruler in bold and the line, while with selection it doesn't highlight the line, but the rule in non-bold instead. So you still see, if the selection ends before or after the new line in the ruler too. Other editors could behave different...

So I don't think extra MoveCursorDown(1) is a good idea.

Plausible in the moment where we don't highlight the ruler number any longer.

So when there is a selection, from the user perspective there is no such thing as "cursor position" or "current line", there is only "selection position".

Ok and how about the additional Cursor.Right() to place the cursor behind the end of the last selection?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW: gnome-text-editor does something similar. Without selection it highlights the ruler in bold and the line, while with selection it doesn't highlight the line, but the rule in non-bold instead.

On my Debian, gnome-text-editor still highlights both the ruler and the line itself when there is a selection.

Ok and how about the additional Cursor.Right() to place the cursor behind the end of the last selection?

Initially it seemed like a good idea to me. But now I've thought more about it (after thinking "maybe we should rather remove Move(-1, c.buf) from Deselect() instead?")... From a pure utilitarian perspective it probably doesn't make much difference whether we do this extra Cursor.Right() or not, while from consistency / visual niceness perspective: since the cursor is not displayed, the visually perceived "selection end" is at the last character of the selection, not right after it. So with this extra Cursor.Right(), it might be a bit annoying/unexpected that pressing Down moves the cursor not just below the last selection but slightly to the right of it. Also it might seem a bit inconsistent with other cases when Deselect(false) is used, i.e. our selectionEndNewline case and cursor movements to the right (CursorRight, WordRight, ...).

Copy link
Collaborator Author

@JoeKar JoeKar Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not really convinced by it.

the cursor is not displayed, the visually perceived "selection end" is at the last character

...and therefore including it as well.

Example:

test
test

Selecting the first "test" from left to right:

|test string (unmoved)
T|est string (1. Shift-Right)
TE|st string (2. Shift-Right)
TES|t string (3. Shift-Right)
TEST| string (4. Shift-Right)

tes|t string (Down)

Selecting the first "test" from right to left:

test| string (unmoved)
tes|T string (1. Shift-Left)
te|ST string (2. Shift-Left)
t|EST string (3. Shift-Left)
|TEST string (4. Shift-Left)

tes|t string (Down)

Compared to gnome-text-editor:

[...]

test| string (Down)

May look nice from cursor block highlighting point of view, but is weird from cursor movement point of view.

@JoeKar JoeKar changed the title actions: Perform CursorDown with selection like GUI editors do actions: Perform Cursor(Page)Down with selection like GUI editors do Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants