[Settings/Run] LowLevel Keyboard hooking for Hotkeys #3825

traies · 2020-05-28T16:10:59Z

Summary of the Pull Request

Replaced use of NHotkey.Wpf with low level keyboard hooks for both Settings and Run.

References

PR Checklist

Applies to [launcher] - Handling keyboard shortcuts in launcher must be in sync with the rest of powerToys #2325
CLA signed. If not, go over here and sign the CLA

Detailed Description of the Pull Request / Additional comments

Removed old Hotkey code from Run, including dependencies to the NHotkey nuget package.
Introduced new classes to the interop library, which can be used to register callbacks for Key Presses using low level hooking.

Validation Steps Performed

Start PowerToys.
Go to Settings\Run.
Change the Hotkey to start the Run prompt (for example, to Win + Space)
Wait a few seconds and use the hotkey.
The Run prompt should appear.

alekhyareddy28

LGTM. Verified that it works as expected. Had a small question regarding the expected behavior when the hotkeys are held pressed, is it taken as one key press or multiple? Right now it seems to be taking it as multiple key presses. If that is the expected behavior it LGTM.

alekhyareddy28 · 2020-06-01T16:48:06Z

src/common/interop/HotkeyManager.h

+        HotkeyManager();
+        ~HotkeyManager();
+
+        HOTKEY_HANDLE RegisterHotkey(Hotkey ^ hotkey, HotkeyCallback ^ callback);


Just curious, why are we using a ^? Is it because of interop?

The ^ character means a CLR pointer. This is because this is not standard C++ but rather C++/CLI. It just means a reference to a garbage collected (managed) object.

arjunbalgovind · 2020-06-01T17:37:50Z

Yet to look at the code, but I observed some things while testing:

Sometimes when I change the hotkey for launcher, click somewhere else on the window (for it to apply) and invoke the shortcut immediately it doesn't work. It works the next time I try it. This could be because it takes some time to apply, not really sure and may not be much of an issue.
Sometimes the PT Run window moves slightly off-center. I repro this more frequently if I change the hotkey and try invoking PT Run while Settings is in focus by pressing the hotkey multiple times.
This also has this start menu issue that users reported when they tried using the KBM workaround for using win key shortcuts for PT Run [PowerToys Run / KBM; BUG] PT Run opens behind Start menu #3469 . I'm not sure why Run's visibility does not close start menu automatically when triggered from the hook side, whereas it worked with registerhotkey side. That could be a separate issue which needs to be fixed which need not be in this PR. It's also odd that when start menu is open launcher opens off center - it seems to depend on windows that are open.

arjunbalgovind · 2020-06-01T17:40:43Z

The change affects the hotkey control for FZ as well, so we should loop in @enricogior. With this change users can enter windows shortcuts as well, however since the FZ hotkey works using the RegisterHotkey API those won't work.

I'll let Arjun approve this as he has more context.

alekhyareddy28 · 2020-06-01T18:06:15Z

Yet to look at the code, but I observed some things while testing:

Sometimes when I change the hotkey for launcher, click somewhere else on the window (for it to apply) and invoke the shortcut immediately it doesn't work. It works the next time I try it. This could be because it takes some time to apply, not really sure and may not be much of an issue.

Sometimes the PT Run window moves slightly off-center. I repro this more frequently if I change the hotkey and try invoking PT Run while Settings is in focus by pressing the hotkey multiple times.

This also has this start menu issue that users reported when they tried using the KBM workaround for using win key shortcuts for PT Run [PowerToys Run / KBM; BUG] PT Run opens behind Start menu #3469 . I'm not sure why Run's visibility does not close start menu automatically when triggered from the hook side, whereas it worked with registerhotkey side. That could be a separate issue which needs to be fixed which need not be in this PR. It's also odd that when start menu is open launcher opens off center - it seems to depend on windows that are open.

I think the off center issue is because of xaml islands. Not sure if it is related to this PR. @somil55 thoughts?

enricogior · 2020-06-01T18:06:37Z

Hi @arjunbalgovind
I'm going to take a look at how FZ editor hotkey is effected, thanks for the heads up.

enricogior · 2020-06-01T18:33:03Z

@arjunbalgovind
it seems to work as expected. Not sure what is the problem you are describing:

With this change users can enter windows shortcuts as well, however since the FZ hotkey works using the RegisterHotkey API those won't work.

"windows shortcuts" you mean generic win+key or "existing windows shortcut" that will be processed by the system and not passed to FZ?

arjunbalgovind

All the hook logic looks good. Not entirely sure what causes the weird interactions between launcher and low level hooks, it could be Xaml Islands as Alekhya mentioned so maybe we should validate if it still happens after moving to WPF.

arjunbalgovind · 2020-06-01T18:26:55Z

src/common/interop/HotkeyManager.cpp

+    handle |= hotkey->Win << 8;
+    handle |= hotkey->Ctrl << 9;
+    handle |= hotkey->Shift << 10;
+    handle |= hotkey->Alt << 11;


[nit] these can be constants rather than hardcoded numbers.

arjunbalgovind · 2020-06-01T18:32:36Z

src/common/interop/KeyboardHook.cpp

+    hookHandle = SetWindowsHookEx(
+        WH_KEYBOARD_LL, 
+        (HOOKPROC)(void*)Marshal::GetFunctionPointerForDelegate(hookProc), 
+        GetModuleHandle(msclr::interop::marshal_as<std::wstring>(curModule->ModuleName).c_str()),


Any reason why we cant pass in NULL like we do in existing code where we use SetWindowsHookEx?

After looking at the documentation more closely, I think you are right, this should be set to null since the hook procedure is within the process and not in a different dll. https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-setwindowshookexa

arjunbalgovind · 2020-06-01T19:22:03Z

@arjunbalgovind
it seems to work as expected. Not sure what is the problem you are describing:

With this change users can enter windows shortcuts as well, however since the FZ hotkey works using the RegisterHotkey API those won't work.

"windows shortcuts" you mean generic win+key or "existing windows shortcut" that will be processed by the system and not passed to FZ?

I meant existing windows shortcuts. Like if a user tries to set it to Win+Space or something like that. It was like this earlier as well, but before you couldn't even enter Win+Space so a user would never run into that problem.

enricogior · 2020-06-01T19:37:45Z

@arjunbalgovind
in the old settings it was the same and we didn't receive any complain (doesn't mean we won't in the future ;) )
But for now I think we are good.

traies · 2020-06-02T18:51:10Z

Yet to look at the code, but I observed some things while testing:

Sometimes when I change the hotkey for launcher, click somewhere else on the window (for it to apply) and invoke the shortcut immediately it doesn't work. It works the next time I try it. This could be because it takes some time to apply, not really sure and may not be much of an issue.

Sometimes the PT Run window moves slightly off-center. I repro this more frequently if I change the hotkey and try invoking PT Run while Settings is in focus by pressing the hotkey multiple times.

This also has this start menu issue that users reported when they tried using the KBM workaround for using win key shortcuts for PT Run [PowerToys Run / KBM; BUG] PT Run opens behind Start menu #3469 . I'm not sure why Run's visibility does not close start menu automatically when triggered from the hook side, whereas it worked with registerhotkey side. That could be a separate issue which needs to be fixed which need not be in this PR. It's also odd that when start menu is open launcher opens off center - it seems to depend on windows that are open.

Yeah there is some lag when changing shortcuts, its because of the File Watching mechanism I believe. It is present from before this PR.
I don't know about this one to be honest. If its a Xaml Island issue, maybe its fixed by the WPF branch?
I'll look into this one, maybe its fixable by changing how setting the Run window to foreground works when the shortcut is pressed or something.

traies · 2020-06-02T18:53:08Z

LGTM. Verified that it works as expected. Had a small question regarding the expected behavior when the hotkeys are held pressed, is it taken as one key press or multiple? Right now it seems to be taking it as multiple key presses. If that is the expected behavior it LGTM.

I don't think that behavior is intended, I'll fix that

htcfreek · 2020-06-03T18:29:11Z

Hi @arjunbalgovind
I'm going to take a look at how FZ editor hotkey is effected, thanks for the heads up.

@enricogior
As I reported in issue #3154 I am not able to enter Win+ or esc shortcuts in FZ editor setting's hotkey input. This is on all hotkey inputs in settings except KBM.

arjunbalgovind · 2020-06-03T19:12:06Z

Hi @arjunbalgovind
I'm going to take a look at how FZ editor hotkey is effected, thanks for the heads up.

@enricogior
As I reported in issue #3154 I am not able to enter Win+ or esc shortcuts in FZ editor setting's hotkey input. This is on all hotkey inputs in settings except KBM.

@htcfreek, this PR will fix the Win+ shortcuts case allowing you to enter it, but FZ currently does not support using existing windows shortcuts. We currently do not support a single key (like Esc) for the hotkey input, but we might add that in the future. Agreed that we should put a better error message than just displaying "Undefined". The changes from this PR will be included in 0.19.

arjunbalgovind

Tested locally. For some reason when I open Run the first time with Alt+Space the text box doesn't have focus so I need to click inside it. It could be from the stealing focus logic, and that doesn't seem to fix the start menu interaction either, so I think the stealing focus logic can be reverted and we'll work on the start menu interaction thing separately since it wasn't directly related to your PR.

crutkas · 2020-06-05T22:25:39Z

@traies can we get this merged in? lotta tweaks and i know you're getting ramped off

traies · 2020-06-08T07:09:02Z

I removed the commit that tried to fix the focus issues that @arjunbalgovind mentioned because they weren't fixing the problem. I also rebased to master and checked it is building the installer properly now.

As for the focus issues, I think it would probably be best if it is spun out to another PR. As it currently stands for this PR, trying to hit the shortcut to bring up the Launcher prompt while having the start menu open causes the focus to be kept by the start menu. This only happens when launching the solution from runner, and not when launching the PowerLauncher executable directly, for some reason.

dsrivastavv · 2020-06-08T15:41:28Z

@traies Can you please check if you are facing this issue after #4113 as well?

traies · 2020-06-08T18:47:28Z

@traies Can you please check if you are facing this issue after #4113 as well?

It is still there after #4113, unfortunately.

arjunbalgovind · 2020-06-08T20:09:17Z

@somil55 shall we merge this in? There is already an issue to track the start menu focus problem so we could reprioritize that. Users earlier observed it when they used KBM to remap Win+R to Alt+Space and open Launcher, and it happens now with these low level hook changes as well.

crutkas · 2020-06-09T22:07:54Z

lets get this merged in here,

dsrivastavv · 2020-06-10T00:10:09Z

@arjunbalgovind Sure!

arjunbalgovind · 2020-06-11T00:30:34Z

@ryanbodrug-microsoft can we merge this in? These are the things that we need to check after fixing:

[PowerToys Run / KBM; BUG] PT Run opens behind Start menu #3469 - was reported earlier when users used KBM shortcut remaps to open PT Run, and that issue exists with this as well since we moved to LL hooks
We need to validate that the default Alt+Space hotkey override does not take place on OS version less than 19H1, because PowerLauncher should not be working in that case.

crutkas · 2020-06-11T01:15:43Z

We need to validate that the default Alt+Space hotkey override does not take place on OS version less than 19H1, because PowerLauncher should not be working in that case.

In reality now that we removed islands, It should work much lower. The issue then becomes the dual settings work which it pretty costly from a resource stance.

traies requested review from alekhyareddy28 and a team May 28, 2020 16:10

alekhyareddy28 previously approved these changes Jun 1, 2020

View reviewed changes

alekhyareddy28 requested a review from arjunbalgovind June 1, 2020 17:03

arjunbalgovind approved these changes Jun 1, 2020

View reviewed changes

crutkas mentioned this pull request Jun 2, 2020

[PowerToys Run] Add an option to override classic Run dialog #3986

Closed

traies force-pushed the dev/traies/low-level-hook-interop branch from 5ccfceb to e5c330a Compare June 3, 2020 06:15

traies requested review from arjunbalgovind and alekhyareddy28 June 3, 2020 06:34

traies force-pushed the dev/traies/low-level-hook-interop branch from d56ad08 to 3b5e07e Compare June 3, 2020 07:17

arjunbalgovind approved these changes Jun 3, 2020

View reviewed changes

crutkas mentioned this pull request Jun 5, 2020

Are we able to set PowerToys Run Key to Windows key? (Instead of opening the start menu) #4075

Closed

traies and others added 3 commits June 7, 2020 23:17

[Launcher/Settings] Low Level Keyboard Hooks

385778e

[Run] LowLevel Keyboard Hook for Hotkeys

f8f676e

Prevent shortcuts from auto repeating when keeping the keys pressed down

bf89bbe

traies force-pushed the dev/traies/low-level-hook-interop branch from 3b5e07e to bf89bbe Compare June 8, 2020 07:01

ryanbodrug-microsoft added this to the InVEST-2006 milestone Jun 10, 2020

ryanbodrug-microsoft self-requested a review June 11, 2020 19:58

ryanbodrug-microsoft approved these changes Jun 11, 2020

View reviewed changes

arjunbalgovind merged commit 670033c into master Jun 11, 2020

arjunbalgovind mentioned this pull request Jun 11, 2020

[PowerToys Run (on master)] Search box does not appear in focus if you Alt+Space after interacting with anything else #4270

Closed

crutkas deleted the dev/traies/low-level-hook-interop branch June 25, 2020 21:00

arjunbalgovind mentioned this pull request Jul 2, 2020

WinKey for launcher throws a lot of false positives #4578

Closed

bergamin mentioned this pull request Apr 21, 2021

Input Lag Due to Keyboard Manager #4291

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Settings/Run] LowLevel Keyboard hooking for Hotkeys #3825

[Settings/Run] LowLevel Keyboard hooking for Hotkeys #3825

traies commented May 28, 2020

alekhyareddy28 left a comment

alekhyareddy28 Jun 1, 2020

traies Jun 2, 2020

arjunbalgovind commented Jun 1, 2020

arjunbalgovind commented Jun 1, 2020

alekhyareddy28 commented Jun 1, 2020 •

edited

Loading

enricogior commented Jun 1, 2020

enricogior commented Jun 1, 2020

arjunbalgovind left a comment

arjunbalgovind Jun 1, 2020

arjunbalgovind Jun 1, 2020

traies Jun 2, 2020

arjunbalgovind commented Jun 1, 2020 •

edited

Loading

enricogior commented Jun 1, 2020

traies commented Jun 2, 2020

traies commented Jun 2, 2020

htcfreek commented Jun 3, 2020

arjunbalgovind commented Jun 3, 2020 •

edited

Loading

arjunbalgovind left a comment

crutkas commented Jun 5, 2020

traies commented Jun 8, 2020

dsrivastavv commented Jun 8, 2020

traies commented Jun 8, 2020

arjunbalgovind commented Jun 8, 2020

crutkas commented Jun 9, 2020

dsrivastavv commented Jun 10, 2020

arjunbalgovind commented Jun 11, 2020

crutkas commented Jun 11, 2020

[Settings/Run] LowLevel Keyboard hooking for Hotkeys #3825

[Settings/Run] LowLevel Keyboard hooking for Hotkeys #3825

Conversation

traies commented May 28, 2020

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

alekhyareddy28 left a comment

Choose a reason for hiding this comment

alekhyareddy28 Jun 1, 2020

Choose a reason for hiding this comment

traies Jun 2, 2020

Choose a reason for hiding this comment

arjunbalgovind commented Jun 1, 2020

arjunbalgovind commented Jun 1, 2020

alekhyareddy28 commented Jun 1, 2020 • edited Loading

enricogior commented Jun 1, 2020

enricogior commented Jun 1, 2020

arjunbalgovind left a comment

Choose a reason for hiding this comment

arjunbalgovind Jun 1, 2020

Choose a reason for hiding this comment

arjunbalgovind Jun 1, 2020

Choose a reason for hiding this comment

traies Jun 2, 2020

Choose a reason for hiding this comment

arjunbalgovind commented Jun 1, 2020 • edited Loading

enricogior commented Jun 1, 2020

traies commented Jun 2, 2020

traies commented Jun 2, 2020

htcfreek commented Jun 3, 2020

arjunbalgovind commented Jun 3, 2020 • edited Loading

arjunbalgovind left a comment

Choose a reason for hiding this comment

crutkas commented Jun 5, 2020

traies commented Jun 8, 2020

dsrivastavv commented Jun 8, 2020

traies commented Jun 8, 2020

arjunbalgovind commented Jun 8, 2020

crutkas commented Jun 9, 2020

dsrivastavv commented Jun 10, 2020

arjunbalgovind commented Jun 11, 2020

crutkas commented Jun 11, 2020

alekhyareddy28 commented Jun 1, 2020 •

edited

Loading

arjunbalgovind commented Jun 1, 2020 •

edited

Loading

arjunbalgovind commented Jun 3, 2020 •

edited

Loading