[AlwaysOnTop] Sound played when pin/unpin fail

davidegiacometti commented 2 years ago

Microsoft PowerToys version

0.59.0

Running as admin

[ ] Yes

Area(s) with issue?

Always on Top

Steps to reproduce

Sound is played if you try to pin/unpin desktop/taskbar/taskview/start but there is no window to pin/unpin.
If for some reason pin/unpin of a window fail, the sound is played the same (PT Running non elevated and try to pin an elevated window).

https://github.com/microsoft/PowerToys/blob/a11f9ab4c9852265c396a167b8d73fb3d516cedd/src/modules/alwaysontop/AlwaysOnTop/AlwaysOnTop.cpp#L162 Result of UnpinTopmostWindow and PinTopmostWindow is not checked.

✔️ Expected Behavior

Sound played

❌ Actual Behavior

Sound not played

Other Software

No response

crutkas commented 2 years ago

good idea

davidegiacometti commented 2 years ago

First is an easy fix. Second could be tricky: needs to figure out the best way to detect the foreground window is desktop/taskbar/taskview/start. Process name is not enough, maybe window title could help.

htcfreek commented 2 years ago

First is an easy fix. Second could be tricky: needs to figure out the best way to detect the foreground window is desktop/taskbar/taskview/start. Process name is not enough, maybe window title could help.

You can try to use the window class. I know that it is a special one for taskbar.

Window title will be a problem as it is translated.

Why is process not enough (except of taskbar and desktop)?

davidegiacometti commented 2 years ago

Nice catch @htcfreek Quick look at processes and classes using PT StyleReportTool on Windows 10 21H2:

Taskbar
Window: explorer.exe
Class: Shell_TrayWnd

Start and TaskView
Window: SearchApp.exe
Class: Windows.UI.Core.CoreWindow

Desktop
Window: explorer.exe
Class: Progman

htcfreek commented 2 years ago

Nice catch @htcfreek Quick look at processes and classes using PT StyleReportTool:
Taskbar
Window: explorer.exe
Class: Shell_TrayWnd

Start and TaskView
Window: SearchApp.exe
Class: Windows.UI.Core.CoreWindow

Desktop
Window: explorer.exe
Class: Progman

Windows.UI.Core.CoreWindow should not be used to block something. It's a generic winui class.

SearchApp is only search on win10. And start and message center on win10 should should be an other process. Taskbar on win10 is something like ShellExperienceHost.exe.

davidegiacometti commented 2 years ago

@crutkas I was looking into this and it will end in adding many exclusions.

Process	ClassName	Target
EXPLORER.EXE	Shell_TrayWnd	Taskbar
EXPLORER.EXE	WorkerW	Desktop
EXPLORER.EXE	Windows.UI.Core.CoreWindow	Task view
SEARCHAPP.EXE	Windows.UI.Core.CoreWindow	Start menu
STARTMENUEXPERIENCEHOST.EXE	Windows.UI.Core.CoreWindow	Start menu
SHELLEXPERIENCEHOST.EXE	Windows.UI.Core.CoreWindow	Action center and system tray flyouts
SEARCHHOST.EXE	Windows.UI.Core.CoreWindow	Windows 11 start menu
EXPLORER.EXE	XamlExplorerHostIslandWindow	Windows 11 task view

I don't expect them to change frequently but I don't like the idea to maintain this. I think desktop and taskbar should be handled since are causing https://github.com/microsoft/PowerToys/issues/19251

The problem with start menu, action center, and system flyouts (battery, volume, calendar) is that AOT try to unpin them since as flyouts they are already topmost and the result is that only the unpin sound is played.

AOT is able to remove topmost from Window even if not set by AOT. Is this intended or a bug? Not sure if we are able to detect and exclude flyouts: same behaviour is happening with OneDrive and 3rd party flyouts.

CC @SeraphimaZykova and sorry for the long message 😄

SeraphimaZykova commented 2 years ago

@davidegiacometti thank you for your investigation :) Removing topmost from non-AOT windows is a bug as well as playing unpinning sound, I'll take care of this.

microsoft / PowerToys