jesseduffield / lazygit

simple terminal UI for git commands
MIT License
53.58k stars 1.87k forks source link

Improve clicking in diff view to enter staging/patch building #3986

Open stefanhaller opened 1 month ago

stefanhaller commented 1 month ago

When a single file (not a directory) is selected in the files panel, you can click in the diff on the right to enter staging, with the clicked line selected. This is very convenient, but there are several problem with this:

This last one is really a bummer. It is a very common use case for me to review my branch in the commits panel, and then I notice something that I want to improve; for example, I realize that a certain diff hunk should be extracted into a separate commit. To do that, I have to hit enter to go into the commit files view, then find the file again that I was just looking at, hit enter again to go into patch building mode, and then find the hunk again that I was just looking at. This is very cumbersome if there are many changed files in the commit, or many diff hunks in the file in question. It would be so much easier to just click in the diff view of the commit and go straight into patch building mode, with the clicked line selected.

(Side note: for editing a given line in the diff we recently improved this by using delta's hyperlink feature, so you can now jump straight to the editor by clicking on a line number. This is great, and I use it all the time; and this is a workaround for cases where you don't absolutely need a custom patch. For example, when I see a left-over printf statement from debugging that I want to drop from a commit, I now jump to the editor, delete it there, and amend the change into the commit I'm looking at. It should be as easy to make a custom patch and drop it, though.)

To implement all this, we can use delta's hyperlinks again, but it's a bit of a hacky solution. When clicking on a line in the diff (but not on a hyperlinked line number) we can examine the rest of the line to see if there are any lazygit-edit: URLs in it; if so, we use it to determine which file and line we're on. We can then select that file in the tree view on the left, and work out the line number in the patch that corresponds to the line number in the file.

I made a draft PR that implements this, and it works reasonably well. The main problem is that not all lines in the diff have a hyperlink; the ones between hunks don't, and deleted lines don't, either. As a user you need to learn to click on a line that has an underlined line number, which is not intuitive.

@dandavison I wonder if we can improve this by adding a mode to delta where it adds "invisible" hyperlinks to all lines in the diff, not just added or context lines. One way to do this could be to add a space at the beginning or end of the line and attach the hyperlink to that, and follow it with a \b character so that it's invisible. Very hacky, yes, but should improve things a bit.

Alternatively, we could use custom, private OSC messages to annotate all lines with file/lineNo information in some custom format. Probably a bit cleaner. Let me know if you'd be interested in exploring this more, or if you have other ideas how to solve this.

@jesseduffield Keen to hear your thoughts on all this.

stefanhaller commented 1 month ago

I find the idea of using a custom OSC protocol to annotate each line of the diff very attractive, the more I think about it. If it works out, we can encourage other pagers to support it, or other host applications (not sure there are any?) to use it. This would be somewhat similar to OSC 8 for hyperlinks.

In lazygit, it would allow us (provided we find a way to focus the main view; more on that in #3988) to select a line in the diff, and either hit enter to go directly to the staging view, or e to edit that line. We wouldn't need the current --hyperlinks support at all any more, although we can of course keep it for those who prefer to click line numbers (it would be orthogonal).

When using no pager (or a pager that only colors things) we can parse the diff ourselves to work out what file and line was clicked on, so we can offer the same features to all users.

@dandavison Would you be interested in collaborating on this?

dandavison commented 1 month ago

Hi @stefanhaller I appreciate the careful description of this problem and proposal. I'm being slow to understand it, but that is my fault.

The main problem is that not all lines in the diff have a hyperlink; the ones between hunks don't, and deleted lines don't, either.

I thought that delta added hyperlinks to the line numbers of all lines except removed lines. What is a line "between hunks"? This might help make sure we're on the same page: https://github.com/dandavison/delta/blob/main/ARCHITECTURE.md#handling-diff-hunk-lines

stefanhaller commented 1 month ago
Screenshot 2024-10-13 at 15 34 42

I'm talking about all the lines that are marked with red arrows. Maybe it's not equally important for all of them; if you select one of them and hit enter or e, it maybe reasonable to do nothing.

So yes, it's mostly about deleted lines; these should be annotated with the same number as the added line that follows (or the context line that follows, if there's no added line in the hunk).

Note that there's a minor problem with this approach: there's some ambiguity when selecting a line in a block of deleted lines. This is ok if you hit e to jump to the editor, you will arrive at the right line there; but when hitting enter to go to the staging view, we can't tell which of the deleted lines was selected, because they all share the same line number. I don't think there's much we can do about that though.