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

Fix lags for UWP application in full screen mode #12860

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

mykhailopylyp
Copy link
Contributor

Summary of the Pull Request

What is this about:
Remove retries from get_process_path as this function is used in different places and should be generic. For example, when used in low-level keyboard hook it is unacceptable to block execution for 1 second(it happens for full screen UWP applications).

When alt+tab is pressed in full-screen UWP application we receive WM_PRIV_WINDOWCREATED message and FancyZones::WndProc is blocked for one second. Take into account, that Move newly created windows to their last known zone should be checked. So basically we have cases when EnumChildWindows doesn't find a different pid and retry doesn't make sense in this case.

What is include in the PR:

How does someone test / validate:
Test for KBM

  • Add remapping Ctrl(Left) + Q -> Play/Pause Media for video.ui.exe(Movies & TV)
    image
  • Verify that remapping works when Movies & TV is in full screen mode

Test for fancy zones

  • Enable Fancy zones
  • Check Move newly created windows to their last known zone
  • Open Movies & TV in full-screen mode
  • Press Alt+Tab
  • Verify absence of lag

@SeraphimaZ
Any idea how we can keep #12284 changes?

Quality Checklist

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA.

@mykhailopylyp mykhailopylyp added Product-Keyboard Shortcut Manager Issues regarding Keyboard Shortcut Manager Product-FancyZones Refers to the FancyZones PowerToy labels Aug 24, 2021
Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

If this is used on places where we might need to do retries and other places where we might not need to do retries, why not make retryAttempts an argument for the function?
It could be called with 10 from fancyzones and 1 from everywhere else.

@jaimecbernardo
Copy link
Collaborator

Just make sure you don't sleep at the end if this is the last retry, also.

@mykhailopylyp
Copy link
Contributor Author

If this is used on places where we might need to do retries and other places where we might not need to do retries, why not make retryAttempts an argument for the function?
It could be called with 10 from fancyzones and 1 from everywhere else.

We also need to call it with one retry from fancyzones because FancyZones::WndProc is blocked for 1 second on WM_PRIV_WINDOWCREATED message and as a result, we get the lag when switching(Alt+Tab) from a full-screen UWP app.

@SeraphimaZ
Can you list calls to get_process_path that should be retried in order to keep #12284?

@SeraphimaZykova
Copy link
Collaborator

@mykhailopylyp those calls should be retried.

auto process_path = get_process_path(window);

return IsZonableByProcessPath(get_process_path(window), excludedApps);

auto processPath = get_process_path(window);

@mykhailopylyp
Copy link
Contributor Author

@SeraphimaZ
If we add retry to the following call

auto processPath = get_process_path(window);

we still would have the lag on switching(Alt+Tab) from full-screen uwp applications.

What is weird is that the following code is called on switching(Alt+Tab) from full-screen uwp applications

else if (message == WM_PRIV_WINDOWCREATED)
{
auto hwnd = reinterpret_cast<HWND>(wparam);
WindowCreated(hwnd);
}

Probably we should add some checks here.

@mykhailopylyp
Copy link
Contributor Author

@SeraphimaZ
Can you reproduce EnumChildWindows fails on the first call? Can you share the steps to reproduce?

@SeraphimaZykova
Copy link
Collaborator

@SeraphimaZ
Can you reproduce EnumChildWindows fails on the first call? Can you share the steps to reproduce?

I tested it with the Windows Settings app when I worked on #12284, only the second or third attempt was successful. The window was opening and after a short time it was moved to a zone. However, now it succeeds on the first attempt, I can't reproduce the same behavior.

@mykhailopylyp
Copy link
Contributor Author

@SeraphimaZ
How important this retry is? Do we know how much users are impacted by it?

IMO, even if sometimes uwp window is not moved to a last know fancy zone, it's not a big deal. On the other hand, lag for alt+tab or not usable keyboard in full-screen uwp apps should be fixed in this release.
Also, it doesn't worth adding hackish retry solutions. Honestly, I don't know how to do it now.
Furthermore, we haven't completely solved 'move newly created window to the last known zone' issue. #9590 (comment)

@SeraphimaZ, @jaimecbernardo, thoughts?

@SeraphimaZykova
Copy link
Collaborator

@mykhailopylyp I agree that the lag is more important to fix. I think we could try to find another solution for moving uwp window to the last zone.

@jaimecbernardo
Copy link
Collaborator

Fixing the lag is more important. While 'move newly created window to the last known zone' failing is an annoyance, causing delays in other applications, especially making some unusable, such as games, is worse.

Let's get this in then.

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM! Great work.

@mykhailopylyp mykhailopylyp merged commit 0f55256 into microsoft:master Aug 25, 2021
@mykhailopylyp mykhailopylyp deleted the issue-12639 branch August 25, 2021 15:21
sorgloomer added a commit to sorgloomer/PowerToys that referenced this pull request Sep 2, 2021
The retry loop has been removed as of microsoft#12860 , but the comment stayed.
@sorgloomer sorgloomer mentioned this pull request Sep 2, 2021
10 tasks
jaimecbernardo pushed a commit that referenced this pull request Sep 2, 2021
The retry loop has been removed as of #12860 , but the comment stayed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-FancyZones Refers to the FancyZones PowerToy Product-Keyboard Shortcut Manager Issues regarding Keyboard Shortcut Manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants