Skip to content

Focus the main view #4429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Apr 21, 2025
Merged

Focus the main view #4429

merged 16 commits into from
Apr 21, 2025

Conversation

stefanhaller
Copy link
Collaborator

  • PR Description

This adds a new 0 keybinding to the side panels that focuses whatever main view is currently displayed, with the goal of making it easier to scroll the main view (using the normal navigation keys , . < >), and being able to search the main view using /.

Alternatively to pressing 0 you can also click the main view to focus it. Note that previously it was possible to go directly to the staging panel by clicking in the main view when a file was selected; this now takes a double click, because the first click just focuses the main view, but you can go to staging from there by clicking again.

I'm reasonably happy with the overall behavior, but it takes some getting used to, so we'll want to test this for a while to see if it doesn't make the focus handling too confusing.

Also, it works best when using a pager, because then you get a very clear visual distinction between the focused main view and the staging view. Without a pager they look identical, so it can be confusing when you select a file, click in the main view, see something that looks like the staging panel, and then nothing happens when you press space to stage hunks. Maybe we should consider jumping directly to staging when no pager is used, to make this less confusing. (But note that this situation will change again if/when we manage to get #4332 working.)

Fixes #3988.

  • 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 stefanhaller added the enhancement New feature or request label Mar 26, 2025
Copy link

codacy-production bot commented Mar 26, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for 5f809801 62.73%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (5f80980) Report Missing Report Missing Report Missing
Head commit (b51fdb7) 54694 47337 86.55%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#4429) 381 239 62.73%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

@stefanhaller
Copy link
Collaborator Author

Input from @jesseduffield:

I'm not clear on the value of having a cursor in the view. I would have expected that up/down would just scroll, similar to when you're in a less pager.

Having a cursor is mainly motivated by #3986; that issue talks mostly about clicking in the view, but I would find it useful to have the same functionality when hitting enter in the focused main view. For example, select a commit, focus it, move through it with . or arrow-down, and hit enter to go into patch building. Or e to edit the file at the selected line. I have a rough draft PR for this at #3985, but it only works for clicking. I should rebase that branch onto this one so that you get a sense of what it feels like to hit enter in the focused main view, but that's a bit of work and I'm not sure I'll have time for this today.

Of course, as long as we don't have #3986 (and it's not clear we ever will, since it depends on support from the pager), you are right that a cursor makes little sense. And in views other than diffs (e.g. branch log) it never does.

Options:

  1. Keep the cursor in anticipation of 3986, and live with the strangeness for now. (Maybe forever if we never get around to it.)
  2. Remove the cursor highlight for now and only add it when we do 3986. The behavior change might be confusing then.
  3. Don't merge this PR now, only do it if we have solved 3986 and do both together.

Even with 3986, it will only work when using a pager that supports whatever custom protocol we come up with, so maybe we still need to make the cursor an opt-in config.

@stefanhaller
Copy link
Collaborator Author

I rebased #3985 onto this PR now, so it now works when pressing enter in the focused view. (Pressing e is not supported yet.)

@stefanhaller
Copy link
Collaborator Author

@jesseduffield As discussed privately, in this PR we no longer show a cursor highlight in the focused main view. We do this in the followup PR (#3985), but only with an opt-in user config.

This is now almost ready, except that I need to find a way to block until all view content has been read from the task when pressing > (or end); also, there are strange bugs with searching that I need to figure out. Keeping as draft for now.

@stefanhaller
Copy link
Collaborator Author

I consider this ready now, opening for review.

@stefanhaller stefanhaller marked this pull request as ready for review April 3, 2025 16:19
@stefanhaller stefanhaller force-pushed the focus-main-view branch 2 times, most recently from c5fc123 to b51fdb7 Compare April 16, 2025 14:30
@jesseduffield
Copy link
Owner

Code looks good, and testing this I really like the behaviour.

I did encounter a crash when testing it out: Upon pressing '0' on a file with merge conflicts I get the following:

panic: interface conversion: *context.MergeConflictsContext is not types.ISearchableContext: missing method ClearSearchString

goroutine 1 [running]:
github.com/jesseduffield/lazygit/pkg/gui/controllers.(*SwitchToFocusedMainViewController).focusMainView(0x140000a98c0, {0x100c881ba, 0x4})
	/Users/jesse/repos/lazygit/pkg/gui/controllers/switch_to_focused_main_view_controller.go:77 +0xc0
github.com/jesseduffield/lazygit/pkg/gui/controllers.(*SwitchToFocusedMainViewController).handleFocusMainView(...)
	/Users/jesse/repos/lazygit/pkg/gui/controllers/switch_to_focused_main_view_controller.go:71
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).callKeybindingHandler(0x140002ea308, 0x140000021c0?)
	/Users/jesse/repos/lazygit/pkg/gui/keybindings.go:540 +0x11c
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).SetKeybinding.func1()
	/Users/jesse/repos/lazygit/pkg/gui/keybindings.go:485 +0x24
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).SetKeybinding.(*Gui).wrappedHandler.func3(0x6?, 0x101c10138?)
	/Users/jesse/repos/lazygit/pkg/gui/keybindings.go:479 +0x24
github.com/jesseduffield/gocui.(*Gui).execKeybinding(0x101c10108?, 0x6?, 0x140002594a8?)
	/Users/jesse/repos/lazygit/vendor/github.com/jesseduffield/gocui/gui.go:1553 +0x6c
github.com/jesseduffield/gocui.(*Gui).execKeybindings(0x14000324000, 0x14000116488, 0x14000259560)
	/Users/jesse/repos/lazygit/vendor/github.com/jesseduffield/gocui/gui.go:1521 +0x3bc
github.com/jesseduffield/gocui.(*Gui).onKey(0x140000ae4a8?, 0x140002594e0?)
	/Users/jesse/repos/lazygit/vendor/github.com/jesseduffield/gocui/gui.go:1328 +0x58
github.com/jesseduffield/gocui.(*Gui).handleEvent(0x140000ae480?, 0x14000259548?)
	/Users/jesse/repos/lazygit/vendor/github.com/jesseduffield/gocui/gui.go:845 +0x40
github.com/jesseduffield/gocui.(*Gui).processEvent(0x14000324000)
	/Users/jesse/repos/lazygit/vendor/github.com/jesseduffield/gocui/gui.go:799 +0x180
github.com/jesseduffield/gocui.(*Gui).MainLoop(0x14000324000)
	/Users/jesse/repos/lazygit/vendor/github.com/jesseduffield/gocui/gui.go:778 +0x108
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).Run(0x140002ea308, {{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}})
	/Users/jesse/repos/lazygit/pkg/gui/gui.go:866 +0x434
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).RunAndHandleError.func1()
	/Users/jesse/repos/lazygit/pkg/gui/gui.go:872 +0x48
github.com/jesseduffield/lazygit/pkg/utils.SafeWithError(0x14000045928?)
	/Users/jesse/repos/lazygit/pkg/utils/utils.go:90 +0x5c
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).RunAndHandleError(0x140002ea308, {{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}})
	/Users/jesse/repos/lazygit/pkg/gui/gui.go:871 +0xc4
github.com/jesseduffield/lazygit/pkg/app.(*App).Run(...)
	/Users/jesse/repos/lazygit/pkg/app/app.go:270
github.com/jesseduffield/lazygit/pkg/app.Run({0x10107bc18?, 0x140000e08f0?}, 0x140002d0a80, {{0x0, 0x0}, {0x0, 0x0}, {0x0, 0x0}, {0x0, ...}})
	/Users/jesse/repos/lazygit/pkg/app/app.go:48 +0xb0
github.com/jesseduffield/lazygit/pkg/app.Start(0x14000045ef8, {0x0, 0x0})
	/Users/jesse/repos/lazygit/pkg/app/entry_point.go:168 +0x9b0

@stefanhaller
Copy link
Collaborator Author

I did encounter a crash when testing it out: Upon pressing '0' on a file with merge conflicts I get the following:

What a stupid bug; fixed in 0c20f49.

Pressing '0' on a file with merge conflicts now does the same as pressing enter, which I think is reasonable.

Copy link

codacy-production bot commented Apr 20, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for 548d4861 62.73%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (548d486) Report Missing Report Missing Report Missing
Head commit (4e21a09) 55373 47992 86.67%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#4429) 381 239 62.73%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

So that we don't have to pass the map to controllers.
Previously we would render the diff for a directory to the main/secondary pair,
but a diff for a file to the staging/stagingSecondary pair. (And similar for
commit files: main/secondary for directories, but
patchBuilding/patchBuildingSecondary for files.)

I always found this confusing and couldn't really understand why we are doing
this; but now it gets in my way because I want to attach a controller to
main/secondary so that they can be focused. So change it to always use the main
context pair for everything we render from a side panel.
In this state of the code it isn't worth much because it's not any more than a
SimpleContext, but we'll add things to it later in the branch.
In this commit this is only possible by pressing '0' in a side panel; we'll add
mouse clicking later in the branch.

Also, you can't really do anything in the focused view except press escape to
leave it again. We'll add some more functionality in a following commit.
And only while the task is running.

This avoids accumulating lots of blocked goroutines when scrolling a view down
more than 1024 times (the capacity of the readLines channel).
@stefanhaller stefanhaller enabled auto-merge April 21, 2025 16:04
@stefanhaller stefanhaller merged commit e4362ee into master Apr 21, 2025
14 checks passed
@stefanhaller stefanhaller deleted the focus-main-view branch April 21, 2025 16:05
stefanhaller added a commit that referenced this pull request Apr 23, 2025
…is focused (#4505)

- **PR Description**

Allow pressing `{`, `}`, `(`, and `)` to change the diff context size
and rename threshold when the main view is focused. This was forgotten
in #4429.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow focusing the main view
2 participants