Skip to content
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

Integrate keyboard manager with settings v2[Part-2] #2107

Conversation

udit3333
Copy link
Contributor

Summary of the Pull Request

Adding Custom Actions to trigger Keyboard Manager module to launch Remap Keyboard and Edit Shortcuts windows.

PR Checklist

Detailed Description of the Pull Request / Additional comments

  1. Added bindings to trigger on button click action on new settings UI.
  2. Added logic to allow background runner to launch window on foreground.
  3. Updated Keyboard Manager module to launch the Windows on valid Custom Actions.

Validation Steps Performed

Validated locally by launching both Remap Keyboard and Edit Shortcuts window from the settings UI.

@udit3333 udit3333 requested review from yuyoyuppe, enricogior and a team April 13, 2020 15:11
@yuyoyuppe
Copy link
Collaborator

  • I've tried launching Debug version locally, but Settings window doesn't open for me (nothing happens).
  • Could you please help me understand why do we need to create separate windows via custom actions instead of showing all settings inside a Settings v2 page?

@udit3333
Copy link
Contributor Author

  • I've tried launching Debug version locally, but Settings window doesn't open for me (nothing happens).

@yuyoyuppe This is an known issue currently with new settings projects I'm adding an Github issue for it now. What is happening the Microsoft.PowerToys.Settings.UI.Runner on building deletes the dependencies dll alternatively . Can you try build the solution again and launch Powertoys from the exe and make sure the output directory of Microsoft.PowerToys.Settings.UI.Runner project x64\Debug\netcoreapp3.1 looks like this:
image

  • Could you please help me understand why do we need to create separate windows via custom actions instead of showing all settings inside a Settings v2 page?

Keyboard Manager UI windows are being implemented on module side mainly because the logic to receive the Key stroke with LowLevelKeyboardHook and interpret the Key stroke as a remapped stroke on UI is implemented in the module.
Adding @arjunbalgovind if I'm missing other reasons.

Copy link
Collaborator

@yuyoyuppe yuyoyuppe left a comment

Choose a reason for hiding this comment

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

Yep, that helped. We'd like to minimize the pt module interface in the future, so please consider using another mechanism to communicate between settings and the module (preferably via some .json file)

Copy link
Contributor

@arjunbalgovind arjunbalgovind left a comment

Choose a reason for hiding this comment

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

LGTM

src/modules/keyboardmanager/ui/EditKeyboardWindow.cpp Outdated Show resolved Hide resolved
src/modules/keyboardmanager/ui/EditKeyboardWindow.cpp Outdated Show resolved Hide resolved
src/modules/keyboardmanager/ui/EditKeyboardWindow.cpp Outdated Show resolved Hide resolved
src/modules/keyboardmanager/ui/EditShortcutsWindow.cpp Outdated Show resolved Hide resolved
@udit3333
Copy link
Contributor Author

Yep, that helped. We'd like to minimize the pt module interface in the future, so please consider using another mechanism to communicate between settings and the module (preferably via some .json file)

Sure I'm currently leveraging the logic implemented in SettingsV2 which uses Ipc to communicate with the modules. As we update the logic for new Settings and modules to communicate with either file based trigger implementation or some other mechanism we can leverage same logic here as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants