Launch editor at line from diff view

[NotSqrt]

Fixes #893

[NotSqrt]

It may need thorough testing, I just tested a few cases in the diff view, and checked that it does not fail in some other "Launch editor" contexts (Status, Commit).

[davvid]

Thanks for PR, it looks like you're on the right track. I had a few notes, so let me know what you think. This is great feature.

[davvid]

This line (where we're passing a widget to a command) is a sign that we need to refactor a little.

The logic below should probably move into the (numbers) widget and so that the diff widget can push the line_number value into the selection model.

That will then make it available to the command here via the context.selection object. That does seem like the right place for the line number to be stored.

The question does remain as to where exactly to do the update. The DiffEditor class should probably take care of it by either modifying the DiffTextEdit to make the _cursor_changed callback "public" by renaming it, and overriding it in the subclass so that we can tack on the extra behavior of updating the selection model.

Alternatively, the DiffEditor could be a little more decoupled and instead register its own self.cursorPositionChanged.connect(...) callback instead, and push the value over there. That seems better since it's simpler and less coupled.

[NotSqrt]

I imagine the following updates:

# in cola.widgets.diff
class DiffTextEdit(VimHintedPlainTextEdit):
    def _cursor_changed(self):
        """Update the line number display when the cursor changes"""
        line_number = max(0, self.textCursor().blockNumber())
        if self.numbers:
            self.numbers.set_highlighted(line_number)
+           newfile_line = self.numbers.newfile_line()


class DiffLineNumbers:
+    def newfile_line(self):
+        if self.lines and self.highlight_line >= 0:
+            # Find the next valid line
+            for i in range(self.highlight_line, len(self.lines)):
+                # take the "new" line number: last value in tuple
+                line_number = self.lines[i][-1]
+                if line_number > 0:
+                    return line_number
+
+            # Find the previous valid line
+            for i in range(self.highlight_line - 1, -1, -1):
+                # take the "new" line number: last value in tuple
+                line_number = self.lines[i][-1]
+                if line_number > 0:
+                    return line_number

But then, I don't quite see how the context could work to propagate that newfile_line value into the context of LaunchEditor.
I see the context gets created in new_context.
But after a quick look at the SelectionModel, I need your confirmation that the line_number fits (it's relevant to the current file, so maybe its place is near SelectionModel.filename. I guess the line_number does not have to survive a union).

[davvid]

I was thinking we could just have a new member variable in the SelectionModel constructor that defaults to 0, e.g. line_number. self.context.selection.line_number = line_number in the DiffEditor's callback can be used to store it.

We don't want that behavior on the base class because it's used by other widgets, which shouldn't be updating the semi-global line number.

When the diff command runs, it's the same context object, so you can get the value back out via the same self.context reference.

[NotSqrt]

Done, let me know what you think.

[davvid]

Cool, this is looking much better. So there's one last bit that will require a tiny bit of adjustment.

We've now modified the default LaunchEditor command, which is provided via the launch_editor actions function in the DiffEditor widget.

The problem is now that every LaunchEditor command is going to pickup the line number from the diff editor, and that's probably unwanted. For example, that variant only works with a single file, and things like the status widget have the ability to launch an edit against multiple files in a single shot.

Other widgets will lose that ability as well.

The next step would then be to expose a new LaunchEditorAtLine command which subclasses LaunchEditor for this new functionality. It can still reuse the same name() and UI labels to keep translations simple.

Then a new actions function launch_editor_at_line can be exposed and it can be used from the DiffEditor class only. That way it's only that class that gets this new behavior, and all of the other places where LaunchEditor is used will be unaffected.

Thanks for working through this, we're super close now.

[davvid]

It seems like the end of this function should probably have return 0 as a fallback, or maybe return None is more appropriate so that it avoids the line_numbers code path in the Edit command.

[davvid]

Last tweak -- if we're going to override _cursor_changed from the base class then we should rename it to cursor_changed (without the leading underscore) to signal that it's a a little more "public". I try to avoid touching underscore things outside of the same class.

[davvid]

These last issues were smaller so I went ahead and merged this as-is with some tweaks on top. Please take a look.

BTW, it looked like there was a bug in the range(self.highlight_line, len(self.lines)) line because it was missing a + 1 after the len(...). I changed it to using slices, which changes the semantics, so please double-check that this is okay (and that you didn't intentionally skip the last line).

I can take a look at separating out the action+command unless you beat me to it. thanks again!

[NotSqrt]

@davvid Perfect !

Yes, it's indeed better with slices !

Thanks a lot for the quick feedback !