Add support for recursive blame #2285
Conversation
- Replaces `BlameFilePopup.blame: Option<BlameProcess>` with `BlameFilePopup.blame_stack: Vec<BlameProcess>`. - Adds keybinding for going back from a blame (`b`). - Makes keybinding for `blame` (`Shift+B`) work in the `popups/blame_file.rs` view, where it will take the commit and open a blame from that.
| @@ -93,7 +93,7 @@ pub struct BlameFilePopup { | |||
| table_state: std::cell::Cell<TableState>, | |||
| key_config: SharedKeyConfig, | |||
| current_height: std::cell::Cell<usize>, | |||
| blame: Option<BlameProcess>, | |||
| blame_stack: Vec<BlameProcess>, | |||
There was a problem hiding this comment.
I would like us to not keep this stack around. @cruessler and I agreed on working out an asynchronous blaming that will deliver results iteratively so the worst case scenarios are not as slow anymore and this stack will save a lot of data when going only in one direction through the blame history and only optimize for going back fast and not going back and forth.
Furthermore I have a gut feeling that it might lead to weird stacks when jumping to a file history from a blame and then to another blame again which will not properly reset the stack
There was a problem hiding this comment.
Ah, that makes sense. So, to clarify, do you want to a) have somewhat of a non-popping stack, b) just nix the back functionality altogether, or c) did you have something else in mind?
There was a problem hiding this comment.
Just a friendly ping :)
@extrawurst
There was a problem hiding this comment.
My guess is c), @extrawurst does not mind having a jump history in general. Keeping the whole BlameProcess around is the problem. If you just kept a stack of the CommitIds and selected line, or whatever is more reasonable in this context, like a BlameFileOpen maybe, this would be fine and sufficient to alleviate the concerns above.
I also would love to have this feature in gitui.
There was a problem hiding this comment.
IMO, also, b) should be avoided. Being able to jump back is a very important part of UX when jumping anywhere in code.
|
Thanks for tackling this. I only had one comment or request to rework. Would love to add this feature |
|
lets fix the ci, and i noticed that when the popup is open the command bar goes blank, that seems wrong |
Will get on that when I get a chance! Appreciate the update. |
Also @extrawurst I'm not sure if you saw my other comment, but I needed some more info. |
There was a problem hiding this comment.
@kanielrkirby: So, I've been asked to look at this, and I agree with you. The question above needs to be resolved first. See above for my attempt.
I'll gladly look into the details when this is entering the next round.
| ### Added | ||
| * add recursive git blame (do `SHIFT+B` in the revlog to go forward, and `b` to go back). | ||
|
|
||
| ## [0.26.3] - 2024-06-02 | ||
|
|
There was a problem hiding this comment.
Could you please move this to the unreleased section? Thanks. :)
| @@ -93,7 +93,7 @@ pub struct BlameFilePopup { | |||
| table_state: std::cell::Cell<TableState>, | |||
| key_config: SharedKeyConfig, | |||
| current_height: std::cell::Cell<usize>, | |||
| blame: Option<BlameProcess>, | |||
| blame_stack: Vec<BlameProcess>, | |||
There was a problem hiding this comment.
My guess is c), @extrawurst does not mind having a jump history in general. Keeping the whole BlameProcess around is the problem. If you just kept a stack of the CommitIds and selected line, or whatever is more reasonable in this context, like a BlameFileOpen maybe, this would be fine and sufficient to alleviate the concerns above.
I also would love to have this feature in gitui.
This Pull Request closes #2194.
It changes the following:
BlameFilePopup.blame: Option<BlameProcess>withBlameFilePopup.blame_stack: Vec<BlameProcess>. This involved changing any references to it, which happened to be a fair amount.b).blame(Shift+B) work in thepopups/blame_file.rsview, where it will take the commit and open a blame from that.blameandbackkeybindings in blame_file.popups/file_revlog.rsI followed the checklist:
make checkwithout errors (this reached the same pointmasterdoes, which fails ontest_pre_commit_py. Does this need an issue?)