[Github-comments] [geany/geany-plugins] git-changebar: Add the possibility to undo hunk at cursor position (#531)

[Colomban Wendling]

b4n requested changes on this pull request.

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 :+1: 

> +
+    if (data->found) {
+      ScintillaObject *sci = doc->editor->sci;
+
+      sci_start_undo_action(sci);
+
+      if (data->new_lines > 0) {
+        gint pos = sci_get_position_from_line(sci, data->new_start - 1);
+        sci_set_target_start(sci, pos);
+        pos = sci_get_position_from_line(sci, data->new_start + data->new_lines - 1);
+        sci_set_target_end(sci, pos);
+        sci_replace_target(sci, "", FALSE);
+      }
+
+      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?

> +
+      if (data->old_lines > 0) {
+        gint line = sci_get_current_line (sci);
+        gint pos;
+
+        if (data->new_lines == 0 && !data->first_line_removed) {
+          line++; /* marker for deleted hunk is on previous line except the 1st line */
+        }
+        pos = sci_get_position_from_line (sci, line);
+
+        insert_buf_range (doc, contents, pos,
+                          data->old_start - 1,
+                          data->old_lines);
+      }
+
+      sci_scroll_caret(sci);

Similarly, what about

```C
      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));
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/531#pullrequestreview-37969835
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20170512/baab62db/attachment.html>