Clamp maxY in patch explorer and merge conflicts to allow large hunks

[ChrisMcD1]

• PR Description

Did a git bisect and found that #4470 was introduced by f3eb180.

Our problem comes down to the implementation of SetCursorY in gocui, which was throwing out the y value we provided it, since it was higher than the inner height of the view.

lazygit/vendor/github.com/jesseduffield/gocui/view.go

Lines 583 to 589 in a0ec22c

	func (v *View) SetCursorY(y int) {
	maxY := v.InnerHeight()
	if y < 0 || y >= maxY {
	return
	}
	v.cy = y
	}

We only had 2 callsites to SetCursorY, both of which I tested manually and updated in this PR. We could put this clamping logic into gocui, but the name Set doesn't imply that it does any clamping. I'm open to renaming the function and making the PR there, if we would want.

Fixes #4470

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run 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)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[stefanhaller]

This is only a partial fix. It does fix the display right after pressing a, but if you then scroll down (using the scroll wheel) to see where the hunk ends, the selection is still not drawn correctly.

I guess I'm fine with merging it in this state because it improves the situation a tiny little bit at least; but to fix this properly we need to make more fundamental changes to gocui. At the moment gocui has a lot of code that tries to ensure that cx/cy are always inside the visible portion of the view, and this is the root of the problem. Not all of the code does, though; for example, if you scroll the selected line of list view out of the visible portion of the view using the scroll wheel, then cy happily becomes negative or larger than the height (and has to). So the real fix will be to audit all the code to make sure that "out-of-bounds" cursor values are accepted, and handled correctly. I'd expect this to be a pretty large amount of work, but I haven't looked closely into it, and maybe I'm wrong and it's not so bad.

Another change that would be good to make is to change the meaning of cx/cy so that they are relative to the (wrapped) view content, rather than relative to the visible area of the view. I suppose this would simplify a lot of things; for example, we wouldn't have to adapt cy when scrolling the view up or down. But I'm not sure whether it's a good idea to make this change at the same time as the other one.

[ChrisMcD1]

but if you then scroll down (using the scroll wheel) to see where the hunk ends, the selection is still not drawn correctly.

Ah good point. I'm 100% on my keyboard, so I didn't know you could even do that! Interestingly, it seems that there is no way to scroll on the keyboard. j and k and <up> and <down> don't do anything when selecting a hunk. Is that expected?

I'm also not sure if I fixed the integration tests in an appropriate way... The SelectedLines matcher is quite particular that you provide it a matcher for every line, but on my machine, we only select through the +12a line, but in the integration test runner, we have a lot more vertical space and select through the +13a line. Just by poking around I found this IsSelected matcher. Is that what I should use?

And if so, should I make it a bit narrower of a selection range (Maybe to +10a) to support other contributors who might be developing in a setup with less vertical space to work with?

[stefanhaller]

Interestingly, it seems that there is no way to scroll on the keyboard. j and k and <up> and <down> don't do anything when selecting a hunk. Is that expected?

<up>, <down>, j and k all select the previous/next hunk when you are in hunk selection mode. Sounds like you had only a single hunk in your testing.

I don't think there's any other way to scroll the view without moving the selection except by using the scroll wheel.

I'm also not sure if I fixed the integration tests in an appropriate way...

It's not worth spending time and energy on trying to get this test to work. The behavior is dependent on the screen size now, and that's different for everybody. You can't possibly write an integration test that works everywhere. (We can write one that succeeds on CI, because we have a fixed window size there, but it will then fail for people running it locally, which is no good.)

One more reason to rather do the proper fix instead. I'm planning to look into this sometime soon, and if I don't come up with a fix before the next release, I'm happy to merge this PR (without the test) because it already improves it for non-mouse users.

[ChrisMcD1]

It's not worth spending time and energy on trying to get this test to work.

Okay, I pushed a version without the test. I agree that's its fragile, I only even attempted to include it because there is a very similar test (with similar limitations I imagine) already present here

lazygit/pkg/integration/tests/staging/stage_hunks.go

Line 8 in a0ec22c

var StageHunks = NewIntegrationTest(NewIntegrationTestArgs{

One more reason to rather do the proper fix instead.

Totally agree. I don't think I have it in me to dive into the gocui internals right now, so I'll leave that all to you

[stefanhaller]

#4589 makes this obsolete.

[ChrisMcD1]

Seems good 🫡