-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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: Fixed an issue where it didn't work to map certain keys to Actions #15420
Fix: Fixed an issue where it didn't work to map certain keys to Actions #15420
Conversation
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.
You know our codebase well!!!
LGTM. code wise.
As we are not in an area that needs unicode support, could you please send a screenshot or video recording of the validation?
Co-authored-by: 0x5bfa <62196528+0x5bfa@users.noreply.github.com>
@yaira2 I found the cause of the NumKeys mapping not working, Not localized Pad0-9 Now it's workedTesting.KeyMap.165756.mp4 |
Now ignored non-valid keys aka Red Box and more optimalization for speed |
…ity with detailed comments.
Let's get to testing
|
…XTorLukas/Files into xtorlukas/Fix-HotKeyCharCorrect
…XTorLukas/Files into xtorlukas/Fix-HotKeyCharCorrect
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.
LGTM other than two things below, code wise.
I can test these on my end this weekend.
src/Files.App/UserControls/KeyboardShortcut/KeyboardShortcut.xaml
Outdated
Show resolved
Hide resolved
Co-authored-by: 0x5BFA <62196528+0x5bfa@users.noreply.github.com>
…XTorLukas/Files into xtorlukas/Fix-HotKeyCharCorrect
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.
Code Quality is over A+.
Looks great to me.
I haven't tested on my end but if someone did I think this can be merged.
If still opened, I'll test this weekend.
Thank you, I'm glad I'm not a useless liability here even though I've only been in this community for a short time. I'll definitely keep trying to improve the quality of the code. |
I just have an idea for the future, it would be good to merge all the work with "Key binding" into one class and split it into functions, at least now it's too much for one method Files/src/Files.App/Views/Settings/ActionsPage.xaml.cs Lines 54 to 143 in d015e04
|
Definitely. |
Sounds good. |
Will this have any effect on existing users? |
It won't be, I just fixed the correct display of mapped keys and added an informative teaching tip about invalid key binding |
Resolved / Related Issues
To prevent extra work, all changes to the Files codebase must link to an approved issue marked as
Ready to build
. Please insert the issue number following the hashtag with the issue number that this Pull Request resolves.Steps used to test these changes
Stability is a top priority for Files and all changes are required to go through testing before being merged into the repo. Please include a list of steps that you used to test this PR.