-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add a user configuration to enable arrow keys to move the cursor #153
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
Conversation
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #153 +/- ##
==========================================
+ Coverage 91.58% 91.59% +0.01%
==========================================
Files 123 124 +1
Lines 24921 25279 +358
==========================================
+ Hits 22823 23154 +331
- Misses 2098 2125 +27
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a user-configurable option to enable arrow keys for cursor movement in normal mode, specifically designed to help users with non-standard keyboard layouts (like bépo) where h/j/k/l keys are not conveniently positioned for navigation.
Changes:
- Added persistent application configuration storage system with JSON serialization
- Introduced
enable_arrow_keys_in_normal_modeconfiguration option that maps arrow keys to h/j/k/l movement commands in normal mode - Modified application state initialization to support custom configuration via new
with_configmethod
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/config/app_config.rs | New module implementing configuration storage and persistence with JSON serialization to ~/.config/helix-trainer/config.json |
| src/config/mod.rs | Added app_config module and exports for AppConfig and ConfigStorage |
| src/ui/state/substates.rs | Added enable_arrow_keys_in_normal_mode boolean field to ConfigState with default value of false |
| src/ui/state/mod.rs | Added with_config method to support custom configuration initialization while maintaining backward compatibility with existing new method |
| src/main.rs | Integrated configuration loading on startup and saving on exit, with error handling and logging |
| src/input/handlers.rs | Implemented arrow key to h/j/k/l mapping logic with conditional behavior based on configuration, including comprehensive tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bug-ops
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Review
Thanks for the contribution! Arrow key support for non-standard keyboard layouts like bépo is a useful feature.
I've reviewed the implementation and found a few issues that should be addressed before merging.
Summary
✅ Good:
- Clean implementation following existing
ProfileStoragepattern - Feature is opt-in (disabled by default)
- Good test coverage
- Error handling uses
Result<_, String>instead of proper error types - Field duplication between
AppConfigandConfigState - Config is saved on every exit regardless of changes
See inline comments for specific suggestions.
fe46af4 to
7c28006
Compare
Signed-off-by: Jean-François <jfm@laposte.net>
Signed-off-by: Jean-François <jfm@laposte.net>
Signed-off-by: Jean-François <jfm@laposte.net>
Signed-off-by: Jean-François <jfm@laposte.net>
7c28006 to
2cc46aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Hey, @bilboquet! Great implementation! A few items to consider for future PRs:
These don't block the PR but worth revisiting when refactoring config handling. |
Description
This PR adds the support for arrow keys in navigation mode. I need this because I use the "bepo" layout, so h,j,k and l are dispersed around my keyboard, and it does not make much sense to use them for navigating.
Remapping c,t,s and r - that are at the same place as h,j,k and l - would conflict very badly helix key bindings.
I hope this would be useful to other non "standard" keyboard layout.
Type of Change
Related Issues
Closes #
Changes Made
Testing
Test Results
cargo nextest run --lib)# Test commands run cargo nextest run --lib cargo clippy -- -D warnings cargo +nightly fmt -- --check cargo deny checkTest Coverage
Screenshots/Demo
Checklist
Performance Impact
Security Considerations
Additional Notes