Fix crash when clicking in the status view

[stefanhaller]

• PR Description

The status view is not supposed to be focusable right now. (This might change soon, but for now it isn't.) Pressing '0' on it does nothing.

However, clicking on it would still focus it, because the click handler in MainViewController assumed that when the clicked view doesn't have the focus, then its "other" view must have, and we just want to toggle the focus between the two (like when pressing tab). It didn't take the possibility into account that the current side panel isn't focusable at all; if it was, then its SwitchToFocusedMainViewController would have handled the click.

To fix this, check if the "other" view has the focus before handling the click, and do nothing otherwise.

This also fixes clicking in the main views of the Worktrees or Submodules tabs, or any other tabs whose main views are not focusable.

Fixes #4566.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for eecb59d1	✅ 56.52%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (eecb59d)	Report Missing	Report Missing	Report Missing
Head commit (bbd17ab)	56451	49028	86.85%

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 (#4567)	46	26	56.52%

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

[ChrisMcD1]

For my own understanding:
It seems like at the construction of this MainViewController, the otehrContext is defined as NormalSecondary for the Normal one, and vice-versa.

Which makes this seem different from the normal focusMainView method that is triggered when you hit 0.

lazygit/pkg/gui/controllers/switch_to_focused_main_view_controller.go

Lines 74 to 82 in a62db52

	func (self *SwitchToFocusedMainViewController) focusMainView(mainViewName string) error {
	mainViewContext := self.c.Helpers().Window.GetContextForWindow(mainViewName)
	mainViewContext.SetParentContext(self.context)
	if context, ok := mainViewContext.(types.ISearchableContext); ok {
	context.ClearSearchString()
	}
	self.c.Context().Push(mainViewContext, types.OnFocusOpts{})
	return nil
	}

.

However, clicking on it would still focus it, because the click handler in MainViewController assumed that when the clicked view doesn't have the focus, then its "other" view must have, and we just want to toggle the focus between the two (like when pressing tab).

Based on this, I expected there to be multiple of these controllers, with the otherContext to be one of Files, Branches, and so on. I think this a fundamental lack of understanding of what the NormalSecondary context is on my part.

[stefanhaller]

Thanks for asking; thinking about a reply led me to a much better fix. 😄

So, we have two MainViewControllers, one for each view of the pair. As you said, the otherContext is set to the respective other view of the pair, and it is used for toggling to the other view when pressing tab.

Your confusion seems to be between otherContext and parentContext. The otherContext is static and never changes after construction; the parentContext points back to the side panel, and is set dynamically in SwitchToFocusedMainViewController.focusMainView.

Do let me know if you have more questions; the whole architecture around contexts and controllers is a bit tricky to understand, and it took me quite a while to wrap my head around it.

But to explain the bug a bit more: we have two kinds of mouse click bindings, focus-specific ones and global ones. The one in SwitchToFocusedMainViewController is focus-specific, it will only be called when the click is in the target view and the side panel has the focus (dispatched from here). The one in MainViewController, however (which is meant to be only used for clicking in the main view when either that view itself or the other view of the pair has the focus) was a global one (dispatched from here when no focus-specific one was found), so it was also called when one of the side panels that don't have a SwitchToFocusedMainViewController was focused.

So instead of putting more if statements into the body of that global click handler, the better fix is to make it focus-specific too; this allows us to get rid of the existing if too.

I also added two more cleanup commits while I was at it.

[ChrisMcD1]

Okay, this bug makes more sense and this fix looks good for it.

I just spent a chunk of time diving more deeply into the core architecture, and this is the sense I came away with

I'm still a bit confused about the role of ContextMgr versus parentContext. Why do we not just push the mainView onto the ContextMgr's ContextStack when a user clicks into it? Versus having each context keep track of its parentContext?

In fact, it does seem like we push onto the ContextMgr's stack here, so why is parentContext needed at all?

[stefanhaller]

Very nice picture!

A few things can probably be improved about it (but these are nitpicks):

• "View queries Model." This is not true, the view simply draws the runes that were put into it, and it is the context that does this. So it's the context that queries the model, not the view.
• "Context contains View." I find this misleading, it sounds like the context has an instance of a view as a field. But views are owned by the gocui layer, so instead of "contains" we should maybe say "references" or "uses".
• "Controller modifies Model." I'm not aware of any cases where this happens, except for the optimistic modification of a File's status when you stage it, which is just an optimization. Normally, the controller changes the repo state by issuing git commands, and Refresh then brings the model up to date with those changes.

I'm still a bit confused about the role of ContextMgr versus parentContext. Why do we not just push the mainView onto the ContextMgr's ContextStack when a user clicks into it? Versus having each context keep track of its parentContext?

Good question. ContextMgr and parentContext are orthogonal concepts; the stack is used for cases where a context is pushed on top and popped again, e.g. a popup, or the focused main view. The parentContext is usually used for longer-term relationships that survive switching side panels. As far as I'm aware, the only such cases currently are the SubCommits and the CommitFiles contexts, which keep track of what their parent side panel is, and they stay visible even when you switch to another side panel. The design around these is a bit awkward though: it should be possible to see two SubCommits views at once, one in the branches panel and one in the Reflog panel. But as soon as you open the second one, the first one goes away because we only have one context.

And now we come to how and why I'm using the parentContext for the focused main view: it's a bit of an abuse of this property. You are right that the focus handling uses the stack; it doesn't need the parentContext in the way we use it in other cases, as described above. But I also need to know what the owning side context is (so that we can ask it to handle clicks in the already focused main view, see here, and rerender the view on refresh, see here), and the ContextManager doesn't have an API for "give me the next view in the stack". The parentContext field was a convenient place to store this. Maybe it would have been better to add an extra field for this to MainContext, e.g. owningSidePanelContext or some such, but then I'd need a type assertion in postRefreshUpdate, so I'm not sure that's really better.

[stefanhaller]

I pushed a WIP commit that adds owningSidePanelContext. I find the type assertion in postRefreshUpdate not so bad after all; we need some check whether the current context is a main context, and previously we checked based on the key, now we check based on the type, and then use the type. I guess that's fine.

Edit: we need the same type assertion in the the two currentSidePanel functions in context_lines_controller.go and rename_similarity_threshold_controller.go. I missed these at first, fortunately we have a test for one of them which crashed.

[jesseduffield]

the ContextManager doesn't have an API for "give me the next view in the stack".

If we were to add such an API, would that be a viable alternative to the current fix? If so I think that would be good cos it would spare us from having to the owningSidePanelContext field. I can't imagine a case where the main view would not be on top of a side panel in the context stack.

[stefanhaller]

I didn't do that initially because it kind of didn't feel right to rely on the stack order; but I suppose you are right and we can. Changed in 40f8092.

(I didn't add documentation because none of the other functions in IContextMgr have any, and I was too lazy to add some to all these. For the new one it would be worth documenting that we return nil if the context doesn't have a next one, but panic if it isn't in the stack at all. We might also question this behavior, of course.)

[jesseduffield]

LGTM