Navigation Menu

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

fix(list): disable quit keybinding while filtering #118

Merged
merged 1 commit into from Feb 8, 2022

Conversation

lorenries
Copy link
Contributor

The fix in #108 resets the quit keybinding after the switch on the filter state. Instead, it should get reset in the switch's default case so it doesn't override turning it off in the filtering state.

@muesli muesli added the bug Something isn't working label Feb 8, 2022
@aymanbagabas
Copy link
Member

The fix in #108 resets the quit keybinding after the switch on the filter state. Instead, it should get reset in the switch's default case so it doesn't override turning it off in the filtering state.

You're right, the quit keybinding will get overwritten when filtering after the switch. However, this will also cause a bug where the keybinding gets disabled after the filtering state. i.e. keybinding is enabled, gets disabled in the filtering state, won't be enabled again in the default state because it uses the previous value.

Instead, I think we need to introduce a disableQuitKeybindings property in the model struct:

diff --git a/list/list.go b/list/list.go
index e18cc71f9e64..9380e0e967aa 100644
--- a/list/list.go
+++ b/list/list.go
@@ -107,6 +107,8 @@ type Model struct {
 	// Key mappings for navigating the list.
 	KeyMap KeyMap
 
+	disableQuitKeybindings bool
+
 	// Additional key mappings for the short and full help views. This allows
 	// you to add additional key mappings to the help menu without
 	// re-implementing the help component. Of course, you can also disable the
@@ -521,6 +523,7 @@ func (m *Model) StopSpinner() {
 // Helper for disabling the keybindings used for quitting, incase you want to
 // handle this elsewhere in your application.
 func (m *Model) DisableQuitKeybindings() {
+	m.disableQuitKeybindings = true
 	m.KeyMap.Quit.SetEnabled(false)
 	m.KeyMap.ForceQuit.SetEnabled(false)
 }
@@ -591,7 +594,6 @@ func (m Model) itemsAsFilterItems() filteredItems {
 
 // Set keybindings according to the filter state.
 func (m *Model) updateKeybindings() {
-	quit := m.KeyMap.Quit.Enabled()
 	switch m.filterState {
 	case Filtering:
 		m.KeyMap.CursorUp.SetEnabled(false)
@@ -624,6 +626,7 @@ func (m *Model) updateKeybindings() {
 		m.KeyMap.ClearFilter.SetEnabled(m.filterState == FilterApplied)
 		m.KeyMap.CancelWhileFiltering.SetEnabled(false)
 		m.KeyMap.AcceptWhileFiltering.SetEnabled(false)
+		m.KeyMap.Quit.SetEnabled(!m.disableQuitKeybindings)
 
 		if m.Help.ShowAll {
 			m.KeyMap.ShowFullHelp.SetEnabled(true)
@@ -634,7 +637,6 @@ func (m *Model) updateKeybindings() {
 			m.KeyMap.CloseFullHelp.SetEnabled(minHelp)
 		}
 	}
-	m.KeyMap.Quit.SetEnabled(quit)
 }
 
 // Update pagination according to the amount of items for the current state.

@lorenries
Copy link
Contributor Author

@aymanbagabas Ah yes, good point! Just updated the branch with your patch 👍🏻

@aymanbagabas
Copy link
Member

@lorenries could you please combine the 2 commits and rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants