-
-
Notifications
You must be signed in to change notification settings - Fork 278
Improved diff popup with fzf-lua & advanced range selections #1748
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
base: master
Are you sure you want to change the base?
Conversation
Is the existing fzf-lua integration not working? neogit/lua/neogit/lib/finder.lua Lines 336 to 345 in 2b27dda
Anyways, super cool you're taking the initiative here :) As a general note, the fzf specific stuff doesn't belong in the action, it's an implementation detail of the FuzzyFinderBuffer (and underlying Finder class). The action doesn't need to know/care which finder engine is being used. Diffing between branch ranges, tag ranges, and commit ranges already works in the 'diff range' action. the input can be any valid refspec. However, the UX could be improved, especially when a range is selected in the log/reflog buffer. We could do more to pre-populate selections. Diffing a specific commit against HEAD is a good idea - "diff this" just does a refspec to it's parent. Adding something like So to summarize:
|
The existing fzf-lua integration works well for general list fuzzy finding, but I've tried to enhance the diff menu by using fzf-lua's specific providers (like fzf_lua.git_branches(), fzf_lua.git_commits(), etc.) when available for actions like branch/commit/stash/tag ranges. This offers richer previews and native fzf-lua handling for those specific object types. If these specific fzf-lua methods aren't available or for other generic lists, it falls back to Neogit's FuzzyFinderBuffer (which then uses the existing generic fzf-lua integration if enabled). |
I edited my above comment before seeing you had replied already, so have a look.
Ah, ok, that's going to be a no-go then. I'm not interested in having fzf specific stuff at this layer, because that would mean snacks and telescope specific stuff would also belong here... which quickly gets out of hand. Honestly, I've considered removing all integrations, and just implementing my own fuzzy picker that is more tailored to whats needed here, but... not a high priority. |
First of all, thanks for this amazing project and your guidance! I understand the concern about scalability if every integration (telescope, snacks, etc.) required its own handling at that level. My primary goal with the changes was to improve the user experience for diffing ranges, especially when Let me address the specific points and explain the rationale behind my initial approach:
Path Forward: Perhaps This would keep the Even if we abstract the picker logic, I believe presenting distinct branch, commit and tag actions in the diff popup menu itself is more user-friendly than a single "diff range" that then prompts "What type of range?" This directness in the menu improves discoverability and reduces an interaction step for the user. The If enhancing Thanks again! |
Really great write up - lets dig into it.
As for why it wouldn't work with manual entry... I don't know. That should 😅 Regardless, it's quite a simple fix, so I'll merge that now. I know discoverability of this "everything is interactive" isn't great - in fact I think it's mostly hidden unless you happen to notice it working by accident. Open to ideas on improving that.
To the first point, maybe I'm weird, but I don't personally see the value-add of that information. I can understand that others would, but IMO having a single list that supports commits/branches/tags/refs/stashes is ideal. That said, I know not everyone will agree with me 😄 The second point, having separate actions per type, I think boils down to either providing a more "user friendly" and "rich" UI with specific actions for specific "types" of objects, or a UI that recognizes that all those "types" are all just revisions under the surface, and providing one interface that doesn't discriminate (but is therefore less "rich"). I think I see an issue, though, with more bespoke pickers like you describe. Items like (the above issue aside, I think your "item_type" solution is reasonable) There's probably some points I missed, but that's the meat of it :) |
My initial observation that the old 'diff range' didn't list commits makes much more sense now. If 'diff range' can be enhanced to seamlessly use commits selected from the log view, that would indeed cover a significant part of the commit diffing use case. Thanks for merging the fix related to manual entry! I appreciate your perspective on preferring a single, unified list for all ref types in the picker. You raised an excellent point: if we have, say, a "Diff Tag Range" action that only shows tags in its picker, how would a user diff a tag against For power-user scenarios like diffing Enhancing Even with an enhanced
This hybrid UI (discoverable common actions + a flexible generic one), backed by an enhanced I'm happy to work on refactoring towards the Let me know your thoughts on this direction. Thanks again for the great discussion! |
This PR brings a bunch of new commands to the diff popup that I've been missing personally.
b
), commit/ref ranges (c
), and tag ranges (t
).h
).f
) or paths/globs (p
) against HEAD.S
) is also updated to use the new picker.fzf-lua
(if available) for selecting branches, commits, tags, and stashes.diffview.open
can now accept an array of arguments, simplifying how diffs with multiple file arguments are invoked.--literal-pathspecs
from default git arguments to allow globs in paths.Testing:
I've tested these changes on Neovim v0.12.0-dev-461+gb28bbee539. The fzf-lua integration works as expected, and the fallback to another fuzzy finder (which I've tested with Telescope) works too.