Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
stefanhaller opened this issue Oct 11, 2024 · 3 comments · May be fixed by #3985
Open

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

stefanhaller opened this issue Oct 11, 2024 · 3 comments · May be fixed by #3985
Labels
enhancement New feature or request

Comments

@stefanhaller
Copy link
Collaborator

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:

  • if you are using a custom pager that does more than just coloring the diff (e.g. delta), you usually end up with the wrong line selected; it's not very far away, but still annoying. This is because the line numbers in the pager's diff view don't exactly line up with the line numbers in the staging view.
  • it's only available when a file is selected. When a directory is selected, clicking in the diff collapses or expands the directory, which I suppose was not intentional.
  • it's not available when looking at a diff of a commit.

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
Copy link
Collaborator Author

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
Copy link

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants