Vimode: fix cursor position after using undo #1328

scresto09 · 2024-04-16T10:15:32Z

Trying to get more Vim-like behavior for cursor position after using undo (U key)

techee · 2024-04-25T15:52:47Z

Seems to make sense, just curious when the problem exactly happens - could you give an example when the output of the plugin differs from vim?

scresto09 · 2024-04-25T17:07:42Z

Sure, here's an example:

Open a 5 line file
Move the cursor to line 2
press "2dd" to delete 2 lines
press "u" to cancel

With a real VIM, the cursor stays on line 2
With Geany Vimode not patched, the cursor goes to the last line

techee · 2024-05-08T15:07:46Z

But this isn't really what vim does, is it? Try the following with this patch:

Delete a line in the middle of a file.
Scroll to the top of the file so you have the cursor e.g. on the first line
Press u

The undo happens but your cursor is still on the first line instead of being at the position of the change which is very confusing. I think what you want instead is to get the information where the undo happened and move the cursor to the first line of the diff. Maybe you could achieve this using SC_PERFORMED_UNDO (not sure, I haven't studied how exactly this works and if it provides enough information).

scresto09 · 2024-05-13T15:06:22Z

You're right, my previous correction was not correct.

327cdde is a new fix using the SC_MOD_BEFOREINSERT and SC_PERFORMED_UNDO events to save the cursor position.

techee

Looks good to me apart from the few formal comments above and the possible goto_nonempty() use. Would you incorporate those changes into this PR?

techee · 2024-05-15T16:21:45Z

vimode/src/cmds/edit.c

@@ -162,14 +163,14 @@ void cmd_del_word_left(CmdContext *c, CmdParams *p)
 void cmd_undo(CmdContext *c, CmdParams *p)
 {
 	gint i;
-	gint pos = SSM(p->sci, SCI_GETCURRENTPOS, 0, 0);
+	start_undo(c);
 	for (i = 0; i < p->num; i++)
 	{
 		if (!SSM(p->sci, SCI_CANUNDO, 0, 0))
 			break;
 		SSM(p->sci, SCI_UNDO, 0, 0);
 	}


Shouldn't goto_nonempty() be used here to go to the first non-white character on a line? vim seems to behave this way, even though in a bit inconsistent manner - when you perform dd, undo goes to the first non-white character while when you use 2dd, it goes where the cursor was before.

If goto_nonempty() is used, it should only be run when some actual undo happened and also when the cursor is at the beginning of a line so it doesn't jump somewhere else when you e.g. delete a word in the middle of a sentence.

OK, I made these changes and I am now using goto_nonempty() 2b99b71.
That's right, this is probably the simplest way to approach VIM's behavior.

Well, what I had in mind was something like (beware, totally untested!!!):

void cmd_undo(CmdContext *c, CmdParams *p) { gboolean undo_performed = FALSE; gint pos; gint i; undo_start(c); for (i = 0; i < p->num; i++) { if (!SSM(p->sci, SCI_CANUNDO, 0, 0)) break; SSM(p->sci, SCI_UNDO, 0, 0); undo_performed = TRUE; // <--- this is added } undo_end(c); // the code below is added pos = SSM(ctx->sci, SCI_GETCURRENTPOS, 0, 0); if (undo_performed && is_start_of_line(p->sci, pos)) goto_nonempty(p->sci, pos, FALSE); }

without the need to pass anything extra in the context. Am I missing something? Since I didn't run or even compile it, it's quite possible.

excmd_undo() could just call this function then.

OK, I tried to refactor the code 63b7e25.
But excmd_undo cannot simply call cmd_undo because of the use of CmdParams parameter.
So I moved this code into a new undo_apply function which is called by cmd_undo and excmd_undo.

vimode/src/cmds/undo.c

techee · 2024-05-15T16:25:08Z

vimode/src/cmds/undo.h

+
+#include "context.h"
+
+void start_undo(CmdContext *c);


I'd prefer the names to start with undo_, so e.g. undo_start() here.

techee · 2024-05-15T16:25:33Z

vimode/src/cmds/undo.c

+	c->undo_pos = pos;
+}
+
+gboolean end_undo(CmdContext *c)


Is the gboolean return value used for anything?

techee · 2024-05-15T16:26:40Z

vimode/src/cmds/undo.c

+{
+	ScintillaObject *sci = c->sci;
+
+	if (c->undo_pos != -1) {


{ on a separate line to match the style of the rest of the code.

Also two empty lines between functions in the file if possible.

techee · 2024-05-15T16:26:55Z

vimode/src/cmds/undo.h

@@ -0,0 +1,28 @@
+/*
+ * Copyright 2024 Jiri Techet <[email protected]>


Like above.

techee · 2024-05-15T16:27:44Z

vimode/src/cmds/undo.h

+
+void start_undo(CmdContext *c);
+void update_undo(CmdContext *c, gint pos);
+gboolean end_undo(CmdContext *c);


void instead of gboolean?

techee · 2024-05-18T15:16:54Z

vimode/src/cmds/edit.c

@@ -162,14 +163,14 @@ void cmd_del_word_left(CmdContext *c, CmdParams *p)
 void cmd_undo(CmdContext *c, CmdParams *p)
 {
 	gint i;
-	gint pos = SSM(p->sci, SCI_GETCURRENTPOS, 0, 0);
+	start_undo(c);
 	for (i = 0; i < p->num; i++)
 	{
 		if (!SSM(p->sci, SCI_CANUNDO, 0, 0))
 			break;
 		SSM(p->sci, SCI_UNDO, 0, 0);
 	}


Well, what I had in mind was something like (beware, totally untested!!!):

void cmd_undo(CmdContext *c, CmdParams *p) { gboolean undo_performed = FALSE; gint pos; gint i; undo_start(c); for (i = 0; i < p->num; i++) { if (!SSM(p->sci, SCI_CANUNDO, 0, 0)) break; SSM(p->sci, SCI_UNDO, 0, 0); undo_performed = TRUE; // <--- this is added } undo_end(c); // the code below is added pos = SSM(ctx->sci, SCI_GETCURRENTPOS, 0, 0); if (undo_performed && is_start_of_line(p->sci, pos)) goto_nonempty(p->sci, pos, FALSE); }

without the need to pass anything extra in the context. Am I missing something? Since I didn't run or even compile it, it's quite possible.

excmd_undo() could just call this function then.

vimode/src/utils.c

techee · 2024-05-21T21:33:20Z

Looks good, thanks!

One last request - would you squash all the commits into one? I'll merge it afterwards.

techee · 2024-05-21T21:36:37Z

Also prefix the commit message with vimode: so it's clear what plugin the commit modifies.

scresto09 · 2024-05-22T08:54:12Z

OK, I did it, is it okay?
Thanks

techee · 2024-05-22T15:10:08Z

Looks great, thanks!

techee added the plugin/vimode label Apr 23, 2024

techee requested changes May 15, 2024

View reviewed changes

techee requested changes May 18, 2024

View reviewed changes

techee approved these changes May 21, 2024

View reviewed changes

vimode: fix cursor position after using undo (u key)

cc7d25d

scresto09 force-pushed the vimode-fix-undo-pos branch from 63b7e25 to cc7d25d Compare May 22, 2024 08:52

techee merged commit 0e299a8 into geany:master May 22, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vimode: fix cursor position after using undo #1328

Vimode: fix cursor position after using undo #1328

scresto09 commented Apr 16, 2024

techee commented Apr 25, 2024

scresto09 commented Apr 25, 2024

techee commented May 8, 2024

scresto09 commented May 13, 2024

techee left a comment

techee May 15, 2024

scresto09 May 17, 2024 •

edited

Loading

techee May 18, 2024

scresto09 May 21, 2024

techee May 15, 2024

techee May 15, 2024

techee May 15, 2024

techee May 15, 2024

techee May 15, 2024

techee May 15, 2024

techee May 18, 2024

techee commented May 21, 2024

techee commented May 21, 2024

scresto09 commented May 22, 2024

techee commented May 22, 2024

		@@ -0,0 +1,28 @@
		/*
		* Copyright 2024 Jiri Techet <[email protected]>

Vimode: fix cursor position after using undo #1328

Vimode: fix cursor position after using undo #1328

Conversation

scresto09 commented Apr 16, 2024

techee commented Apr 25, 2024

scresto09 commented Apr 25, 2024

techee commented May 8, 2024

scresto09 commented May 13, 2024

techee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scresto09 May 17, 2024 • edited Loading

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

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

techee commented May 21, 2024

scresto09 commented May 22, 2024

techee commented May 22, 2024

scresto09 May 17, 2024 •

edited

Loading