Skip to content
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

Make losing focus on Confirmations count as closing them #4277

Conversation

ChrisMcD1
Copy link
Contributor

@ChrisMcD1 ChrisMcD1 commented Feb 17, 2025

  • PR Description

This will fix #4052 where new users click out of the panel, but the state never gets updated.

It makes sense to do this as a close and not a confirm, because losing focus could be due to many things, and doesn't indicate an explicit confirmation.

It is up to the implementation of the initial popup message that a cancelling or closing should mean they don't see it again. Most popups don't have a meaningful ConfirmOpts.OnClose method, so this change will be a no-op for them.
The helper that creates the OnClose method I call here should ensure that it is not nil in normal operations, but I fear there could be some moment where it ends up as nil, so I'm guarding against that.

Here is a list of potentially impacted closing methods. The only one I worry about a little bit because I am unfamiliar with its section of code is the CredentialsHelper.

Introductory Message (what the issue was reported on):

lazygit/pkg/gui/gui.go

Lines 1008 to 1020 in 01eece3

onConfirm := func() error {
gui.c.GetAppState().StartupPopupVersion = StartupPopupVersion
err := gui.c.SaveAppState()
gui.waitForIntro.Done()
return err
}
gui.c.Confirm(types.ConfirmOpts{
Title: "",
Prompt: gui.c.Tr.IntroPopupMessage,
HandleConfirm: onConfirm,
HandleClose: onConfirm,
})

Breaking Changes Message

lazygit/pkg/gui/gui.go

Lines 1077 to 1087 in 01eece3

onConfirm := func() error {
gui.waitForIntro.Done()
return nil
}
gui.c.Confirm(types.ConfirmOpts{
Title: gui.Tr.BreakingChangesTitle,
Prompt: gui.Tr.BreakingChangesMessage + "\n\n" + message,
HandleConfirm: onConfirm,
HandleClose: onConfirm,
})

CredentialsHelper

HandleClose: func() error {
ch <- "\n"
return nil
},

Fixes #4052

  • 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

This will fix jesseduffield#4052
where new users click out of the panel, but the state never gets
updated.

It makes sense to do this as a close and not a confirm, because losing
focus could be due to many things, and doesn't indicate an explicit
confirmation.

It is up to the implementation of the initial popup message that a
cancelling or closing should mean they don't see it again.
@ChrisMcD1 ChrisMcD1 marked this pull request as ready for review February 17, 2025 03:08
@ChrisMcD1
Copy link
Contributor Author

Converting back into draft while I figure out how to prevent the double-closing problem that the integration tests revealed.

@ChrisMcD1 ChrisMcD1 marked this pull request as draft February 17, 2025 04:15
@ChrisMcD1
Copy link
Contributor Author

After poking around and making the specified HandleClose methods idempotent with some atomic bools, I realized that there really is nothing we can do because the wrapper around the close method itself is non-idempotent. The assumption that these are called only once is too baked in.

func (self *ConfirmationHelper) wrappedConfirmationFunction(cancel goContext.CancelFunc, function func() error) func() error {
return func() error {
cancel()
self.c.Context().Pop()
if function != nil {
if err := function(); err != nil {
return err
}
}
return nil
}
}

Here's some potential alternatives I'm thinking of:

  1. While a Confirmation window is open, we block all other keystrokes that are not confirm or cancel. (Could be confusing for the users of the original ticket that didn't notice the intended keybindings in the bottom of the screen. That could be alleviated by adding something to the welcome message telling them to hit to close)
  2. We add a separate OnFocusLost method that only these few things have defined. It gets no wrapping, so it can be safely called more than once if the caller guarantees idempotency.

@stefanhaller
Copy link
Collaborator

While a Confirmation window is open, we block all other keystrokes that are not confirm or cancel

We don't have to go this far. As far as I can tell, the only way to switch focus while a confirmation is showing is with the number keys, so it's probably enough to prevent this in JumpToSideWindowController.goToSideWindow.

I like preventing this better than auto-closing the confirmation when it loses focus; seems more robust to me.

@ChrisMcD1
Copy link
Contributor Author

Closed in favor of #4284

@ChrisMcD1 ChrisMcD1 closed this Feb 18, 2025
@ChrisMcD1 ChrisMcD1 deleted the 4052-welcome-message-persisting branch February 18, 2025 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Welcome message not dissapearing after first open
2 participants