Skip to content

Conversation

ferrariofilippo
Copy link
Contributor

Resolved / Related Issues
Items resolved / related issues by this PR.

Comments

  • The root of the problem was that, after the properties windows was opened, the focus returned to the main window because of the closing of the flyout context menu. I fixed the bug opening Properties after the menu gets closed
  • If everything is ok, I'll proceed with code cleanup

Validation
How did you test these changes?

  • Built and ran the app

@gave92
Copy link
Member

gave92 commented Sep 11, 2022

To be noted that there is also a simpler alternative:

// Workaround for #9926
propertiesWindow.IsAlwaysOnTop = true;
propertiesWindow.DispatcherQueue.CreateTimer().Debounce(() =>
{
    if (propertiesWindow.Visible)
    {
        propertiesWindow.Activate();
        propertiesWindow.IsAlwaysOnTop = false;
    }
}, TimeSpan.FromSeconds(1));

Adding the above in OpenPropertiesWindowAsync() (@ line 82) appears to be enough to fix the issue. It's a bit hacky though.

@ferrariofilippo
Copy link
Contributor Author

To be noted that there is also a simpler alternative:

// Workaround for #9926
propertiesWindow.IsAlwaysOnTop = true;
propertiesWindow.DispatcherQueue.CreateTimer().Debounce(() =>
{
    propertiesWindow.Activate();
    propertiesWindow.IsAlwaysOnTop = false;
}, TimeSpan.FromSeconds(1));

Adding the above in OpenPropertiesWindowAsync() (@ line 82) appears to be enough to fix the issue. It's a bit hacky though.

I took into account a solution like this, but there are two drawbacks: if, for any reason, the flyout context menu takes more than a second to close, the bug would remain unfixed (it would be a one-in-a-million case). The second problem is that the program might seem not optimized or inefficient because of that delay. I know it's only a second, but I'm obsessed with optimization.

@gave92
Copy link
Member

gave92 commented Sep 11, 2022

For "optimization" the simple solution is actually better since you don't wait for the menu to close before opening the properties window. But I agree it might not cover all cases.
Edit: also it has an issue where keyboard navigation in properties window starts working after 1 second.

@yaira2
Copy link
Member

yaira2 commented Sep 12, 2022

Using the hwnd, can we force the new window to the foreground?

@gave92
Copy link
Member

gave92 commented Sep 12, 2022

With my solution I set it always on top for 1 second. It appears in front but it's not active. Not sure if we can force it to be active.

@ferrariofilippo
Copy link
Contributor Author

Using the hwnd, can we force the new window to the foreground?

You can't really do that the way we'd want to.
Setting IsAlwaysOnTop to true means that the Properties Window can't be hidden by any other window (I don't think we want this).
Forcing the Prop. Window to be shown above others (using BringToFront() or MoveInZOrderToTop()) would be useless, because, as the context menu closes, the main window will be brought on top of others.
Mine and @gave92 solutions are the only viable.

yaira2
yaira2 previously approved these changes Sep 12, 2022
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Sep 12, 2022
@yaira2 yaira2 merged commit 8a8c697 into files-community:main Sep 12, 2022
@yaira2
Copy link
Member

yaira2 commented Sep 12, 2022

@ferrariofilippo does this work for the drives widget as well?

@ferrariofilippo
Copy link
Contributor Author

@ferrariofilippo does this work for the drives widget as well?

Yes, it does

@gave92
Copy link
Member

gave92 commented Sep 12, 2022

@ferrariofilippo solution looks good to me. I just find it annoying that it has to be done in each place we have a context menu and each has slightly different code, but no biggie.

@yaira2
Copy link
Member

yaira2 commented Sep 12, 2022

Likewise but this is still a good solution

gave92 added a commit to gave92/files-uwp that referenced this pull request Sep 12, 2022
gave92 added a commit to gave92/files-uwp that referenced this pull request Sep 12, 2022
@ferrariofilippo ferrariofilippo deleted the BugFix_Properties_Open_Behind_Window_#9926 branch September 12, 2022 19:32
yaira2 pushed a commit that referenced this pull request Sep 12, 2022
Remove temp variable for code of #9973
ferrariofilippo added a commit to ferrariofilippo/Files that referenced this pull request Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Properties window opens behind main app window
3 participants