Key shortcuts quirky when elevated (Implement a centralized shortcut key handler)

[enricogior]

The centralized shortcut key handler will allow modules that don't run elevated to be invoked when the foreground window is elevated. It will require the runner to run elevated.

[ivan100sic]

@martinchrzan

In order to implement this feature, we need to refactor existing code so that ColorPicker is shown immediately after being launched. Can this be done using AppStateHandler and how?

[martinchrzan]

@ivan100sic I don't really know what you mean by showing immediately after being launched, but you probably want to change the code in KeyboardMonitor.cs - Hook_KeyboardPressed - here you can see what happens when you press activation key. I assume you want to replace local low keyboard hook with your logic, this would be the place to do so.
How do you pass the info about activation key being presses? Do you open named pipe or do you want to always start a new process and then kill it when closed?

[ivan100sic]

We want to change ColorPicker so that:

• When ColorPicker is enabled in the settings, ColorPicker.exe does not start until its hotkey is pressed.
• When the hotkey is pressed, ColorPicker.exe starts.
• ColorPicker exits instead of hiding.
• ColorPicker does not run its own KB hook
• The Runner has a KB hook and asks each module for its hotkey.
• When the hotkey is pressed, a method invoke() is called in the DLL.
• Modules do not need to provide a hotkey, in that case, invoke() won't be called for them.

[martinchrzan]

I don't like this approach due to several issues with that:

• Starting a new process every time you activate will cause poor performance - it will not be super quick to show up
• Modules might need to react to more than one hotkey, if we add more of them (I might have 2 hotkeys to start 2 different modes of the module), how will that be handled?
• I need to close ColorPicker and Zoom Window when user press Esc - how would that be handled?

Also dependency should be reversed - each module should register itself in some keyboard manager and let it know that it want to react on some hotkey. Parent (in this case The runner) should not know about actual modules that are using its keyboard manager.

[enricogior]

@martinchrzan
do we have actual data for the poor performance? Keeping all modules in memory is also not ideal, if a module can startup at a decent speed we should consider to not keep it running in memory.

Initially we are not going to support arguments when starting the app, but for sure we can add a startup argument.

I need to close ColorPicker and Zoom Window when user press Esc - how would that be handled?

Not sure what do you mean, is it a problem of not receiving the Esc event or about how to proceed to quit the .exe?

[enricogior]

Parent (in this case The runner) should not know about actual modules that are using its keyboard manager.

What are the reasons for not implementing the keyboard manager in the runner?

[martinchrzan]

I have nothing against implementing it in runner, I was just saying that modules should register themselves (think of dependency injection) rather than runner knowing explicitly about all modules that are interested in key hooks. This logic should be in runner.

I do not have any hard data about performance, just doing some tests:

• if I launch it and show it immediately, it is a bit slower (but noticeable) than if it was already running - this can be further slowed significantly if settings.json is in use - as I do retry to read it, might take several seconds and we have seen this locked issues quite often (and this is the same story for Launcher)

• we will see windows loading cursor when starting a new process

• there is an issue with different DPIs and multiple monitors - sometimes color picker does not show right next to the mouse because somehow proper DPI is propagated into the app, only once it is shown for the first time (this issue does not occur all the time), however if you are just showing/hidding it, you will minimize this issue only for the first time it is shown, next time it always works.

• I believe this change should be as generic as possible - I don't know about other modules, but I can imagine that some of them might do some extra stuff on the first start - we might have modules that need to initialize something in the future - how will this support that?

• Regarding Esc - I want to hide/close the app on Esc, therefore I want to listen to global key event as for the activation to do that.

Now I noticed even a bigger issue - activation key handling is not enough for color picker! I am running a low level mouse hook and that one needs to run in admin mode as well - if it doesn't, you are not able to pick the color, because the left mouse key is not fired if clicking into an elevated app.

[enricogior]

@martinchrzan

I was just saying that modules should register themselves (think of dependency injection) rather than runner knowing explicitly about all modules that are interested in key hooks.

The original design made the runner the coordinator for all modules. The runner is responsible to load and start each module and coordinate with the settings app (modules should not invoke the Settings app directly).
Unless there is a technical reason for reverting that logic, I would prefer we keep that approach and not mix architectures for different features.

can imagine that some of them might do some extra stuff on the first start

PT Run is one of those modules and for it the plan is to not restart it every time.
We want to support both scenarios since not all the modules need to be running all the time and the initial understanding was that ColorPicker didn't, clearly it does (given the other problems, in particular the one related to DPI, I see why we should keep ColorPicker running).

With regard to the mouse hook, it seems also clear that ColorPicker needs to run elevated all the time, trying to use a centralized mouse hook is not an ideal solution because we don't want to keep the mouse hook running when it's not needed, so it would make more sense and be more flexible if it runs on demand inside the application itself (we do the same in FZ, we have a keyboard and a mouse hooks active only while a windows is dragged).

[rdorris]

PT Run not working with any hotkey I set. I've attached logs if it helps.
2020-09-02.txt
2020-09-03.txt

[enricogior]

@rdorris
this issue is for tracking a work item that is not implemented yet, please open a dedicated issue or search for existing issues for the PT Run non working.

[rdorris]

@enricogior
I did, and it was closed and I was asked to post it on this issue. (#6270)

[enricogior]

@rdorris
there was a misunderstanding, this issue is not for reporting any existing bug, but it will implement a new feature that will effect existing bugs.

[enricogior]

Fixed in 0.23.0 https://github.com/microsoft/PowerToys/releases/tag/v0.23.0