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

[launcher] Location and multi monitor support #2446

Merged

Conversation

dsrivastavv
Copy link
Contributor

@dsrivastavv dsrivastavv commented Apr 28, 2020

Summary of the Pull Request

Added code to position launcher in upper center of the screen and handle display in a multi monitor setting.

PR Checklist

Validation Steps Performed

  1. Manually validated that launcher appears in the center when it is visible.
  2. Verified that position is updated when DPI is changed from system settings.
  3. Validated that Launcher is correctly shown/hidden when monitor is changed.

@@ -144,6 +146,15 @@ private double WindowLeft()
return left;
}

private double WindowTop()
Copy link

Choose a reason for hiding this comment

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

sorry for commenting out of context, does WindowTop() get the window position, and calculates the Y coordinate using dip1 and dip2? Some comments on the function would be helpful to facilitate a walkthrough of the function :) I will run your branch on my side to test that it launches on the top center of the screen :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laviusmotileng-ms Sure! Thanks for suggestion.

@dsrivastavv dsrivastavv requested a review from crutkas April 29, 2020 17:04
@dsrivastavv dsrivastavv added the Product-PowerToys Run Improved app launch PT Run (Win+R) Window label Apr 29, 2020
@dsrivastavv dsrivastavv changed the title Somil55/launcher location [launcher] Location and multi monitor support Apr 29, 2020
@ryanbodrug-microsoft
Copy link
Contributor

@somil55 Can we put this in dev/build-feature. This branch should only be used for any last minute demo updates.

@ryanbodrug-microsoft
Copy link
Contributor

This looks great on my primary monitor (laptop), but on my other monitor it looks like this:

image

@ryanbodrug-microsoft
Copy link
Contributor

@somil55 , but on the plus side its not moving ;)

@dsrivastavv
Copy link
Contributor Author

This looks great on my primary monitor (laptop), but on my other monitor it looks like this:

image

@ryanbodrug-microsoft Is it the case with every launch or just the first launch on your second monitor ?

This reverts commit 38f3992.
@alekhyareddy28
Copy link
Contributor

  • It appears as follows on my secondary monitor (on first launch on that monitor) -
    image

  • If I launch it for the second time, on the same monitor it positions itself fine and also centers itself. However, when I move to the primary monitor and come back to teh secondary one, I face the same issue as stated above.
    image

  • On my primary monitor, it is a little more to the left and not centered as follows (Not sure if we want that). Launching it multiple times does not change it's position.
    image

@ryanbodrug-microsoft
Copy link
Contributor

@somil55 This looks better. A couple things I've noticied.

  1. Any time I switch monitors, the first open loads offcenter (This might have been a preexisting issue).
  2. This PR should be merged in dev/build-features.
  3. If I move the window then it doesn't remember the position that i've moved it to.

I think items 1 & 3 can be captured in seperate issues and triaged. Can you just verify that the first issue wasn't introduced in this PR?

@dsrivastavv
Copy link
Contributor Author

@somil55 This looks better. A couple things I've noticied.

  1. Any time I switch monitors, the first open loads offcenter (This might have been a preexisting issue).
  2. This PR should be merged in dev/build-features.
  3. If I move the window then it doesn't remember the position that i've moved it to.

I think items 1 & 3 can be captured in seperate issues and triaged. Can you just verify that the first issue wasn't introduced in this PR?

@ryanbodrug-microsoft I was waiting for changes from dev/PowerLauncher to be integrated into build-features branch. I will update this PR. Regrading 3, I haven't worked on it in this PR. I will create a issue for this. And can you provide some more context on 1 ? I wasn't noticing this issue or I might be misunderstanding this.

@dsrivastavv dsrivastavv changed the base branch from dev/PowerLauncher to dev/build-features May 1, 2020 00:10
@alekhyareddy28
Copy link
Contributor

alekhyareddy28 commented May 1, 2020

@somil55, I see a stutter on first launch on any monitor which doesn't happen from the second time onwards. The border is rendered first. Other than that the changes LGTM. That might have been there before as well.
stutter_first_launch

@dsrivastavv
Copy link
Contributor Author

dsrivastavv commented May 1, 2020

@alekhyareddy28 There is some lag when the XAML host components are resized. This issue is evident when you show/hide searchbox. I will file an issue for this UI behaviour.

@dsrivastavv
Copy link
Contributor Author

@ryanbodrug-microsoft Regarding comment #1 I verified that it is a preexisting issue on current dev/build-features branch.

@crutkas
Copy link
Member

crutkas commented May 1, 2020

i think this has a subset of issues that this doesn't always solve. All dealing with DPI. My suggestion i think we should close this PR, create POC that mimics without islands, and make sure it is DPI aware.

this will allow us to then rapidly iterate

Copy link
Contributor

@ryanbodrug-microsoft ryanbodrug-microsoft left a comment

Choose a reason for hiding this comment

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

This isn't quite perfect yet, but is a definite improvement. Lets get this PR in and make sure there are tasks for.

  1. Switching monitors, and opening brings the launcher up off center.
  2. Loading the xaml island on resize causes a flicker.
  3. When the user manually positions the launcher it doesn't rememeber the position on consecutive launches.

@dsrivastavv dsrivastavv merged commit f44109a into microsoft:dev/build-features May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-PowerToys Run Improved app launch PT Run (Win+R) Window
Projects
None yet
4 participants