Vimode: Fix cursor hang with folded lines #1326

scresto09 · 2024-04-15T15:04:02Z

Fix cursor hang when we want to move cursor on top line and this line is folded

To reproduce the problem, when vimode plugin is enabled:
Fold a few lines by clicking the "minus" icon.
Move the cursor to the bottom of these lines and try to move back to the top.
The cursor doesn't go back and seem to hang.

… is folded

techee · 2024-04-25T15:50:50Z

I'm not sure this is the correct way to address the issue. I think we should take the folds into account and perform the motion commands on top of the visible lines instead of the document lines. I tried to replace this PR with

#1338

Please let me know what you think about the changes.

scresto09 · 2024-05-07T14:16:57Z

In response to pull request #1338

After read the https://sourceforge.net/p/scintilla/bugs/2438/ report, I think the best way to automatically set the cursor position on the visible line is surely to track the SCN_MARGINCLICK event. I did some modification to handle this with the commit 5608b74 .

techee · 2024-05-08T12:22:00Z

After read the https://sourceforge.net/p/scintilla/bugs/2438/ report, I think the best way to automatically set the cursor position on the visible line is surely to track the SCN_MARGINCLICK event. I did some modification to handle this with the commit 5608b74 .

Thanks for noticing the problem with line wrapping. I think your general approach is right - I've just slightly rewritten the doc_line_from_visible_delta() function to be easier to grasp by my poor brain, please check fa7025b if it looks good to you.

There are a few things in your patch which I don't think are completely correct though - I'll add some inline comments to your code.

techee · 2024-05-08T12:23:07Z

vimode/src/cmds/motion.c

+static gint doc_line_from_visible_delta(CmdParams *p, gint line, gint delta, gint *previous)
+{
+	gint step = delta < 0 ? -1 : 1;
+	gint new_line = p->line;


Should be line instead of p->line - we don't always pass p->line as the parameter of this function.

techee · 2024-05-08T12:31:49Z

vimode/src/cmds/motion.c

-		pos = SSM(p->sci, SCI_GETLINEENDPOSITION, one_above, 0);
+
+	if (previous > -1) {
+		guint pos = SSM(p->sci, SCI_POSITIONFROMLINE, previous, 0); 


I think you didn't understand the purpose of this strange "one above" and then SCI_LINEDOWN dance. While you get to the correct line alright, you lose the cursor's x position on the line this way. Notice how, when moving cursor up and down, it remembers the maximum x coordinate on the line and even after bing on lines where the maximum x is lower *like e.g. an empty line where x is 0), it returns back to the right position on longer lines.

Unfortunately this "maximum x" is not possible to obtain from Scintilla but Scintilla automatically recovers it when doing SCI_LINEDOWN or SCI_LINEUP. So the trick here is to first go to the line above or below, and then perform SCI_LINEDOWN or SCI_LINEUP to get us to the right line and get the x position of the cursor. When you remove this code, you'll always end with the x position at the beginning of the line.

Yes, I think I understood the problem you explained correctly.

But from what I understood, for cmd_goto_up this method is only really necessary when the line you want to go to is not the previous visible line.

With this fix my idea was to have "doc_line_from_visible_delta" fill the "previous" variable with the line number just below the visible line to access it with a SET_POS_NOX, then in any case do a SCI_LINEUP to go to the desired line.

Same for goto_down and SCI_LINEDOWN.

OK, so it was me who misunderstood your code :-).

Even though your version works, I'll probably go for #1338 which is easier for me to understand - unless you have some problems with it.

techee · 2024-05-08T12:38:32Z

vimode/src/cmds/motion.c

+		SET_POS_NOX(p->sci, pos, FALSE);
+	}
+
+	if (new_line > p->line) SSM(p->sci, SCI_LINEDOWN, 0, 0);


Is all this code necessary? Maybe I'm missing something but I did just this:

fa7025b#diff-15a3a15a958bc6f85e0f1fae419e23c60bbee47bf617ef133f7539fc1f29b277R128

doc_line_from_visible_delta() only returns a line from the valid line range and when you are on the first line, this is already the "line above" for which you then perform SCI_LINEDOWN so it should be OK on this side. On the other end of the document when you are on the last line, SCI_LINEDOWN just won't do anything so there's no need for some special handling there.

It's just a detail but this code is useful to have the same behavior as VIM.
With VIM, when the cursor is on the last line but not the last character and you press the down arrow, the cursor does not move.
Without this code, the cursor will move to the end of the line.

Alright, yeah, the previous code handled right that, I just forgot why exactly it was there. I've fixed it in my PR.

techee

Looks good except for the minor comments below.

I was actually thinking about not doing this at all because it doesn't cover all fold/unfold situations like e.g. using Geany's keybindings for folding but the patch is simple and better than nothing so let's do it.

Would you post this as a separate pull request? If #1338 looks good to you, I'd merge that PR after which this one could be merged.

techee · 2024-05-08T13:16:52Z

vimode/src/utils.c

+	gint fold_parent = line;
+
+	/* go through the parents as long as they are not visible */
+	while (SSM(sci, SCI_GETLINEVISIBLE, fold_parent, 0) == FALSE) {


Better !SSM(...) instead of the FALSE comparison.

Also place { on separate line to match the other code style.

techee · 2024-05-08T13:17:15Z

vimode/src/utils.c

+	while (SSM(sci, SCI_GETLINEVISIBLE, fold_parent, 0) == FALSE) {
+		gint prev_parent = SSM(sci, SCI_GETFOLDPARENT, fold_parent, 0);
+
+		if (prev_parent == -1) break;


break on separate line.

techee · 2024-05-08T13:18:33Z

vimode/src/utils.c

+	{
+		/* move the cursor on the visible line before the fold */
+		gint pos = SSM(sci, SCI_POSITIONFROMLINE, fold_parent, 0);
+		SET_POS(sci, pos, TRUE);


Use SET_POS_NOX() instead. This one will preserve the "maximum x" cursor value so when further moving up or down, this one will be recovered.

techee · 2024-05-08T13:18:58Z

vimode/src/utils.h

@@ -32,5 +32,8 @@ void perform_substitute(ScintillaObject *sci, const gchar *cmd, gint from, gint
 	const gchar *flag_override);

 gint get_line_number_rel(ScintillaObject *sci, gint shift);
+void ensure_current_line_expanded(ScintillaObject *sci);


Again, remove, it's from another patch.

techee · 2024-05-08T13:19:37Z

vimode/src/vi.c

@@ -306,6 +306,14 @@ gboolean vi_notify_sci(SCNotification *nt)
 		}
 	}

+	if (nt->nmhdr.code == SCN_MARGINCLICK) {


{ on separate line.

I've just merged #1338 which functionality-wise covers most of this PR but doesn't contain the SCN_MARGINCLICK part. Would you create a separate pull request containing it?

techee · 2024-05-21T22:05:52Z

Closing this PR as #1338 was merged. Having SCN_MARGINCLICK handled would however be worth adding as a separate PR.

scresto09 · 2024-05-22T13:04:52Z

OK, I just create #1349 for SCN_MARGINCLICK part.
Thanks

Fix cursor hang when we want to move cursor on top line and this line…

5709047

… is folded

scresto09 closed this Apr 15, 2024

Merge branch 'geany:master' into vimode-fix-fold-up

be15445

scresto09 reopened this Apr 16, 2024

scresto09 changed the title ~~Fix cursor hang with folded lines~~ Vimode: Fix cursor hang with folded lines Apr 16, 2024

techee added the plugin/vimode label Apr 23, 2024

Automatically set the cursor position on the visible line

5608b74

try to fix some issues of the plugin with folded lines

274ff97

scresto09 mentioned this pull request May 7, 2024

vimode: Handle folded lines correctly #1338

Merged

techee reviewed May 8, 2024

View reviewed changes

techee requested changes May 8, 2024

View reviewed changes

techee closed this May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vimode: Fix cursor hang with folded lines #1326

Vimode: Fix cursor hang with folded lines #1326

scresto09 commented Apr 15, 2024

techee commented Apr 25, 2024

scresto09 commented May 7, 2024

techee commented May 8, 2024

techee May 8, 2024

techee May 8, 2024

scresto09 May 13, 2024

techee May 18, 2024

techee May 8, 2024

scresto09 May 14, 2024

techee May 18, 2024

techee left a comment

techee May 8, 2024

techee May 8, 2024

techee May 8, 2024

techee May 8, 2024

techee May 8, 2024

techee May 21, 2024

techee commented May 21, 2024

scresto09 commented May 22, 2024

Vimode: Fix cursor hang with folded lines #1326

Vimode: Fix cursor hang with folded lines #1326

Conversation

scresto09 commented Apr 15, 2024

techee commented Apr 25, 2024

scresto09 commented May 7, 2024

techee commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techee commented May 21, 2024

scresto09 commented May 22, 2024