Add Go to line feature for the blame view #2262

andrea-berling · 2024-06-09T10:11:52Z

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:

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 new command definition for the go to line functionality in BlameFilePopup
Add clamping of the selected row in set_open_selection

I followed the checklist:

I added unittests
I ran make check without errors
I tested the overall application
I added an appropriate item to the changelog'

cruessler · 2024-06-27T12:54:50Z

@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 CHANGELOG.md), the blame view gets reloaded when I navigate to a line using the new popup. Is is possible to keep the blame view’s state when jumping to a line?

extrawurst · 2024-06-27T19:15:22Z

src/queue.rs

@@ -68,6 +68,8 @@ pub enum StackablePopupOpen {
 	InspectCommit(InspectCommitOpen),
 	///
 	CompareCommits(InspectCommitOpen),
+	///
+	GotoLine,


i think its a bit of a missed opportunity to not pass it some context: enum Context {Blame, FileView} and PossibleRange(min,max) to be able to reuse this in various places

In my last commit I added the Context enum and used it to preserve the BlameProcess status across line jumps, and I added the two variants you mentioned in line comments. That's because cargo wouldn't let me compile if I didn't use those variants, which I didn't. Do you have a specific example in mind where those pieces of information could be used? If so, I could use them to implement those use cases as to make them more interesting, otherwise I can leave the comments around as placeholder for future functionality 🙂

andrea-berling · 2024-06-30T11:34:14Z

@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?

cruessler

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.)

src/popups/blame_file.rs

src/popups/goto_line.rs

cruessler · 2024-07-01T07:56:36Z

src/queue.rs

@@ -56,6 +56,13 @@ pub enum Action {
 	UndoCommit,
 }

+#[derive(Debug, Clone)]
+pub enum Context {


What do you think about making this a bit more specific by renaming to GotoLineContext (or whatever fits best)?

I held off giving it a more specific name until I understood more clearly what @extrawurst meant in #2262 (comment). But yeah, ideally, once I have a better idea of where else this Context enum would be used, I will give it a specific name

andrea-berling · 2024-07-01T09:51:16Z

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.

@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

cruessler · 2024-07-01T10:49:55Z

@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?

andrea-berling · 2024-07-01T11:32:32Z

@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 extrawurst#2262 (comment) Address extrawurst#2262 (comment)

andrea-berling · 2024-07-06T16:31:18Z

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 blame field of BlameFilePopup pub (it sticks out like a sore thumb), but it works fine and doesn't run into the issues reported by @cruessler. What do you think?

cruessler · 2024-07-07T09:44:19Z

@andrea-berling I do understand the concern. Is there any chance we can work around this issue by passing blame: None to BlameFileOpen and reusing the existing self.blame in this piece of code? https://github.com/andrea-berling/gitui/blob/76577fc88c3bd0ebe4f3a87742b90cbdc7390245/src/popups/blame_file.rs#L397-L405 Or by switching self.blame: Option for self.blame: BlameRequest with enum BlameRequest { StartNew, KeepExisting, UseNew(BlameProcess) }?

Apart from that, I tested the branch using cargo run and I found it to work well!

andrea-berling · 2024-07-08T09:27:13Z

@cruessler yes, I tried the first solution you proposed before pushing my latest version, what I found was that when re-using self.blame if not None then it would work, but since there is a single blame_file_popup field in the app, the self.blame object used for the first file blamed would be used in any other blame going forward. For example, if I blamed CONTRIBUTING.md, it would keep showing me the blame for CONTRIBUTING.md, even if I blamed CHANGELOG.md afterwards.

The second one you propose is more interesting: if instead of reusing self.blame when it is not None we re-use it when explicitly requested (i.e. KeepExisting) when closing the Go To Line popup, and we request a new one when opening a new blame, it might just work 🙂 I will try that one and let you know

andrea-berling · 2024-07-16T21:24:41Z

@cruessler I made the change using your enum idea, it seems to work and I like it better 🙂

cruessler

@extrawurst Just tested on Mac and Linux (Ubuntu, KDE). Looks good to me!

extrawurst · 2024-07-21T05:43:00Z

src/popups/goto_line.rs

+					if c.is_ascii_digit() {
+						// I'd assume it's unusual for people to blame
+						// files with milions of lines
+						if self.line.len() < 6 {


for me this is very cryptic? why hardcode to 6 digits? why is it called line wouldn't digits be more fitting and descriptive? instead of this we can just try to parse the result and only allow enter if the current input is valid and even show it red in case it is not.

The popup is called goto_line, thus the first name I came up with for the input was: line 😄 but I'm not married to it, digits works just as well for me, I trust your judgement for fitting-ness and descriptiveness 🙂 and yeah, I went for a very basic implementation at first based on the simplifying assumption that most people don't work with text files with hundreds of thousands of lines, but I like your idea of validating the input based on the max range and highlighting it red if not valid. I was even thinking of allowing to put a minus sign in front of the whole this to allow navigation to the last line, the second to last line, and in general, the n-th to last line. I will work something up in the next few days 🙂

extrawurst · 2024-07-21T05:46:47Z

src/popups/blame_file.rs

+					self.hide_stacked(true);
+					self.visible = true;
+					self.queue.push(InternalEvent::OpenPopup(
+						StackablePopupOpen::GotoLine,


here we can provide the context: Blame, MaxLine

extrawurst

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

andrea-berling · 2024-09-07T15:01:59Z

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 extrawurst#2262 (comment) Address extrawurst#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

extrawurst · 2024-09-17T12:41:31Z

@andrea-berling could you update to recent master, would love to get this over the finish line

andrea-berling · 2024-09-18T16:03:42Z

@extrawurst done, I've merged your master, made some last-minute adjustments, and added a Changelog entry 🙂 let me know if there is anything else you need from me

andrea-berling · 2024-09-18T16:16:31Z

@extrawurst Fixed the conflicts from today's commits 😅 now we should be good 🙂

cruessler · 2024-09-19T06:43:55Z

@extrawurst @andrea-berling I want to do some testing on my machines later in the day, provided it has not already been merged by then. :-)

extrawurst · 2024-09-19T08:33:05Z

Can’t merge for now. It breaks the command bar when the popup is open and I think we should have a default text in the input

cruessler · 2024-09-26T06:22:39Z

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: Invalid input: only numbers between -1 and 0 (included) are allowed (-1 means denotes the last line, -2 denotes the second to last line, and so on). (Also, one of means denotes probably can be removed.)

andrea-berling · 2024-10-01T18:58:36Z

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: Invalid input: only numbers between -1 and 0 (included) are allowed (-1 means denotes the last line, -2 denotes the second to last line, and so on). (Also, one of means denotes probably can be removed.)

@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. Contributing.md, Cargo.lock), you might need to wait a second or two before you can open the popup. Same amount of time you need to wait to even see the contents from the file in the UI, so I would say the UX should be reasonable and not too confusing/frustrating.

Can’t merge for now. It breaks the command bar when the popup is open and I think we should have a default text in the input

@extrawurst hey 👋 so, when you say "It breaks the command bar", you mean that the popup makes it disappear, like in the following screenshot?

If so, I agree, it's not a great UX, although I didn't catch it at first. Curse of knowledge I guess, I suppose that because I knew you need to press Enter to go to a line and Esc to close the popup I didn't notice the lack of a hint.
What do you think of such a command bar for the popup?

I haven't pushed the change yet, it's still on my local machine. Let me know if you like it, so I can push that as well 🙂

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?

andrea-berling · 2024-11-05T18:43:22Z

Hey guys 👋 any news on this? 🙂 @extrawurst @cruessler

extrawurst reviewed Jun 27, 2024

View reviewed changes

cruessler suggested changes Jul 1, 2024

View reviewed changes

andrea-berling added a commit to andrea-berling/gitui that referenced this pull request Jul 6, 2024

Load blame from the last blame file instead of context

76577fc

Remove context and references to it Addres extrawurst#2262 (comment) Address extrawurst#2262 (comment)

cruessler approved these changes Jul 19, 2024

View reviewed changes

extrawurst reviewed Jul 21, 2024

View reviewed changes

extrawurst requested changes Jul 21, 2024

View reviewed changes

andrea-berling added 7 commits September 17, 2024 12:19

Fix make check errors

9c526de

Move goto_line_popup up the components list for proper event handling

55da5d9

Remove unnecessary goto_line_popup_is_open boolean

Preserve BlameProcess in BlamePopup when jumping lines

6bdab9d

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

Load blame from the last blame file instead of context

20ed1d2

Remove context and references to it Addres extrawurst#2262 (comment) Address extrawurst#2262 (comment)

Add a BlameRequest enum for re-using blame data in Goto line

e1291f0

Modiy uses of BlameFileOpen accordingly Make the blame field of BlameFilePopup private again

extrawurst force-pushed the master branch from 8757559 to 4855ca6 Compare September 17, 2024 10:20

andrea-berling added 2 commits September 17, 2024 20:28

Merge remote-tracking branch 'upstream/master'

be182ae

Fix up negative indexing and add changelog entry

56732c2

Merge remote-tracking branch 'upstream/master'

3c8afa1

andrea-berling added 2 commits October 1, 2024 19:55

Merge remote-tracking branch 'upstream/master'

dc65a8d

Prevent GotoLine popup from opening before blame load and fix typo

bea05c2

Merge remote-tracking branch 'upstream/master'

df83ef0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Go to line feature for the blame view #2262

Add Go to line feature for the blame view #2262

andrea-berling commented Jun 9, 2024 •

edited

Loading

cruessler commented Jun 27, 2024

extrawurst Jun 27, 2024

andrea-berling Jun 30, 2024

andrea-berling commented Jun 30, 2024

cruessler left a comment

cruessler Jul 1, 2024

andrea-berling Jul 4, 2024

andrea-berling commented Jul 1, 2024

cruessler commented Jul 1, 2024

andrea-berling commented Jul 1, 2024

andrea-berling commented Jul 6, 2024

cruessler commented Jul 7, 2024 •

edited

Loading

andrea-berling commented Jul 8, 2024

andrea-berling commented Jul 16, 2024

cruessler left a comment

extrawurst Jul 21, 2024

andrea-berling Aug 1, 2024

extrawurst Jul 21, 2024

extrawurst left a comment

andrea-berling commented Sep 7, 2024

extrawurst commented Sep 17, 2024

andrea-berling commented Sep 18, 2024

andrea-berling commented Sep 18, 2024

cruessler commented Sep 19, 2024

extrawurst commented Sep 19, 2024

cruessler commented Sep 26, 2024

andrea-berling commented Oct 1, 2024

andrea-berling commented Nov 5, 2024

Add Go to line feature for the blame view #2262

Are you sure you want to change the base?

Add Go to line feature for the blame view #2262

Conversation

andrea-berling commented Jun 9, 2024 • edited Loading

cruessler commented Jun 27, 2024

extrawurst Jun 27, 2024

Choose a reason for hiding this comment

andrea-berling Jun 30, 2024

Choose a reason for hiding this comment

andrea-berling commented Jun 30, 2024

cruessler left a comment

Choose a reason for hiding this comment

cruessler Jul 1, 2024

Choose a reason for hiding this comment

andrea-berling Jul 4, 2024

Choose a reason for hiding this comment

andrea-berling commented Jul 1, 2024

cruessler commented Jul 1, 2024

andrea-berling commented Jul 1, 2024

andrea-berling commented Jul 6, 2024

cruessler commented Jul 7, 2024 • edited Loading

andrea-berling commented Jul 8, 2024

andrea-berling commented Jul 16, 2024

cruessler left a comment

Choose a reason for hiding this comment

extrawurst Jul 21, 2024

Choose a reason for hiding this comment

andrea-berling Aug 1, 2024

Choose a reason for hiding this comment

extrawurst Jul 21, 2024

Choose a reason for hiding this comment

extrawurst left a comment

Choose a reason for hiding this comment

andrea-berling commented Sep 7, 2024

extrawurst commented Sep 17, 2024

andrea-berling commented Sep 18, 2024

andrea-berling commented Sep 18, 2024

cruessler commented Sep 19, 2024

extrawurst commented Sep 19, 2024

cruessler commented Sep 26, 2024

andrea-berling commented Oct 1, 2024

andrea-berling commented Nov 5, 2024

andrea-berling commented Jun 9, 2024 •

edited

Loading

cruessler commented Jul 7, 2024 •

edited

Loading