Trying to get more Vim-like behavior for cursor position after using undo (U key) You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany-plugins/pull/1328
-- Commit Summary --
* fix cursor position after using undo (u key)
-- File Changes --
M vimode/src/cmds/edit.c (2)
-- Patch Links --
https://github.com/geany/geany-plugins/pull/1328.patch https://github.com/geany/geany-plugins/pull/1328.diff
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?
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
But this isn't really what vim does, is it? Try the following with this patch: 1. Delete a line in the middle of a file. 2. Scroll to the top of the file so you have the cursor e.g. on the first line 3. 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 pushed 1 commit.
327cdde2d506400ad6e05342c0d6095e9c254e0e use SC_MOD_BEFOREINSERT and SC_PERFORMED_UNDO events to track cursor position to restore after undo
You're right, my previous correction was not correct.
[327cdde](https://github.com/geany/geany-plugins/pull/1328/commits/327cdde2d506400ad6e...) is a new fix using the SC_MOD_BEFOREINSERT and SC_PERFORMED_UNDO events to save the cursor position.
@techee requested changes on this pull request.
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?
@@ -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.
@@ -0,0 +1,41 @@
+/* + * Copyright 2024 Jiri Techet techet@gmail.com
Your file, your name :-). You can skip the email address if you are worried of spam.
- This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +#ifndef __UNDO_H__ +#define __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.
- */
+ +#include "undo.h" +#include "utils.h" + +void start_undo(CmdContext *c) +{ + c->undo_pos = -1; +} + +void update_undo(CmdContext *c, gint pos) +{ + c->undo_pos = pos; +} + +gboolean end_undo(CmdContext *c)
Is the gboolean return value used for anything?
+void start_undo(CmdContext *c) +{ + c->undo_pos = -1; +} + +void update_undo(CmdContext *c, gint pos) +{ + c->undo_pos = pos; +} + +gboolean end_undo(CmdContext *c) +{ + ScintillaObject *sci = c->sci; + + if (c->undo_pos != -1) {
{ on a separate line to match the style of the rest of the code.
@@ -0,0 +1,28 @@
+/* + * Copyright 2024 Jiri Techet techet@gmail.com
Like above.
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +#ifndef __UNDO_H__ +#define __UNDO_H__ + +#include "context.h" + +void start_undo(CmdContext *c); +void update_undo(CmdContext *c, gint pos); +gboolean end_undo(CmdContext *c);
void instead of gboolean?
@techee commented on this pull request.
+void start_undo(CmdContext *c) +{ + c->undo_pos = -1; +} + +void update_undo(CmdContext *c, gint pos) +{ + c->undo_pos = pos; +} + +gboolean end_undo(CmdContext *c) +{ + ScintillaObject *sci = c->sci; + + if (c->undo_pos != -1) {
Also two empty lines between functions in the file if possible.
@scresto09 pushed 1 commit.
2b99b719de64166b3833280a5ba0c1fdf00c6a4c used goto_nonempty to go to the first non-white character and some more changes
@scresto09 commented on this pull request.
@@ -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); }
OK, I made these changes and I am now using `goto_nonempty()`. This is probably the simplest way to approach VIM's behavior.
@scresto09 pushed 1 commit.
1be4e6adeb28c19ac35c9a9682d21c35f08532f3 added two empty lines
@techee requested changes on this pull request.
@@ -219,3 +220,12 @@ void goto_nonempty(ScintillaObject *sci, gint line, gboolean scroll)
pos = NEXT(sci, pos); SET_POS(sci, pos, scroll); } + + +gboolean is_start_of_line(ScintillaObject *sci, gint pos)
This function can go directly above the function where it's used and be static - there doesn't seem to be any need for it in the rest of the code.
@@ -219,3 +220,12 @@ void goto_nonempty(ScintillaObject *sci, gint line, gboolean scroll)
pos = NEXT(sci, pos); SET_POS(sci, pos, scroll); } + + +gboolean is_start_of_line(ScintillaObject *sci, gint pos) +{ + gint line = SSM(sci, SCI_LINEFROMPOSITION, pos, 0); + gint line_pos = SSM(sci, SCI_POSITIONFROMLINE, line, 0); + + return pos == line_pos ? TRUE : FALSE;
Drop unnecessary `? TRUE : FALSE`.
@@ -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!!!): ```C 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.
github-comments@lists.geany.org