[Refactoring] Improve shortcuts action #9397
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #9397 +/- ##
===========================================
- Coverage 73.75% 73.75% -0.01%
===========================================
Files 447 447
Lines 46538 46541 +3
Branches 3937 3936 -1
===========================================
- Hits 34326 34324 -2
- Misses 12212 12217 +5
🚀 New features to boost your workflow:
|
|
It seems that something might have been affected by the recent refactoring. |
|
Thanks, updated. |
There was a problem hiding this comment.
I see several problems in the current approach which probably not related to this refactoring, but please, check it in advance.
The first one has described in the code.
The second one: when we uploading shortcuts from local storage, we are looking for conflicts between local storage shortcuts and current keymap in the application. The problem here is that actually shortcuts from local storage may not have any conflicts between each other, when there may be conflicts between local storage shortcuts and current keymap shortcuts, which are basically just default values defined in the code (however these conflicts just do not make sense as we are going to update all sequences in current keymap from local storage values)
Speaking "update all sequences" I mean, in general, that all sequences are usually re-written, however there are also corner cases when application may have new shortcuts not saved in the local storage, or part shortcuts from local storage have been removed. This case will not work correctly as well.
As consequence, we may see several warnings in the browser console:
First conflict detector removes sequence for SWITCH_SHORTCUTS, leaving TOGGLE_ANNOTATION_PAGE equal to f1.
Then in reducer it again runs conflict detector and find the conflict again.
Refactoring LGTM, however the feature still needs some improvements.
| Object.keys(conflicts).forEach((conflictingKey) => { | ||
| resultMap[conflictingKey].sequences = removeConflictingSequences( | ||
| resultMap[conflictingKey].sequences, | ||
| conflicts[conflictingKey].sequences, | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Let's consider:
- wrongShortcut to be a shortcut which has conflicts with any other shortcuts
- conflictingShortcut to be a shortcut which conflicts with wrong shortcut
E.g. I set TOGGLE_ANNOTATION_PAGE to f1 in local storage
And we have SWITCH_SHORTCUTS also having f1 sequence
TOGGLE_ANNOTATION_PAGE in this scheme is wrongShortcut
SWITCH_SHORTCUTS isconflictingShortcut
The problem I see: we are filtering sequences of the conflictingShortcut (SWITCH_SHORTCUTS), however it would be more correct to filter sequences of wrongShortcut (TOGGLE_ANNOTATION_PAGE)
|
|
Ok, I see the feature still requires improvements in logic. But refactoring is a step for improving this. Lets merge it for now. |
Motivation and context
The
restoreSettingsAsyncfunction has become quite complex, especially the section with four nested loops. In this PR, I’ve refactored the logic by extracting parts into separate functions to improve readability and maintainability.Related #9032
How has this been tested?
Checklist
developbranch[ ] I have created a changelog fragment[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)License
Feel free to contact the maintainers if that's a concern.