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

Feat: Add VCS change picker for current document and workspace #5472

Closed

Conversation

poliorcetics
Copy link
Contributor

  • doc: fix Hunk::invert doc
  • chore: extract SourcePathFormat from lsp command file
  • feat: Add command and bindings to list changes from VCS in opened document(s)

Closes #5362

@pascalkuthe pascalkuthe added C-enhancement Category: Improvements A-vcs Area: Version control system interaction S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 10, 2023
@pascalkuthe pascalkuthe self-requested a review January 10, 2023 00:44
@pascalkuthe
Copy link
Member

I wanted something like this for a while but I held off implementing it because I think it's tricky to make this useful and create good UI for it.

Right now you only show the first line on the left side of the picker.
If I wanted to filter changes then I would want to filter the full change insteadof just the first line (especially for removals where the next line is shown).
Showing just the first line just seems distracting/disoriantiung to me.
That makes it sort of questionable what the point of the picker is as ]d would offer you more ore less the same thing.
Side node: It seems the highlighting/filter also doesn't work correctly although that might be an architectural problem with the picker:

image

To make this useful you could show context (similar to how git diff shows context) in the preview line as that at-least makes more sense in terms of filtering.
Furthermore I think the picker should show a preview similar to git diff that also shows removed lines (with a red background) as virtual text (should already be possible with #5420 and 0223877) and the new lines with green background (colors are theme dependent of-course).

I am still not 100% sold on the single-file version of this.
If you are interested in actually filtering the content of the hunks a better alternative could be a lines picker (where each line is a separate picker item) with an option to only show changed lines (colored appropriately).

I am personally much more interested in the multi-file picker as that seems more more useful to me because you can filter by file. However the comments above (don't show the first file line, instead show context and a better preview) also apply here.
Furthermore the current version that only applies to open buffers feel very limited.
imara-diff is definitely fast enough to compute a diff for the entire repository. It should be as fast (or faster if we use rayon) than a git diff altough it's a bit trickier to implement.

@pascalkuthe pascalkuthe removed their request for review January 10, 2023 18:47
@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Jan 10, 2023
@haikyuu
Copy link

haikyuu commented Jan 18, 2023

Another way to look at the change picker is in terms of files:
To quote the original issue (which is how I reason about it as well)

When I code I often need to go back to the places I modified

So the starting point would be the list of files that have been modified.

Question 1

user: What files has been modified since the last commit?
helix: here is the list (vcs explorer) of files that changed. I'll also add a little prefix to each file (modified, untracked, deleted ...). Note that files are treated like "folders" containing diffs. So when you click enter on a file, it will show its diffs as children, and their content will be displayed as a popover.

This is very much equivalent to a git status
image

Question 2

user: I'm currently in a file. what changed here since last commit?
helix: When you're in the file, you can navigate between next/prev changes using ] g/ [ g . And when the cursor is on a line that's included in a diff, you can g D to show the diff of that part inline, just like you can see documentation for a symbol.

I think it makes sense to have a single UX for both file explorer and vcs explorer.

@haikyuu
Copy link

haikyuu commented Jan 18, 2023

@pascalkuthe here is the PR for tree explorer #2377 I'm referencing and it's the same used in the screenshot above

@xJonathanLEI
Copy link
Contributor

xJonathanLEI commented Jan 22, 2023

Hmm I was about to work on something similar so I looked at your code. It looks like the workspace_vcs_change_picker command doesn't really load up the changes in the workspace? It only scans the opened buffers.

To me, such a picker is most useful for picking up half-done work. So I could do hx ., and then fire up picker, and go to the modified file. With the currently implementation I assume this won't work?

IMHO the picker as currently designed isn't very useful - I can already navigate through the diffs very efficiently by doing ]g]g]g]g after a gg. I would rather get a added/modified files picker.

And a changes picker is too noisy to be used as a changed files picker.

@poliorcetics
Copy link
Contributor Author

IMHO the picker as currently designed isn't very useful - I can already navigate through the diffs very efficiently by doing ]g]g]g]g after a gg.

Navigation by hunk in the current file is not useful to me. Like at all. I don't want to move, I just want to see what changed at quick glance, without the editor window moving at all. A picker solves that. Also, typing ]g or whatever binding instead of just up and down is not very good UX IMO, I rarely do it for diagnostics for example, despite having remapped diag-navigation to be very convenient on my keyboard. Also, if I'm not on a change because I was reading something, it makes it harder to come back without using Ctrl+O a dozen times or more.

If I wanted to filter changes then I would want to filter the full change insteadof just the first line (especially for removals where the next line is shown).

I could show all the modified lines but I don't think that would be useful either, because if I write a 3 50-lines functions and add 7 imports, its going to have lots of noise for pretty much no real use

We could do a thing where we split hunks bigger than N lines into different parts but I think that would be confusing if it was the default, though it could be very useful. Maybe for a future evolution, once we have user feedback ?

Overall, I think having a base version with only the first line (or the line before for deletions) is good enough for a first version and to see what people will want. We could pin an issue asking for feedback ?

I would rather get a added/modified files picker.

Agreed, all three pickers you mentionned in the issue seem nice, I'll do them all in separate commits

It only scans the opened buffers

Yeah, it was simpler that way but I'll fix it, you're right it would be much better with all files instead of just the opened ones.

@poliorcetics
Copy link
Contributor Author

I forgot you opened #5645 for the file change picker, I'll leave that one to you, let's not duplicate efforts, I'll just fix the workspace-hunks one :)

@poliorcetics
Copy link
Contributor Author

I think I'll wait for #5645 to be merged, that way I'll be able to use the changes (notably changed files discovery) done there

@poliorcetics
Copy link
Contributor Author

Alternatively, I could simply remove the workspace-changes picker for now and keep the current-buffer one, allowing both MR to progress in parallel, I'll do that instead :)

@woojiq woojiq mentioned this pull request Aug 18, 2023
@poliorcetics poliorcetics deleted the 5362-vcs-change-picker branch February 12, 2024 22:12
@hnorkowski
Copy link
Contributor

@poliorcetics Did you give up?

@poliorcetics
Copy link
Contributor Author

Yes, I've been using gitui outside of helix and haven't been maintaining this branch at all so better close it than promise something that won't come

@poliorcetics
Copy link
Contributor Author

Last year, when I worked on this, gitoxide wasn't advanced enough to correctly implement all I wanted, maybe that's changed now and it's possible ? I haven't checked where gix status is at nowadays, but if it works, it should be possible to implement everything this MR intended to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-vcs Area: Version control system interaction C-enhancement Category: Improvements S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VCS changes picker
5 participants