Fix initial scroll bar size

[stefanhaller]

• PR Description

When loading a longer diff, make sure that the scrollbar of the diff view has the right thumb size immediately; i.e. it doesn't become smaller as you scroll down.

Fixes #2389

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.go generate)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• Docs (specifically docs/Config.md) have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[github-actions]

Uffizzi Preview deployment-18642 was deleted.

[jesseduffield]

On my machine linesToRead is 3655 and that causes noticeable lag in rendering content when going from one branch to another (using branches as an example because the content is effectively infinite).

So if we reduce this number to say 1000 the lag is less noticeable but we've reintroduced the problem that this PR tries to solve.

Between performance and accurate scrollbars, I find performance more important. What are your thoughts?

[jesseduffield]

I'm thinking we make this an opt-in feature and then see how many people want to turn it on.

[stefanhaller]

For me, the accurate scroll bar size is way more important for diffs than for branch logs, because as you say, branch logs are effectively infinite in practice anyway. And it seems that branch logs are slower to generate than diffs, so it might make sense to use different max linesToRead values for different commands. What do you think?

[jesseduffield]

I've done some testing locally and I agree: with diffs we should be pretty safe to keep the scrollbar accurate. By the looks of it it's only the branch panels (branches, tags, remote branches) where we would opt for performance.

[stefanhaller]

I pushed a commit to do that. It's maybe not the way we want to pass the information (providing the concrete maxLines values at the call site, I mean), but this way I could most easily see what all the call sites are. Maybe we should use a bool instead (what should we call it?) and then decide in linesToReadFromCmdTask what the limit should be based on that. Let me know what you prefer, I don't have much of a preference here.

[jesseduffield]

I agree with using a bool. Hard to think of a good name, but if we explain what we're doing in a comment it doesn't matter much. I'd call it readEnoughForScrollbar where we set that value to false for branches, remote branches, and tags. Thoughts?

[stefanhaller]

Fine with me; please have a look. I decided to go back to the previous behavior if readEnoughForScrollbar is false, instead of capping at different limits, which I would have found hard to explain.

Feel free to amend this if you want the comment in a different place or with different wording.

[jesseduffield]

I've been thinking about this some more and we might be able to simplify things and get the best of both worlds: the current problem is that when reading many lines, it takes a while before we render to the view which looks laggy. BUT there's no reason we can't read enough lines to fill the view, then render the view, then read the rest of the lines. The downsides I can think of are:

• scrollbar might have a slight flicker to it as it goes from large to small when the second render happens
• there may be some overhead in rendering the view twice
• reading a bunch of lines in a bunch of subprocesses may be expensive to the computer, even if it won't immediately manifest in any lazygit lagginess

But those don't sound too bad to me.

Here's a rough patch I'm currently testing with (if we go with this we'll need to pass in the view height + origin as the threshold instead of a hardcoded value of 100):

diff --git a/pkg/gui/view_helpers.go b/pkg/gui/view_helpers.go index 50a2e9b5d..192345924 100644 --- a/pkg/gui/view_helpers.go +++ b/pkg/gui/view_helpers.go @@ -25,7 +25,7 @@ func (gui *Gui) linesToReadFromCmdTask(v *gocui.View, readEnoughForScrollbar boo	// user scrolls down.	linesToRead := height + oy + 10 -	if readEnoughForScrollbar { +	if true {	// We want to read as many lines initially as necessary to let the	// scrollbar go to its minimum height, so that the scrollbar thumb doesn't	// change size as you scroll down. diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 448857fca..fb3292f24 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -163,6 +163,8 @@ func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), p	case <-stop:	break outer	case linesToRead := <-self.readLines: +	start := time.Now() +	for i := 0; i < linesToRead; i++ {	select {	case <-stop: @@ -188,8 +190,14 @@ func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), p	break outer	}	_, _ = self.writer.Write(append(scanner.Bytes(), '\n')) + +	if i == 100 { +	self.Log.Warnf("took %v to hit line 100", time.Since(start)) +	self.refreshView() +	}	}	self.refreshView() +	self.Log.Warnf("took %v to read lines", time.Since(start))	}	} 

With that patch we're saving over 100ms in some cases.

[mark2185]

scrollbar might have a slight flicker to it as it goes from large to small when the second render happens

"Between performance and accurate scrollbars, I find performance more important. What are your thoughts?" :)

there may be some overhead in rendering the view twice

Would it be more of an overhead than having just one render when everything is ready?

reading a bunch of lines in a bunch of subprocesses may be expensive to the computer, even if it won't immediately manifest in any lazygit lagginess

How many subprocesses are needed for that? How much text does it have to be before it passes the "way too much" limit? After all, it's just text.

[stefanhaller]

@jesseduffield Nice! I tested your patch, and it works well for me. The scrollbar jumping from large to small is a tiny little bit ugly, but it doesn't bother me too much.

How do we take it from here? Would you like me to finish it up, or were you planning to do this yourself? Either is fine with me.

[jesseduffield]

@stefanhaller if you can continue the work that would be great :)

[stefanhaller]

I wasn't entirely sure whether we still need this with the new approach. I left it in for now.

[jesseduffield]

I'm happy to keep it there

[stefanhaller]

We get one extra refresh here (and for the other tests below), I suppose it's from after handling the stop command. Maybe the whole setup can be done better, feel free to improve!

[jesseduffield]

I've pushed a commit to fix that up :)

[stefanhaller]

Thanks! :) Happy to keep it as a separate commit, it's nice to see how the tests change again with this change.

Ready to merge from my side.