git-changebar: Add the possibility to undo hunk at cursor position #531

techee · 2017-02-19T17:01:28Z

The patch adds a new keybinding action for undoing hunks. When cursor is
placed at a line with a diff and the keybinding is invoked, the
corresponding hunk is undone.

To get the previous contents at the given position, a temporary Scintilla
instance is used into which the previous state of the document is loaded
and the corresponding text from the previous state of the document is
extracted. This temporary Scintilla object is then destroyed.

techee · 2017-02-19T17:05:25Z

This is something I wanted for a long time so here's an attempt to implement it.

Note that I based the patch mostly on what I have seen in git-changebar (basically combined the "goto next hunk" code and get_widget_for_buf_range()) without knowing libgit or the majority of the plugin code. So if anyone asks why I did something certain way, the answer is "because I didn't know what I was doing" :-).

techee · 2017-02-23T14:45:30Z

I've added a fix for #532.

frlan · 2017-05-01T11:32:18Z

I've got it inside local branch and it seems to work fine. But did not some kind of extended code review or something.

b4n

I didn't yet do any kind of proper review (not even tested it yet), but sounds reasonable and the feature seems great :)

I'll try and make a proper review in the upcoming days if I can get enough Internet (yeah, not sure I'll make it through without any, crossing fingers :]).

b4n · 2017-05-05T02:03:22Z

git-changebar/src/gcb-plugin.c

@@ -1044,13 +1053,13 @@ goto_next_hunk_diff_hunk_cb (const git_diff_delta *delta,
      if (data->next_line >= 0) {
        return 1;
      } else if (data->line < hunk->new_start - 1) {
-        data->next_line = hunk->new_start - 1;
+        data->next_line = (hunk->new_start == 0) ? 0 : hunk->new_start - 1;


maybe it'd be worth creating a function/macro for that, as it's repeated 3 times? Not sure of a name for it yet though :)

Maybe something like

data->next_line = REMOVED_MARKER_POS (hunk->new_start);

?

Yeah OK sounds good

b4n · 2017-05-05T02:04:22Z

git-changebar/src/gcb-plugin.c

+  old_pos_end = sci_get_position_from_line (old_sci, old_start + old_lines);
+  old_range = sci_get_contents_range (old_sci, old_pos_start, old_pos_end);
+
+  sci_insert_text(doc->editor->sci, pos, old_range);


style: space before the open parenthesis

techee · 2017-05-10T20:24:52Z

@b4n Thanks for having a look at this (I want this feature badly!!!). Please let me know if you have some other suggestions, I'll address them together with your above comments.

b4n

Looks mostly good, but I'd rather not rely on where the caret is. Not relying on it makes it fairly easy to add a right-click version of the feature (I have an unpolished version locally that work just fine, yay :)).

Otherwise, it seems to work great 👍

b4n · 2017-05-13T02:15:24Z

git-changebar/src/gcb-plugin.c

+      }
+
+      if (data->old_lines > 0) {
+        gint line = sci_get_current_line (sci);


Why current line? couldn't it be gint pos = sci_get_position_from_line (sci, data->new_start - 1); instead? I don't get exactly how you can be sure it's the current line, and not relying on that would allow for that to be used on non-current lines too (like adding an entry to the editor menu).

And I don't get why caring about position of the delete marker as we don't use them here, do we?

hum, OK, the marker comment is a bit misleading, but indeed there's the same problem here, so we indeed need a similar fix, sorry. But still not relying on the current line should be easy enough and better IMO.

gint pos = sci_get_position_from_line (sci, data->new_start - 1); returns the pos on the previous line which is wrong when undoing deleted-only diff - I need to advance it to the next line if new_lines == 0 (see a few lines below). I could probably get the length of the line and add it to pos in this case.

Just to understand - what does scintilla return from sci_get_current_line() when you right-click somewhere - I'd expect it doesn't change the current line, does it?

gint pos = sci_get_position_from_line (sci, data->new_start - 1); returns the pos on the previous line which is wrong when undoing deleted-only diff - I need to advance it to the next line if new_lines == 0 (see a few lines below).

Indeed, I was confused at first. I suggest 00e3745

Just to understand - what does scintilla return from sci_get_current_line() when you right-click somewhere - I'd expect it doesn't change the current line, does it?

No it doesn't, but Geany gives us the position of the right click, and other features in the menu work at the click position, not the caret position. So for the feature I want to do the same: propose undoing the diff at the right click position, not at the caret one. See 0c4415d for a proposal.

b4n · 2017-05-13T02:16:01Z

git-changebar/src/gcb-plugin.c

+                          data->old_lines);
+      }
+
+      sci_scroll_caret(sci);


Similarly, what about

scintilla_send_message (sci, SCI_SCROLLRANGE, sci_get_position_from_line (sci, data->new_start + data->old_lines - 1), sci_get_position_from_line (sci, data->new_start - 1));

It's actually similarly wrong, but there's a fixed version in 00e3745

techee · 2017-05-13T21:06:11Z

Looks mostly good, but I'd rather not rely on where the caret is. Not relying on it makes it fairly easy to add a right-click version of the feature

Maybe I misunderstand how you meant it but how does the right-click version know at which position to undo when it don't use the caret position? I'd expect the caret is somewhere where you want to undo, you right-click (at which point I expect Scintilla still preserves the caret position but I may be wrong) and select undo from the menu. But you still need the caret position, not?

b4n · 2017-05-14T21:00:02Z

Maybe I misunderstand how you meant it but how does the right-click version know at which position to undo when it don't use the caret position? I'd expect the caret is somewhere where you want to undo, you right-click (at which point I expect Scintilla still preserves the caret position but I may be wrong) and select undo from the menu. But you still need the caret position, not?

As said above, no: the right click undo would undo at the position of the click, which indeed doesn't move the caret, which means we need be able to undo arbitrary hunks regardless of the caret position.

b4n · 2017-05-14T21:01:29Z

A series of proposed fixes and changes: https://github.com/b4n/geany-plugins/commits/git-changebar/techee/sidebar_undo/review
Feel free to reuse, squash, comment on, bash, nitpick, throw away or anything else you could thunk of :)

b4n · 2017-05-16T23:28:05Z

BTW, you maybe could add a line to the README about the new feature :)

The patch adds a new keybinding action for undoing hunks. When cursor is placed at a line with a diff and the keybinding is invoked, the corresponding hunk is undone. To get the previous contents at the given position, a temporary Scintilla instance is used into which the previous state of the document is loaded and the corresponding text from the previous state of the document is extracted. This temporary Scintilla object is then destroyed.

Diffs for removed lines are shown on previous line - this works well except the first line where there's no previous line so nothing is displayed. This patch adds corresponding tests to the code and shows diffs for the first line on the first line. It also fixes related problems with going to previous/next hunk and undoing hunks.

Don't rely on the caret position when altering the document to undo a hunk, and use the hunk position itself instead. This allows to use the same mechanism anywhere in the document regardless of the caret position, making the code more easily reusable.

…p code

…eted hunk For deleted hunk, move cursor at the end of the undone text. This behavior corresponds to what normal undo does - when undoing deletion, cursor is also placed at the end of the newly inserted text. Also change primary position for SCI_SCROLLRANGE in this case so it becomes the cursor position.

techee · 2017-05-22T13:14:41Z

Thanks @b4n - I've finally had time to have a look at it. Your patches look good (I reviewed only the patches dealing with the undo stuff and didn't spend much time looking at the menu feature but it seems to work fine). I've just squashed your whitespace fix to the original commit and left the rest as separate commits. I've also updated README and added the REMOVED_MARKER_POS() macro.

The last patch is one more minor modification - it updates the cursor position when undoing deleted hunk - originally the cursor was placed at the position where the hunk started but I think it should be placed at the end of the undone deletion which corresponds more closely to what normal undo does. The question is what to do with modified hunks (i.e. those with both old lines and new lines). I still find it more natural to place the cursor behind them so it behaves the same way as delete-only hunk.

frlan · 2017-06-22T18:27:59Z

@b4n @techee Merging it for 1.31?

techee · 2017-06-23T20:55:32Z

@b4n @techee Merging it for 1.31?

I'm fine with it, depends on Colomban.

frlan · 2017-06-25T08:30:05Z

postponed to 1.32

vfaronov · 2017-07-19T07:14:58Z

I have been running with this change for some time. Very useful, thank you @techee and @b4n. No problems so far (I have only a few small repos though).

frlan · 2017-08-19T13:46:03Z

As I'm also running this patchset for some time now I'm going to merge it. Let us more beta test it on master.

techee assigned b4n Feb 19, 2017

techee requested a review from b4n February 19, 2017 17:01

This was referenced Feb 19, 2017

git-changebar: No diff displayed when first line in document deleted #532

Closed

git-changebar: Add menu with actions to the menubar #534

Open

frlan added the plugin/git-changebar label Feb 19, 2017

techee force-pushed the sidebar_undo branch from 7beb433 to 9313143 Compare February 27, 2017 15:34

frlan added this to the 1.31.0 milestone Mar 5, 2017

b4n reviewed May 5, 2017

View reviewed changes

b4n requested changes May 13, 2017

View reviewed changes

techee and others added 8 commits May 22, 2017 15:06

git-changebar: Fix a typo

939fe5d

git-changebar: Add an editor menu item for undoing hunks

cc6889c

git-changebar: Define REMOVED_MARKER_POS() macro and use it to cleanu…

c428a8f

…p code

git-changebar: Update documentation with the "undo hunk" feature

4242fb9

techee force-pushed the sidebar_undo branch from 9313143 to c02fc17 Compare May 22, 2017 13:07

frlan added this to the 1.32.0 milestone Jun 25, 2017

frlan removed this from the 1.31.0 milestone Jun 25, 2017

frlan merged commit 59118d8 into geany:master Aug 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

git-changebar: Add the possibility to undo hunk at cursor position #531

git-changebar: Add the possibility to undo hunk at cursor position #531

techee commented Feb 19, 2017

techee commented Feb 19, 2017

techee commented Feb 23, 2017

frlan commented May 1, 2017

b4n left a comment

b4n May 5, 2017

techee May 13, 2017

b4n May 15, 2017

b4n May 5, 2017

techee commented May 10, 2017

b4n left a comment

b4n May 13, 2017

b4n May 13, 2017

techee May 13, 2017

b4n May 14, 2017

b4n May 13, 2017

b4n May 14, 2017

techee commented May 13, 2017

b4n commented May 14, 2017

b4n commented May 14, 2017

b4n commented May 16, 2017

techee commented May 22, 2017

frlan commented Jun 22, 2017

techee commented Jun 23, 2017

frlan commented Jun 25, 2017

vfaronov commented Jul 19, 2017

frlan commented Aug 19, 2017

git-changebar: Add the possibility to undo hunk at cursor position #531

git-changebar: Add the possibility to undo hunk at cursor position #531

Conversation

techee commented Feb 19, 2017

techee commented Feb 19, 2017

techee commented Feb 23, 2017

frlan commented May 1, 2017

b4n 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

techee commented May 10, 2017

b4n 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 13, 2017

b4n commented May 14, 2017

b4n commented May 14, 2017

b4n commented May 16, 2017

techee commented May 22, 2017

frlan commented Jun 22, 2017

techee commented Jun 23, 2017

frlan commented Jun 25, 2017

vfaronov commented Jul 19, 2017

frlan commented Aug 19, 2017