Add Go to line feature for the blame view #2262
Conversation
|
@andrea-berling Thanks for the PR! I started testing on my machine, and I think this is looking good overall! One thing I noticed, though, is that, in a larger file with syntax highlighting enabled (I chose |
|
@cruessler Added the functionality you requested, it makes a lot of sense for large files. Seems to work for me, can you check it out again? |
There was a problem hiding this comment.
I left a few comments, mostly minor.
I found that there is an issue when you open a blame, then open the goto line popup before syntax highlighting finishes, then close the popup. The syntax highlighting that has completed in the background goes back to an in-progress state and doesn’t change anymore.
@extrawurst There’s a few added #[derive(Debug)] in this PR. Is that something we’d like to avoid? (I know that I’ve added similar derives in the past as well.)
@cruessler That is really weird, I was also worried that just cloning the blame as is could run into that issue, but I tested it multiple times and it seems to work fine on my machine. I could also just store the blame if it has completed |
|
@andrea-berling What happens when you open the goto line popup while syntax highlighting is still running, but close it only once it has finished? |
|
@cruessler progress goes back to 90% and remains there. I will look into it, perhaps I'm pushing the blame status too early |
Remove context and references to it Addres gitui-org#2262 (comment) Address gitui-org#2262 (comment)
|
Found a way to offer the same functionality without the need for a context data structure. Not a fan of the fact I had to make the |
|
@andrea-berling I do understand the concern. Is there any chance we can work around this issue by passing Apart from that, I tested the branch using |
|
@cruessler yes, I tried the first solution you proposed before pushing my latest version, what I found was that when re-using The second one you propose is more interesting: if instead of reusing |
|
@cruessler I made the change using your enum idea, it seems to work and I like it better 🙂 |
There was a problem hiding this comment.
@extrawurst Just tested on Mac and Linux (Ubuntu, KDE). Looks good to me!
There was a problem hiding this comment.
re context: providing Context, MaxLine so we can even provide that as validation in the GotoLine popup and make the validation more powerful instead of just 5 valid digits that parse to a usize.
Context allows us to use this also for the File View in the FileTree
|
Sorry it took me so long to push my latest changes, a bunch of life happened in between 😅 @extrawurst @cruessler what do you guys think? |
Add new stackable popup to get a line number to go to from the user Add a new internal event to change the selection in the most recent blame view as a result of using the go to line popup Add new keybinding for the go to line functionality (Shift-L) Add boolean to BlameFilePopup to check whether the view is currently shadowed by the go to line popup and, if so, let the key events bubble up to the go to line popup Add new command definition for the go to line functionality in BlameFilePopup Add clamping of the selected row in set_open_selection
Remove unnecessary goto_line_popup_is_open boolean
Add Context enum to keep the blame status across line jumps Add a context field to the GotoLinePopup Add blame field to BlameFilePopup Use the blame field in BlameFilePopup.open if not None
Remove context and references to it Addres gitui-org#2262 (comment) Address gitui-org#2262 (comment)
Modiy uses of BlameFileOpen accordingly Make the blame field of BlameFilePopup private again
Add a context struct for the go to line popup to keep the max line number allowed Add support for negative values for the go to line popup input (go to the -n-th to last line) Make the go to line input box red when invalid values are provided Add an error message to the Go to line popup when invalid values are used Allow arbitrarily large values in the Go to line input box
|
@andrea-berling could you update to recent master, would love to get this over the finish line |
|
@extrawurst done, I've merged your |
|
I just tested the changes, and I really like them! There’s one issue that might also not be an issue at all. I can open the popup before the file’s content have been loaded. If I do, the popup doesn’t accept any input because it still has state for a non-loaded file. I get the error message: |
@cruessler Thanks for catching this! The typo was the easiest to fix, but the issue you raised was also reasonably breezy: basically now the go to line popup won't open until it will be possible to determine the actual max line for the file (i.e. until the blame result is something). On most files, I'm not fast enough to try and open the blame popup before this is already the case, so there is no wait at all. On the larger ones (e.g.
@extrawurst hey 👋 so, when you say "It breaks the command bar", you mean that the popup makes it disappear, like in the following screenshot? As per the default text in the input, I'm not so sure: I can't really imagine a bulletproof guess for a good line number for any file (Start of the file? End of the file? Middle of the file? etc.), and I would imagine that most people would end up deleting the default text anyway and put the line number they want to go to (although I guess one could implement something like HTML's input placeholders so the user doesn't need to delete the default text). Did you perhaps already have an idea for what to put as a default, or did you want to brainstorm it/discuss it? |
|
Hey guys 👋 any news on this? 🙂 @extrawurst @cruessler |
|
@andrea-berling sorry for the lag on this. can you resolve conflicts and update it? |
|
@extrawurst Done 🙂 |
|
Also, at this point, I’m tempted to say, let’s get this merged and polish in follow-up PRs. :-) |
|
@andrea-berling this needs another round of conflict resolves :( |
|
@extrawurst Fixed 🙂 |
|
@andrea-berling one little clippy fix (plus a new conflict, the warnings will self heal after rebasing) |
|
@extrawurst Fixed again 😄 |
The "Go to Line" popup was previously managed as a stackable popup, which added unnecessary complexity. This commit changes it to be a non-stackable popup, simplifying the interaction and removing it from the main popup stack management. This makes the code easier to reason about and maintain. This also simplifies the BlameFilePopup API by removing the now-redundant BlameRequest enum.
This commit removes a number of unused `#[derive(Debug)]` implementations across the codebase to reduce code size and improve compile times.
The `GotoLineContext` struct was unnecessary, as it only contained a single `max_line` field. This commit refactors the `GotoLinePopup` to accept `max_line` directly, simplifying the code and removing the need for the `GotoLineContext` struct.
3 participants
It's still probably a bit rough around the edges, but before I made it too pretty, I though I would get some early feedback on it.
This Pull Request fixes/closes #2219.
It changes the following:
blame view as a result of using the go to line popup
I followed the checklist:
make checkwithout errors