Skip to content

[Kbm] Save the remaps to file[part-1]#2184

Merged
udit3333 merged 18 commits into
microsoft:dev/build-featuresfrom
udit3333:user/udit3333/Save-remaps-1
Apr 20, 2020
Merged

[Kbm] Save the remaps to file[part-1]#2184
udit3333 merged 18 commits into
microsoft:dev/build-featuresfrom
udit3333:user/udit3333/Save-remaps-1

Conversation

@udit3333

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

Adding the logic to save the Keyboard Manager configuration to file, persist the configuration on restart and event trigger in Settings to fetch the updates in configuration.

Todo(In a separate PR ): Update the UI on settings list view to reflect the real data.

PR Checklist

Detailed Description of the Pull Request / Additional comments

  1. Current Design is to save the configuration independent from the settings.json in a separate file config-name.json. This will be make the support of multiple configuration easier and configurations file can be shared or use templates.
  2. Added a fake Key Code to represent equivalent to VK_WIN.
  3. Current Design is updating a dummy settings-updated.json to communicate to Settings process that the config json have been successfully updated so settings process can read the file. The reason for using because FileSystemWatcher raises trigger events multiple time during a file update and the settings process have no knowledge if the runner process is still writing to the file. Alternatively, a polling approach can be used here as well to retry after sometime to see if file is free.
  4. FileSystemWatcher fires multiple events on writing to a file. Added a logic to treat all the events fired with a Time threshold as one.

Validation Steps Performed

Validate the changes locally.

@udit3333 udit3333 requested a review from a team April 17, 2020 16:20
@udit3333 udit3333 added the Product-Keyboard Shortcut Manager Issues regarding Keyboard Shortcut Manager label Apr 17, 2020
Comment thread src/modules/keyboardmanager/common/KeyboardManagerState.cpp Outdated
Comment thread src/modules/keyboardmanager/common/KeyboardManagerState.h Outdated
@arjunbalgovind

Copy link
Copy Markdown
Contributor

I noticed that the JSON file is loaded only when runner is launched. Just wanted to confirm is that how all the PowerToys behave? Like if I modify the JSON file after it is launched it will not recognize it? I suppose we can assume the user will not manually modify the JSON but I wanted to clarify that. And will that make the Settings UI side harder since I guess that will be reloaded whenever the file is changed?

Comment thread src/core/Microsoft.PowerToys.Settings.UI.Lib/KeyboadManagerConfigModel.cs Outdated
public class KeyboadManagerConfigModel
{
[JsonPropertyName("remapKeys")]
public List<KeysDataModel> RemapKeys { get; set; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we will be incorporating the registry approach in the future, we can add inProcess and registry subkeys to distinguish the two, or we can just add a separate key which checks if it is inProcess or registry. @saahmedm will we be allowing users to have active key to key remappings in both the registry and in process methods at the same time or will they be independent? Even if they are independent will we allow them to save some remappings of both methods in the config file that and then they toggle between the two with some switch or something? How we plan to handle that will affect how we structure the JSON to avoid a breaking change in the future.

Here's an example scenario:
Maybe the user prefers registry remaps in general and they remap a to b, b to c, etc with registry method. Then the user wants to remap RAlt but the SharpKeys approach doesn't support that afaik. So would we support a scenario where:

  • User can also add an inprocess remap for a few keys (as long as they are independent of the registry method)
    OR
  • User can save some set of remappings like RAlt to something with in process and they can switch between this and the registry approach (the remappings for both approaches would be stored). It would still need a reboot to switch between the two obviously.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@arjunbalgovind with your second scenario, where they switch between these. they would both be on a single config file? or that they switch to a config file with in-process remapping

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For both scenarios I meant single config file. A third scenario would be where you would need to switch to another config for that. Or in essence your config file only stores one method of key to key remapping.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have included the inProcess subkey in the data model for now. @saahmedm for the case if we want both the approaches independent what will happen when user choose to select the regsitry approach will we move all the single key remappings to registry(if possible) or have a separate list of remapping for both of the cases?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missed this guys. imo at one point users should be able to remap keys in-process and registry level at the same time. so the first scenario @arjunbalgovind mentioned.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can make that change on a separate PR, since this already merged. @saahmedm can you make an issue to track adjusting the JSON to support future in-process and registry remaps? We can mark that as a P0 so that the change is made before Build.

Comment thread src/core/Microsoft.PowerToys.Settings.UI.Lib/KeyboardManagerProperties.cs Outdated
Comment thread src/core/Microsoft.PowerToys.Settings.UI.Lib/Utilities/Helper.cs
Comment thread src/modules/keyboardmanager/common/Helpers.cpp
Comment thread src/modules/keyboardmanager/common/KeyboardManagerConstants.h
Comment thread src/modules/keyboardmanager/dll/dllmain.cpp
Comment thread src/modules/keyboardmanager/ui/EditShortcutsWindow.cpp Outdated
Comment thread src/modules/keyboardmanager/ui/EditShortcutsWindow.cpp Outdated
@arjunbalgovind

Copy link
Copy Markdown
Contributor

Where will the Enabled attribute for KBM be stored? Is it to be in the common settings.json? Cause I noticed that the Enable toggle doesnt work right now

@udit3333

Copy link
Copy Markdown
Contributor Author

I noticed that the JSON file is loaded only when runner is launched. Just wanted to confirm is that how all the PowerToys behave? Like if I modify the JSON file after it is launched it will not recognize it? I suppose we can assume the user will not manually modify the JSON but I wanted to clarify that. And will that make the Settings UI side harder since I guess that will be reloaded whenever the file is changed?

Yes in the current implementation it will load only on the startup. In other modules, SettingsV2 process currently sends an Ipc message to SetConfig endpoint in module if some property changes in the Settings UI. In KeyboardManager Settings currently there is nothing a user can change directly from the Settings UI page in the future when we add drop down to select the configuration we can load a new configuration on that event. In the case, when user manually updates the module settings.json I think the Settings UI will pick up those changes on UI when we load the page again and the updated changes will not be propagate to the module we should have an issue for this. And in case of, if user changes the configuration file for Keyboard Manager Settings UI will update the list view on page reload similar to other case. I guess we can send an event on load of the settings ui page to backend modules to refresh there settings or alternatively, replace the Ipc communication with file trigger on module sides to refresh everytime something changed.

@udit3333

Copy link
Copy Markdown
Contributor Author

Where will the Enabled attribute for KBM be stored? Is it to be in the common settings.json? Cause I noticed that the Enable toggle doesnt work right now

That's a good catch I missed the enable/disable logic for Keyboard Manager. I will look how we are doing currently with other modules and either update in this PR or create a separate issue/pr.

@arjunbalgovind arjunbalgovind left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! I think you'll have to resolve some merge conflicts from the latest changes, but apart from that great work! :)

@udit3333 udit3333 merged commit 325db53 into microsoft:dev/build-features Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Product-Keyboard Shortcut Manager Issues regarding Keyboard Shortcut Manager

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants