From f456160c455d460b510fa8b07073603db7e77fd8 Mon Sep 17 00:00:00 2001 From: Jatin K Malik Date: Mon, 13 Jul 2026 18:53:00 -0700 Subject: [PATCH] fix(ui): keep preset and custom shortcut selection exclusive (#509) Selecting a preset after recording a custom combo left the custom entry filled, so both looked active. Sync the dropdown to "Custom Shortcut" when a non-preset is set, and clear the custom field when a preset wins. --- src/vocalinux/ui/settings_dialog.py | 140 +++++++++++++++++++--------- tests/test_settings_mode_change.py | 23 ++++- tests/test_settings_shortcuts.py | 18 ++++ 3 files changed, 133 insertions(+), 48 deletions(-) diff --git a/src/vocalinux/ui/settings_dialog.py b/src/vocalinux/ui/settings_dialog.py index 23b7b42..ae86c5c 100644 --- a/src/vocalinux/ui/settings_dialog.py +++ b/src/vocalinux/ui/settings_dialog.py @@ -1643,7 +1643,8 @@ class SettingsDialog(Gtk.Dialog): self.shortcut_combo.set_tooltip_text("Select the keyboard shortcut for voice typing") _prevent_scroll_on_hover(self.shortcut_combo) - # Populate shortcut options grouped by side + # Populate shortcut options grouped by side, then a Custom sentinel. + # Preset and custom are mutually exclusive: only one is active at a time. for group_label, shortcut_ids in SHORTCUT_GROUPS.items(): # Add group separator as a disabled label entry separator_id = f"__separator_{group_label}__" @@ -1651,13 +1652,12 @@ class SettingsDialog(Gtk.Dialog): for shortcut_id in shortcut_ids: display_name = SHORTCUT_DISPLAY_NAMES.get(shortcut_id, shortcut_id) self.shortcut_combo.append(shortcut_id, display_name) + self.shortcut_combo.append("__custom__", "Custom Shortcut") # Load current shortcut from config current_shortcut = self.config_manager.get_str( "shortcuts", "toggle_recognition", "ctrl+ctrl" ) - if not self.shortcut_combo.set_active_id(current_shortcut): - self.shortcut_combo.set_active_id("ctrl+ctrl") self.shortcut_row = PreferenceRow( title="Shortcut Key", @@ -1695,10 +1695,6 @@ class SettingsDialog(Gtk.Dialog): ) group.add_row(self.custom_shortcut_row) - # Reflect a non-preset active shortcut in the custom entry. - if current_shortcut not in SUPPORTED_SHORTCUTS: - self.custom_shortcut_entry.set_text(current_shortcut) - # Key-capture state for the Record button. self._recording_shortcut = False self.connect("key-press-event", self._on_shortcut_key_press) @@ -1725,6 +1721,9 @@ class SettingsDialog(Gtk.Dialog): self.shortcuts_tab.pack_start(info_box, False, False, 0) + # Reflect active shortcut in combo + custom entry (preset vs custom). + self._sync_shortcut_selection_ui(current_shortcut) + # Connect signals self.shortcut_combo.connect("changed", self._on_shortcut_changed) self.shortcut_mode_combo.connect("changed", self._on_shortcut_mode_changed) @@ -1732,6 +1731,51 @@ class SettingsDialog(Gtk.Dialog): # Update UI based on initial mode self._update_shortcut_ui_for_mode(current_mode) + def _is_preset_shortcut(self, shortcut: str) -> bool: + """Return True if shortcut is one of the built-in double-tap presets.""" + return shortcut in SUPPORTED_SHORTCUTS + + def _set_shortcut_combo_active_id(self, active_id: str) -> None: + """Select a combo item without firing the changed handler.""" + # Handler may not be connected yet (during section build). + try: + self.shortcut_combo.handler_block_by_func(self._on_shortcut_changed) + blocked = True + except TypeError: + blocked = False + try: + if not self.shortcut_combo.set_active_id(active_id): + self.shortcut_combo.set_active_id("ctrl+ctrl") + finally: + if blocked: + self.shortcut_combo.handler_unblock_by_func(self._on_shortcut_changed) + + def _sync_shortcut_selection_ui(self, shortcut: str) -> None: + """Keep preset combo and custom entry mutually exclusive. + + - Preset active: combo shows that preset; custom entry is cleared. + - Custom active: combo shows "Custom Shortcut"; entry holds the combo string. + """ + if self._is_preset_shortcut(shortcut): + self._set_shortcut_combo_active_id(shortcut) + self.custom_shortcut_entry.set_text("") + else: + self._set_shortcut_combo_active_id("__custom__") + self.custom_shortcut_entry.set_text(shortcut) + + def _report_shortcut_apply_result(self, display_name: str, applied: bool) -> None: + """Show success/restart feedback after a shortcut change.""" + if applied: + self.shortcut_info_label.set_markup( + f"Shortcut updated to {display_name}. " + "Active now!" + ) + else: + self.shortcut_info_label.set_markup( + f"Shortcut updated to {display_name}. " + "Restart the app for the change to take full effect." + ) + def _apply_custom_shortcut(self, shortcut: str) -> None: """Validate, persist, and live-apply a custom shortcut string.""" shortcut = shortcut.strip().lower() @@ -1743,8 +1787,24 @@ class SettingsDialog(Gtk.Dialog): ) return + # If the user typed/recorded a preset id, treat it as selecting that preset. + if self._is_preset_shortcut(shortcut): + self.config_manager.set("shortcuts", "toggle_recognition", shortcut) + self.config_manager.save_settings() + self._sync_shortcut_selection_ui(shortcut) + mode_id = self.shortcut_mode_combo.get_active_id() + display_name = SHORTCUT_DISPLAY_NAMES.get(shortcut, shortcut) + logger.info(f"Keyboard shortcut changed to preset via custom entry: {display_name}") + applied = False + if self.shortcut_update_callback: + applied = bool(self.shortcut_update_callback(shortcut, mode_id)) + self._report_shortcut_apply_result(display_name, applied) + return + self.config_manager.set("shortcuts", "toggle_recognition", shortcut) self.config_manager.save_settings() + # Dropdown should show Custom, not a leftover preset. + self._sync_shortcut_selection_ui(shortcut) mode_id = self.shortcut_mode_combo.get_active_id() display_name = get_shortcut_display_name(shortcut, mode_id) @@ -1752,17 +1812,8 @@ class SettingsDialog(Gtk.Dialog): applied = False if self.shortcut_update_callback: - applied = self.shortcut_update_callback(shortcut, mode_id) - if applied: - self.shortcut_info_label.set_markup( - f"Shortcut updated to {display_name}. " - "Active now!" - ) - else: - self.shortcut_info_label.set_markup( - f"Shortcut updated to {display_name}. " - "Restart the app for the change to take full effect." - ) + applied = bool(self.shortcut_update_callback(shortcut, mode_id)) + self._report_shortcut_apply_result(display_name, applied) def _on_custom_shortcut_apply(self, widget): """Handle Set button / Entry activation for a typed custom shortcut.""" @@ -1877,11 +1928,7 @@ class SettingsDialog(Gtk.Dialog): # Update UI to reflect new mode self._update_shortcut_ui_for_mode(mode_id) - # Try to apply the mode change live. Read the active shortcut from the - # saved config rather than the preset combo: a custom shortcut (e.g. - # "alt+r") is stored in config but leaves the preset combo pointing at a - # default/previous preset, so using the combo here would restart the - # listener on the wrong shortcut. + # Apply from saved config (source of truth for both preset and custom). if self.shortcut_update_callback: shortcut_id = self.config_manager.get_str( "shortcuts", "toggle_recognition", "ctrl+ctrl" @@ -1903,6 +1950,14 @@ class SettingsDialog(Gtk.Dialog): f"Restart the app for the change to take full effect." ) + def _revert_shortcut_combo_to_saved(self) -> None: + """Restore the combo selection to match the saved active shortcut.""" + current = self.config_manager.get_str("shortcuts", "toggle_recognition", "ctrl+ctrl") + if self._is_preset_shortcut(current): + self._set_shortcut_combo_active_id(current) + else: + self._set_shortcut_combo_active_id("__custom__") + def _on_shortcut_changed(self, widget): """Handle shortcut selection change.""" if self._initializing: @@ -1914,37 +1969,34 @@ class SettingsDialog(Gtk.Dialog): # Ignore separator entries (used for grouped display) if shortcut_id.startswith("__separator_"): - # Revert to the previously saved shortcut - current = self.config_manager.get_str("shortcuts", "toggle_recognition", "ctrl+ctrl") - self.shortcut_combo.set_active_id(current) + self._revert_shortcut_combo_to_saved() return - # Save to config + # Selecting "Custom Shortcut" does not change the active binding until + # the user records/sets one; just focus the custom entry for convenience. + if shortcut_id == "__custom__": + current = self.config_manager.get_str("shortcuts", "toggle_recognition", "ctrl+ctrl") + if not self._is_preset_shortcut(current): + self.custom_shortcut_entry.set_text(current) + self.custom_shortcut_entry.grab_focus() + self.shortcut_info_label.set_markup( + "Record or type a custom shortcut (e.g. alt+r), then click Set." + ) + return + + # Preset selected: clear any leftover custom entry so UI matches config. + self.custom_shortcut_entry.set_text("") self.config_manager.set("shortcuts", "toggle_recognition", shortcut_id) self.config_manager.save_settings() display_name = SHORTCUT_DISPLAY_NAMES.get(shortcut_id, shortcut_id) logger.info(f"Keyboard shortcut changed to: {display_name}") - # Try to apply the shortcut change live + applied = False if self.shortcut_update_callback: mode_id = self.shortcut_mode_combo.get_active_id() - success = self.shortcut_update_callback(shortcut_id, mode_id) - if success: - self.shortcut_info_label.set_markup( - f"Shortcut updated to {display_name}. " - f"Active now!" - ) - else: - self.shortcut_info_label.set_markup( - f"Shortcut updated to {display_name}. " - f"Restart the app for the change to take full effect." - ) - else: - self.shortcut_info_label.set_markup( - f"Shortcut updated to {display_name}. " - f"Restart the app for the change to take full effect." - ) + applied = bool(self.shortcut_update_callback(shortcut_id, mode_id)) + self._report_shortcut_apply_result(display_name, applied) def _build_test_section(self): """Build the Test Recognition section.""" diff --git a/tests/test_settings_mode_change.py b/tests/test_settings_mode_change.py index 2ecf4c0..e25d83e 100644 --- a/tests/test_settings_mode_change.py +++ b/tests/test_settings_mode_change.py @@ -1,9 +1,8 @@ """Regression test: switching shortcut mode must use the saved shortcut. -A custom shortcut (e.g. "alt+r") is stored in config but leaves the preset -combo pointing at a default/previous preset. When the user toggles between -Toggle and Push-to-Talk, the live listener must be restarted on the *saved* -shortcut, not the stale preset shown in the combo. (Codex P2 on PR #493.) +Config is the source of truth for both presets and custom shortcuts. When the +user toggles between Toggle and Push-to-Talk, the live listener must restart on +the *saved* shortcut (not a stale combo selection). (Codex P2 on PR #493.) The settings dialog can't be instantiated under the mocked-GTK test harness, so (like test_settings_shortcuts.py) this asserts against the source of the @@ -55,3 +54,19 @@ def test_mode_ui_double_tap_text_is_guarded_for_non_combo(): idx_double_tap = body.find("Double-tap this key") assert idx_if != -1 assert idx_double_tap > idx_if + + +def test_sync_ui_mutually_excludes_preset_and_custom(): + """Custom and preset UI state must not both look active at once.""" + body = _method_source("_sync_shortcut_selection_ui") + assert '_set_shortcut_combo_active_id("__custom__")' in body + assert 'self.custom_shortcut_entry.set_text("")' in body + assert "self._is_preset_shortcut(shortcut)" in body + + +def test_preset_change_clears_custom_entry(): + body = _method_source("_on_shortcut_changed") + # After a real preset id is chosen, clear the custom entry so users are not + # left thinking the old custom combo is still active. + assert 'self.custom_shortcut_entry.set_text("")' in body + assert 'shortcut_id == "__custom__"' in body diff --git a/tests/test_settings_shortcuts.py b/tests/test_settings_shortcuts.py index 0df4dba..3133dab 100644 --- a/tests/test_settings_shortcuts.py +++ b/tests/test_settings_shortcuts.py @@ -117,6 +117,24 @@ class TestSettingsDialogShortcutsSection(unittest.TestCase): self.assertIn("SHORTCUT_GROUPS", self.source_code) self.assertIn("SUPPORTED_SHORTCUTS", self.source_code) + def test_custom_shortcut_option_in_combo(self): + """Preset combo includes a Custom Shortcut sentinel for non-presets.""" + self.assertIn( + 'self.shortcut_combo.append("__custom__", "Custom Shortcut")', self.source_code + ) + self.assertIn("def _sync_shortcut_selection_ui(self, shortcut: str)", self.source_code) + + def test_preset_selection_clears_custom_entry(self): + """Selecting a preset clears leftover custom text so active binding is obvious.""" + self.assertIn('self.custom_shortcut_entry.set_text("")', self.source_code) + # Preset path in _on_shortcut_changed must clear before/while saving. + self.assertIn("def _on_shortcut_changed(self, widget)", self.source_code) + + def test_custom_apply_syncs_combo_to_custom(self): + """Applying a custom shortcut selects Custom in the dropdown.""" + self.assertIn("def _apply_custom_shortcut(self, shortcut: str)", self.source_code) + self.assertIn("self._sync_shortcut_selection_ui(shortcut)", self.source_code) + class TestKeyboardBackendsBase(unittest.TestCase): """Test cases for keyboard backends base module."""