Skip to content

Conversation

@0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Jun 10, 2024

Summary

The fix is basically to catch exceptions and use default keys for commands that have the same key(if other exception than ArgumentException the app uses the default keys for all commands).

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.

  1. Set the same key *1
  2. Open Files app
  3. See no crashes

Used this to test

  "ActionsV2": [
    {
      "CommandCode": "OpenHelp",
      "CommandParameter": "",
      "KeyBinding": "F1"
    },
    {
      "CommandCode": "OpenHelp",
      "CommandParameter": "",
      "KeyBinding": "Ctrl+C"
    }
  ],

@yaira2 yaira2 requested a review from hishitetsu June 10, 2024 19:52
@yaira2 yaira2 added the ready for review Pull requests that are ready for review label Jun 10, 2024
Copy link
Member

@hishitetsu hishitetsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crashes can be avoided, but changing duplicate key assignments from the settings doesn't seem to be saved.

@0x5bfa
Copy link
Member Author

0x5bfa commented Jun 14, 2024

I want to reset them in json too then
@yaira2 fyi

@hishitetsu
Copy link
Member

This can only happen by editing the configuration file directly, so I think it is fine to leave it as it is if avoiding crashes is enough.

@0x5bfa
Copy link
Member Author

0x5bfa commented Jun 15, 2024

I mean, instead of using default ones, I'd like to reset in json first and reload. What do you think? Losing user data without notice would be bad but this reload happens before MainPage loaded, meaning dialog cannot be shown.

@hishitetsu
Copy link
Member

I mean, instead of using default ones, I'd like to reset in json first and reload. What do you think? Losing user data without notice would be bad but this reload happens before MainPage loaded, meaning dialog cannot be shown.

I don't think it is a good idea, unless structure of the json is broken.

@0x5bfa
Copy link
Member Author

0x5bfa commented Jun 15, 2024

Okay, I’ll just fix

@hishitetsu
Copy link
Member

It seems that the app no longer starts when there is a duplicate key.

@0x5bfa
Copy link
Member Author

0x5bfa commented Jun 16, 2024

Proven I’m bad at bug fix.
Let me take a look tonight.

Copy link
Member

@hishitetsu hishitetsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed it works. LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed ready for review Pull requests that are ready for review labels Jun 18, 2024
@yaira2
Copy link
Member

yaira2 commented Jun 18, 2024

What solution did we end up going with?

@hishitetsu
Copy link
Member

If there are any custom hotkeys that duplicate other hotkeys, they are removed from the json and restored to default.

@yaira2 yaira2 merged commit 30707d6 into files-community:main Jun 18, 2024
@yaira2 yaira2 changed the title Fix: Fixed ArgumentException at CommandManager.OverwriteKeyBindings Fix: Fixed ArgumentException in CommandManager.OverwriteKeyBindings Jun 18, 2024
@yaira2
Copy link
Member

yaira2 commented Jun 18, 2024

Thank you @0x5bfa and @hishitetsu for your work on this fix!

@0x5bfa 0x5bfa deleted the 5bfa/Fix-ArgumentException-15331 branch June 18, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge Pull requests that are approved and ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: CommandManager.OverwriteKeyBindings System.ArgumentException

3 participants