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

Windows snap hotkeys to move windows between screens #1603

Merged
merged 9 commits into from
Mar 24, 2020

Conversation

vldmr11080
Copy link
Contributor

Summary of the Pull Request

We currently support using WIN + ARROW key to move window through zones inside zone layout on one monitor. This pull requests improves that functionality, allowing movement to be performed in multi monitor scenario.

  1. Sort monitors based on their top-left coordinate.
  2. Once we reach last zone (or first, depending on direction) we switch to the layout on the different monitor and iterate through zones there.

PR Checklist

@enricogior enricogior requested review from PrzemyslawTusinski and removed request for yuyoyuppe March 17, 2020 11:00
@vldmr11080
Copy link
Contributor Author

Todo: Add new unit tests which are checking modified API behavior. I've just updated current ones so they don't crash.

@enricogior enricogior removed their request for review March 19, 2020 09:17
@enricogior
Copy link
Contributor

When making changes/adding unit tests, please add @SeraphimaZ as a reviewer.

@enricogior
Copy link
Contributor

enricogior commented Mar 19, 2020

With three monitors like this, where the monitor number 2 is the primary monitor

image

Win+right arrow moves the window from monitor 2 to monitor 1 and then to monitor 3.

These are the coordinates of the three monitors:
Monitor 1: (res 3840x2160) -719 -3840
Monitor 2: (res 3440x1440) 0 0
Monitor 3: (res 1920x1200) 241 3440

The sorting algorithm should be reversed, because it currently puts the monitor with the higher left coordinate in first position, so the current order is 3-2-1.
Also the case lhs.second.left == rhs.second.left is too specific, monitors can be one on top of the other but not aligned. So you have to always compare both X and Y and probably also the width and height to figure out the actual relative position when there are three or more monitors.
We need unit tests for this code.

@crutkas
Copy link
Member

crutkas commented Mar 19, 2020

Will this scenario work correctly? Aero snap does it correctly. if a windows is on 3, i hit Win+LA, it will go to Monitor one. Win+RA, it will move back to 3
image

@vldmr11080
Copy link
Contributor Author

Will this scenario work correctly? Aero snap does it correctly. if a windows is on 3, i hit Win+LA, it will go to Monitor one. Win+RA, it will move back to 3
image

This is rather basic scenario and it will work (just need to reverse my comparison function). I'm currently thinking of some more robust comparison function, for all kind of monitor number and positions.

@crutkas
Copy link
Member

crutkas commented Mar 19, 2020

This is rather basic scenario and it will work (just need to reverse my comparison function). I'm currently thinking of some more robust comparison function, for all kind of monitor number and positions.

This shouldn't be approved then until this acts like aero snap.

We need to know the L and R virtual monitors to send window to.

@enricogior
Copy link
Contributor

@crutkas
it will only be merged after it will work for scenarios up to 9 monitors and with unit tests to prove it.

@ivan100sic
Copy link
Contributor

@crutkas
it will only be merged after it will work for scenarios up to 9 monitors and with unit tests to prove it.

How are we going to test private methods?

@ivan100sic
Copy link
Contributor

Nevermind, I extracted the critical method (the one that should undergo tests) as a separate Util method, so we can test it. My code will take care of ordering monitors.

* Implemented improved monitor ordering v1

* Fixed some embarrassing bugs, added some tests

* Added one more test

* Extracted a value to a variable

* ASCII art in unit test comments describing monitor layouts

* Removed empty line for consistency

* Update comment to match the code

* Refactored tests, added tests for X,Y offsets
@enricogior
Copy link
Contributor

I tested with different 3 monitor layouts, and it behaves similarly to Windows Snap.
I would be better to test it with 4 or more monitors, but let's not wait and let's merge this.

@vldmr11080 vldmr11080 merged commit 9e8faca into microsoft:master Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants