fix: retest/revert advanced settings functionality after refactor #149
Conversation
…sh data retrieval
…reflect recent changes
…itial layout state
…al layouts based on advanced settings
📝 WalkthroughWalkthroughAdds Kanata disconnect callback that can restore a prior layout, exposes config-cache clearing, adds a KeyboardLayout Changes
Sequence Diagram(s)sequenceDiagram
participant Kanata as KanataService
participant App
participant Config as ConfigService
participant MethodHandler as MethodCallHandler
participant KeySrv as KeyEventService
participant Visibility as VisibilityService
rect rgb(240,248,255)
Note over Kanata,App: Kanata disconnect → restore flow
Kanata->>App: onDisconnect callback
App->>Config: clearCache()
Config-->>App: cache cleared
App->>KeySrv: read keyboard state
App->>App: read prefs
alt Kanata enabled && initialLayout exists
App->>KeySrv: restore previous layout
App->>Visibility: fadeIn()
Visibility-->>App: window shown
end
end
rect rgb(255,250,240)
Note over MethodHandler,Config: Method calls can clear config cache
MethodHandler->>Config: invoke clearConfigCache() callback
Config-->>MethodHandler: cache cleared
MethodHandler->>Config: loadConfig()
Config-->>MethodHandler: fresh config loaded
MethodHandler->>KeySrv: apply layout/settings
KeySrv-->>MethodHandler: confirm applied
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/screens/keyboard_screen.dart (1)
59-62: Consider extracting the repeatedaltLayoutternary into a helper.The same conditional expression
(prefsState.advancedSettingsEnabled && prefsState.showAltLayout) ? prefsState.altLayout : nullis duplicated 8+ times. Extracting it to a local variable at the start ofbuildRowwould reduce repetition and improve readability.🔎 Proposed refactor
Widget buildRow( int rowIndex, List<String> keys, KeyboardState keyboardState, PreferencesState prefsState, ) { List<Widget> rowWidgets = []; + final KeyboardLayout? effectiveAltLayout = + (prefsState.advancedSettingsEnabled && prefsState.showAltLayout) + ? prefsState.altLayout + : null; if (keyboardState.keymapStyle != 'Matrix' && keyboardState.keymapStyle != 'Split Matrix') { for (int i = 0; i < keys.length; i++) { // ... rowWidgets.add(buildKeys( // ... - altLayout: - (prefsState.advancedSettingsEnabled && prefsState.showAltLayout) - ? prefsState.altLayout - : null, + altLayout: effectiveAltLayout, )); } } // Apply similarly to other usages...Also applies to: 85-88, 94-97, 106-109, 115-118, 145-148, 153-156, 161-164
lib/services/method_call_handler.dart (1)
418-419: RedundantfadeIn()calls when disabling advanced settings.
fadeIn()is called at line 419 within the!advancedSettingsEnabledblock, and again at line 443 in the else branch (which is also!advancedSettingsEnabled). One of these calls is redundant.🔎 Proposed fix
// Ensure keyboard is visible if it was hidden by advanced features - fadeIn(); } else { // Clear cached config when re-enabling advanced settings // to ensure recent changes to the config file are reflected clearConfigCache(); if (currentPrefsState.keyboardFollowsMouse) { startMouseTracking(true); } } if (advancedSettingsEnabled) { // ... loading logic } else { fadeIn(); }Also applies to: 442-444
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
README.mddocs/index.mdlib/app.dartlib/models/user_config.dartlib/screens/keyboard_screen.dartlib/services/config_service.dartlib/services/configuration_loader.dartlib/services/kanata_service.dartlib/services/key_event_service.dartlib/services/method_call_handler.dartlib/services/visibility_service.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (30)
docs/index.md (1)
20-26: LGTM! Documentation navigation improvements.The renamed links are more concise, and the new Locales entry complements the advanced features documentation.
README.md (1)
139-191: LGTM! Formatting consistency improvements.The formatting adjustments improve readability without changing any content or functionality.
lib/services/config_service.dart (1)
61-64: LGTM! Clean cache invalidation.The
clearCache()method provides a clean way to force configuration reload from file, which is useful for ensuring fresh config after external changes.lib/services/visibility_service.dart (1)
17-18: LGTM! Correct feature gating.The logic change properly disables the "hide on default layer" feature when advanced settings are disabled by returning
false(allowing the window to show). This aligns with the intent expressed in the comment.lib/services/kanata_service.dart (2)
10-10: LGTM! Well-designed disconnect callback.The
DisconnectCallbacktypedef andonDisconnectfield provide a clean way for external code to handle Kanata disconnection events.Also applies to: 19-19
71-74: LGTM! Proper disconnect notification.The callback is invoked conditionally (only when
_reconnectEnabledis true), which prevents spurious notifications during intentional disconnections.lib/services/configuration_loader.dart (3)
16-19: LGTM! Cache invalidation delegation.The
clearConfigCache()method properly delegates to the config service, providing a clean API for forcing config reload.
54-61: LGTM! Improved layout restoration logic.The change from
updateInitialLayouttoupdateDefaultUserLayoutcombined with the comment clarifies the intent: preserveinitialLayoutso it can be restored when the user layout is toggled off. This supports proper state restoration.
114-118: LGTM! Preserves pre-Kanata layout for restoration.The conditional check ensures
initialLayoutis only set once (when null), preserving the layout that was active before Kanata took control. This enables correct restoration when Kanata is disabled or disconnected.lib/app.dart (3)
108-120: LGTM! Proper disconnect handling with state restoration.The
onDisconnectcallback correctly:
- Checks that Kanata is still enabled in preferences (to avoid restoring when manually disabled)
- Verifies
initialLayoutexists before attempting restoration- Triggers a fade-in to show the restored layout
This ensures smooth recovery from unexpected Kanata disconnections.
167-168: LGTM! Ensures fresh configuration on reload.Clearing the cache before loading forces a fresh read from the config file, which is the expected behavior for a "reload configuration" operation.
509-509: LGTM! Clean callback delegation.Passing the
clearConfigCachecallback to the method handler enables cache invalidation during method call processing, maintaining consistency across configuration updates.lib/screens/keyboard_screen.dart (7)
71-75: LGTM!The
endIndexcalculation correctly differentiates between row 0 (11 keys) and other rows (12 for 6-column with advanced settings, 10 otherwise).
78-81: LGTM!The 6-column layout special handling is now correctly gated behind both
use6ColLayoutandadvancedSettingsEnabled.
123-129: LGTM!The split position logic correctly adjusts for 6-column layouts when advanced settings are enabled.
187-189: LGTM!The reactive shift behavior is now correctly gated behind
prefsState.reactiveShiftEnabled, ensuring users can control this feature independently.
205-207: LGTM!The key index adjustment correctly compensates for the extra column in 6-column layouts when advanced settings are enabled.
481-483: LGTM!The key index increment here correctly compensates for the earlier decrement in
buildKeys, ensuring the alt layout key lookup uses the correct index.
510-513: LGTM!The finger color calculation correctly bypasses the top-row index adjustment for 6-column layouts with advanced settings, avoiding double-adjustment.
lib/services/method_call_handler.dart (7)
44-44: LGTM!The
clearConfigCachecallback is appropriately added to ensure the configuration cache is refreshed when advanced settings are re-enabled.
88-99: LGTM!The conditional update logic correctly preserves the active Kanata/user layout while updating the fallback layout for later restoration.
473-477: Verify the intent: clearing alt layout data on toggle vs. hiding on advanced settings disable.When
showAltLayoutis toggled off (line 475),updateAltLayout(null)clears the stored alt layout. However, the comment at line 409 states "Don't clear altLayout data - just hide it so it can be restored later" when advanced settings is disabled.This creates two different behaviors:
- Toggling
showAltLayoutoff → clears data- Disabling
advancedSettingsEnabled→ preserves dataPlease confirm this asymmetry is intentional. If users expect alt layout to persist when toggled off (similar to advanced settings), consider only calling
updateShowAltLayout(false)without clearing the data.
480-496: LGTM!The custom font enable/disable logic correctly saves the current font before switching and restores it when disabled.
446-466: LGTM!The user layout toggle correctly preserves and restores the initial layout while respecting Kanata's active state to avoid conflicts.
503-531: LGTM!The Kanata enable/disable handling correctly saves/restores the layout and maintains proper state synchronization. The order of operations when disabling (restore layout → update states → disconnect) is sensible.
543-548: LGTM!Triggering
fadeIn()whenhideOnDefaultLayeris disabled ensures the keyboard becomes visible immediately.lib/services/key_event_service.dart (4)
16-17: LGTM!The
_previousLayerfield enables proper restoration to the exact layer the user was on before activating a held layer.
107-132: LGTM!The reworked auto-hide logic correctly handles layer awareness:
- Fades in on key press when hidden, then cancels timer if not on default layer
- Resets timer when on default layer (standard behavior)
- Cancels timer when on non-default layer to keep keyboard visible
295-313: LGTM!The
_isOnDefaultLayerhelper provides clear, hierarchical logic for determining the default layer status based on user layout configuration.
315-319: LGTM!Clearing
_previousLayeralong with_activeTriggersensures a clean state reset.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/services/key_event_service.dart (1)
316-319: Cleanup logic will need adjustment for the stack-based fix.The reset of
_previousLayeris appropriate for cleanup, but this will need to be updated to clear the stack once the multi-held-layer bug is fixed (see comment on lines 251-278).
🧹 Nitpick comments (2)
lib/screens/keyboard_screen.dart (1)
167-176: Extract duplicated shift mapping logic into a helper method.The shift mapping logic here is duplicated in
_getAltLayoutKey(lines 472-483). Consider extracting this into a reusable helper method to improve maintainability and ensure consistent behavior.🔎 Proposed refactor
Add a helper method to the class:
String _getShiftedKey( String key, KeyboardState keyboardState, PreferencesState prefsState, ) { bool isShiftPressed = (keyboardState.keyPressStates["LShift"] ?? false) || (keyboardState.keyPressStates["RShift"] ?? false); if (isShiftPressed && prefsState.reactiveShiftEnabled && keyboardState.fontFamily != '') { if (keyboardState.customShiftMappings != null && keyboardState.customShiftMappings!.containsKey(key)) { return keyboardState.customShiftMappings![key]!; } return Mappings.getShiftedSymbol(key) ?? key; } return key; }Then replace both occurrences with:
- bool isShiftPressed = (keyboardState.keyPressStates["LShift"] ?? false) || - (keyboardState.keyPressStates["RShift"] ?? false); - if (isShiftPressed && - prefsState.reactiveShiftEnabled && - keyboardState.fontFamily != '') { - if (keyboardState.customShiftMappings != null && - keyboardState.customShiftMappings!.containsKey(key)) { - key = keyboardState.customShiftMappings![key]!; - } else { - key = Mappings.getShiftedSymbol(key) ?? key; - } - } + key = _getShiftedKey(key, keyboardState, prefsState);And similarly in
_getAltLayoutKeyat lines 472-483.lib/services/key_event_service.dart (1)
194-204: Consider removing redundant state parameters.Both
_handleToggleLayerand_handleHeldLayeracceptkeyboardStateandappStateas parameters, but often read fresh state directly viaref.read()(e.g., lines 206, 225, 283). While not incorrect, this makes the parameters somewhat redundant and could be confusing.Consider refactoring to rely solely on
ref.read()for state access within these methods and removing the now-unused parameters from the signatures and call sites.Also applies to: 238-250
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/models/user_config.dartlib/screens/keyboard_screen.dartlib/services/key_event_service.dartlib/services/method_call_handler.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (21)
lib/screens/keyboard_screen.dart (6)
44-47: LGTM! Effective gating of alternative layout.The
effectiveAltLayoutlogic correctly ensures that the alternative layout is only shown when bothadvancedSettingsEnabledandshowAltLayoutare true. This provides clear control over the feature availability.
72-82: LGTM! Consistent gating of 6-column layout behavior.The end index logic and special handling for the first row in Split Matrix layouts are properly gated behind
use6ColLayout && advancedSettingsEnabled. This ensures that 6-column layout features are only active when advanced settings are enabled.
63-63: LGTM! Consistent propagation of effectiveAltLayout.All
buildKeyscalls correctly useeffectiveAltLayoutinstead of directly accessingprefsState.altLayout. This ensures the gating logic is consistently applied throughout the rendering pipeline.Also applies to: 86-86, 92-92, 101-101, 107-107, 134-134, 139-139, 144-144
185-187: LGTM! Correct index adjustments for 6-column layouts.The key index adjustments are properly mirrored:
- Line 185-187: Decrement in
buildKeysto normalize indices for downstream operations- Line 461-463: Increment in
_getAltLayoutKeyto access the correct position in the alternative layout arrayBoth are consistently gated behind
use6ColLayout && advancedSettingsEnabled.Also applies to: 461-463
490-491: LGTM! Correct finger color index calculation for 6-column layouts.The conditional adjustment correctly handles the top row index calculation:
- With 6-column layouts (advanced settings enabled): No adjustment needed, as the backtick column is present at index 0
- Without 6-column layouts: Adjust by -1 to account for the missing backtick column
167-169: Remove or document the font family dependency in reactive shift mapping.The condition
keyboardState.fontFamily != ''gates reactive shift symbol display at two locations (lines 167-169 and 474-476), but shift symbol mapping is purely character-based and font-agnostic. TheMappings.getShiftedSymbol()function performs basic character lookups regardless of font configuration. Either remove this coupling or document why custom fonts are required for shifted symbols to display correctly. IffontFamily == ''signals uninitialized state, use an explicit flag instead.lib/models/user_config.dart (1)
35-35: LGTM! Previous review issue resolved.The
foreignfield is now properly serialized intoJson()(line 71), addressing the concern raised in the previous review. Both deserialization (line 35) and serialization (line 71) follow the correct conditional pattern consistent with other optional fields liketriggerandtype.Also applies to: 71-71
lib/services/method_call_handler.dart (9)
88-98: LGTM! Conditional layout update logic is sound.The new conditional logic correctly handles layout updates based on whether advanced features (Kanata or user layout) are active:
- When Kanata or user layout is active: only updates
initialLayout(the revert target)- Otherwise: updates both current and initial layout
This prevents accidentally changing the displayed layout when users modify layout preferences while advanced features are active.
409-426: LGTM! Clear distinction between hiding and clearing altLayout.The logic correctly distinguishes between two scenarios:
- Disabling advanced settings (line 410): Hides
altLayoutwithupdateShowAltLayout(false)but preserves data for potential restoration- Explicitly toggling off showAltLayout (line 474): Clears with
updateAltLayout(null)This semantic difference makes sense—disabling advanced settings is temporary, while toggling off showAltLayout is an explicit user decision. The
clearConfigCache()call when re-enabling (line 422) ensures fresh config is loaded.
435-437: LGTM! Consistent use ofshowAltLayout.The reference to
currentPrefsState.showAltLayoutis consistent with other usages throughout the file (lines 410, 474, 476) and accurately reflects the property's purpose.
447-465: LGTM! Proper save/restore pattern with Kanata conflict prevention.The implementation correctly:
- Saves the current layout to
initialLayoutbefore switching to user layout (when Kanata is not active)- Restores from
initialLayoutwhen toggling off- Includes
!keyboardState.kanataEnabledchecks to prevent state conflicts when both advanced features are activeThis ensures the correct layout is restored regardless of the order in which features are toggled.
469-477: LGTM! Explicit toggle clears altLayout data.When explicitly toggling off
showAltLayout, the implementation correctly:
- Clears the alternative layout data with
updateAltLayout(null)- Hides the display with
updateShowAltLayout(false)This explicit clearing is appropriate for a user-initiated toggle (unlike the temporary hiding when disabling advanced settings).
482-495: LGTM! Consistent save/restore pattern for fonts.The implementation follows the same save/restore pattern used for layouts:
- Saves current font to
initialFontFamilybefore applying custom font- Restores from
initialFontFamilywhen disablingThis provides a good user experience by preserving and restoring the previous font selection.
506-530: LGTM! Proper Kanata lifecycle management with cleanup.The implementation correctly handles Kanata enable/disable:
- Saves/restores layout state with debug logging for troubleshooting
- Calls
kanataService.disconnect()when disabling (line 528) to properly clean up resources- Updates both keyboard and preferences state consistently
- Triggers
fadeIn()to display the restored layoutThe explicit disconnect ensures proper resource cleanup and prevents potential connection leaks.
545-547: LGTM! Appropriate fadeIn when re-showing keyboard.The conditional
fadeIn()call when!hideOnDefaultLayercorrectly triggers the visibility animation when the keyboard is being re-shown. No fadeIn is needed when hiding.
44-44: No issues found —handleMethodCallis called only once in the codebase (lib/app.dart line 491), and that call correctly provides theclearConfigCachecallback as the final parameter:() => _configLoader.clearConfigCache().lib/services/key_event_service.dart (5)
107-132: Auto-hide logic correctly handles state freshness.The approach of re-reading keyboard and app state after potential layer switching (lines 108-111) properly addresses stale state concerns. The conditional logic for fade-in and timer management based on default-layer status is sound.
205-236: Toggle layer logic is correct.Reading fresh state at line 206 before checking prevents stale state issues. The toggle-on/toggle-off logic correctly switches to the layer or reverts to default. Lines 225-227 re-read state after the update, which ensures correctness for the hide-on-default-layer feature.
280-291: Past review concern about stale state has been resolved.Line 283 now correctly reads fresh layout state via
ref.read(keyboardNotifierProvider).layoutinstead of using the stalekeyboardStateparameter. The visibility decision based on whether the current layout matches the default is now accurate.
295-313: Well-designed helper method for default-layer detection.The
_isOnDefaultLayermethod correctly handles multiple scenarios: user layouts, initial layouts, and the absence of configuration. The fallback totrue(line 312) is a sensible default.
194-204: Past review comment resolved: unusedtriggerKeyparameter removed.The previous review flagged an unused
triggerKeyparameter in_handleToggleLayer. This has been correctly addressed—the parameter no longer appears in the method signature, and the call site at lines 161-171 has been updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/services/key_event_service.dart (1)
253-306: Held layer logic correctly handles out-of-order releases.The implementation properly handles complex scenarios:
On key press (lines 254-261): Reads fresh layout state, pushes it onto the stack, and switches to the held layer.
On key release - still on held layer (lines 271-284): Pops from the stack and reverts to the previous layout (or default if stack is empty). Standard case.
On key release - moved away from held layer (lines 285-306): Handles the case where the user toggled to another layer while holding a key. Uses
lastIndexWhereto find and remove the held layer from the stack. This ensures proper reversion order when multiple layers are active.Example scenario this handles correctly:
- Press H1 → Stack: [Default], Current: H1
- Toggle T1 → Stack: [Default, H1], Current: T1
- Release H1 → Remove H1 from stack → Stack: [Default], Current: T1
- Toggle off T1 → Revert to Default
💡 Optional: Simplify the verbose comment
The comment at lines 286-295 includes stream-of-consciousness reasoning ("No, wait."). Consider condensing it:
- // We moved away from the held layer (e.g. toggled another layer on top) - // Remove the held layer's entry from the stack history - // We look for the layer that was pushed when this held layer was activated. - // Since we pushed `currentLayout` (the one before held) onto the stack, - // and we want to remove the effect of "Hold", we should actually remove - // the layer that *is* the held layer from the stack? - // No, wait. - // If we are on T1 (toggled from H1), the stack is [Default, H1]. - // We want to remove H1 from the stack so that T1 falls back to Default. - // So we remove the layer with name == layout.name. + // We moved away from the held layer (e.g., toggled another layer on top). + // Remove the held layer from the stack so that when the current layer is + // toggled off, it reverts to the correct previous layer. + // Example: If on T1 (toggled from H1), stack is [Default, H1]. + // Removing H1 ensures T1 falls back to Default, not H1.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/screens/keyboard_screen.dartlib/services/key_event_service.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/screens/keyboard_screen.dart
🔇 Additional comments (8)
lib/services/key_event_service.dart (8)
16-17: Excellent fix for multiple simultaneous held layers!Using
ref.readto access provider state without causing rebuilds is appropriate here, and the stack-based approach correctly addresses the critical issue from previous reviews where a single_previousLayerfield could not handle multiple held layers active concurrently.
45-45: LGTM: Proper resource cleanup.Clearing the stack in
dispose()prevents memory leaks and stale state.
107-132: Improved auto-hide logic with fresh state reads.The changes address the stale state issue from previous reviews by re-reading keyboard and app state after layer switching (lines 108-111). The auto-hide logic at lines 119-132 correctly handles:
- Window not visible + key pressed: Fade in, but cancel timer if not on default layer (since
fadeIn()starts it by default)- On default layer: Reset the auto-hide timer
- Key pressed while not on default: Cancel the timer
This provides proper auto-hide behavior that respects the current layer state.
174-174: LGTM: WidgetRef parameter enables fresh state reads.Adding the
refparameter allows_handleHeldLayerto read current keyboard state, preventing stale state issues.
199-225: Toggle layer logic correctly uses stack history.The implementation properly handles toggle behavior:
- Reads fresh layout state (line 200) to avoid stale data
- Switches TO the toggle layer when not currently on it, pushing the current layout onto the stack (lines 203-209)
- Reverts to the previous layer when already on the toggle layer, popping from the stack (lines 212-217)
The fallback to
defaultUserLayoutat lines 218-224 handles the case where the stack is empty.
309-322: LGTM: Proper visibility handling with fresh state.The logic correctly re-reads app state (line 311) and keyboard state (line 313) to ensure accurate visibility decisions when reverting from a held layer.
327-344: LGTM: Helper method centralizes default layer logic.The method correctly determines if the current layer is the default by:
- Checking against
defaultUserLayoutwhen user layouts are enabled (lines 332-334)- Falling back to
initialKeyboardLayoutotherwise (lines 337-340)- Defaulting to
truewhen no specific layout is configured (line 343)This provides a single source of truth for default layer determination.
349-349: LGTM: Consistent stack cleanup.Clearing the stack in
clearActiveTriggers()maintains consistency withdispose()and prevents stale state.
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.