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

Enable setting an initial position and maximization launch for Terminal #2817

Conversation

KaiyuWang16
Copy link
Contributor

@KaiyuWang16 KaiyuWang16 commented Sep 19, 2019

This PR is for code changes as well as a spec for #1043

No

PR Checklist

  • Closes Be able to set an initial position for the terminal #1043
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

This PR includes the code changes that enable users to set an initial position (top left corner) and launch maximized. There are some corner cases:

  1. Multiple monitors. The user should be able to set the initial position to any monitors attached. For the monitors on the left side of the major monitor, the initial position values are negative.

  2. If the initial position is larger than the screen resolution and the window is off-screen, the current solution is to check if the top left corner of the window intersect with any monitors. If it is not, we set the initial position to the top left corner of the nearest monitor.

  3. If the user wants to launch maximized and provides an initial position, we launch the maximized window on the monitor where the position is located.

Testing:

To test:

  1. Check-out this branch and build on VS2019
  2. Launch Terminal, and open Settings. Then close the terminal.
  3. Add the following setting into Json settings file as part of "globals", just after "initialRows":
    "initialPosition": "1000, 1000",
    "launchMode": "default"

My test data:
I have already tested with the following variables:
1. showTabsInTitlebar true or false
2. The initial position of the top left corner of the window
3. Whether to launch maximized
4. The DPI of the monitor

Test data combination:

Non-client island window (showTabsInTitlebar true)

  1. Three monitors with the same DPI (100%), left, middle and right, with the middle one as the primary, resolution: 1980 * 1200, 1920 * 1200, 1920 * 1080
    launchMode: default
    In-Screen test: (0, 0), (1000, 500), (2000, 300), (-1000, 400), (-100, 200), (-2000, 100),
    (0, 1119)
    out-of-screen:
    (200, -200): initialize to (0, 0)
    (200, 1500): initialize to (0, 0)
    (2000, -200): initialize to (1920, 0)
    (2500, 2000): initialize to (1920, 0)
    (4000 100): initialize to (1920, 0)
    (-1000, -100): initialize to (-1920, 0)
    (-3000, 100): initialize to (-1920, 0)
    (10000, -10000): initialize to (1920, 0)
    (-10000, 10000): initialize to (-1920, 0)
    (0, -10000): initialize to (0, 0)
    (0, -1): initialize to (0, 0)
    (0, 1200): initialize to (0, 0)
    launch mode: maximize
    (100, 100)
    (-1000, 100): On the left monitor
    (0, -2000): On the primary monitor
    (10000, 10000): On the primary monitor

  2. Left monitor 200% DPI, primary monitor 100% DPI
    In screen: (-1900, 100), (-3000, 100), (-1000, 100)
    our-of-screen: (-8000, 100): initialize at (-1920, 0)
    launch Maximized: (-100, 100): launch maximized on the left monitor correctly

  3. Left monitor 100% DPI, primary monitor 200% DPI
    In-screen: (-1900, 100), (300, 100), (-800, 100), (-200, 100)
    out-of-screen: (-3000, 100): initialize at (-1920, 0)
    launch maximized: (100, 100), (-1000, 100)

For client island window, the test data is the same as above.

Issues:

  1. If we set the initial position on the monitor with a different DPI as the primary monitor, and the window "lays" across two monitors, then the window still renders as it is on the primary monitor. The size of the window is correct.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 25, 2019
@miniksa
Copy link
Member

miniksa commented Sep 26, 2019

  1. For client island window, if we set the initial position to the top left corner of a monitor, the left edge of the window does not coincide with the left edge of the screen. We may need to adjust the size of the window.

Is it by chance off by 8 pixels from the left on a 100% DPI monitor? If so, I'm pretty sure there's a method or window metric hanging around somewhere in the system that will give you that figure of the "invisible window border" so you can offset your position. I know that we talked through this once in the past because conhost.exe set to 0,0 is offset slightly from the left. I don't think we mitigated it, but it is definitely possible to lookup the window metric. If this is the situation, let me know and I'll try to find my notes on the right way to do it.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 26, 2019
@KaiyuWang16
Copy link
Contributor Author

For client island window, if we set the initial position to the top left corner of a monitor, the left edge of the window does not coincide with the left edge of the screen. We may need to adjust the size of the window.

Is it by chance off by 8 pixels from the left on a 100% DPI monitor? If so, I'm pretty sure there's a method or window metric hanging around somewhere in the system that will give you that figure of the "invisible window border" so you can offset your position. I know that we talked through this once in the past because conhost.exe set to 0,0 is offset slightly from the left. I don't think we mitigated it, but it is definitely possible to lookup the window metric. If this is the situation, let me know and I'll try to find my notes on the right way to do it.

Yes, if I move the window left for 8 pixies, if will match the left edge of the screen. Griese also told me that there is a 8 pixies border on win32 windows, but win10 windows only have 1 pixel, and this introduces an invisible border around the Terminal window.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 27, 2019
@KaiyuWang16 KaiyuWang16 requested review from a team and removed request for a team September 30, 2019 22:15
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

I'm generally fine with this, but I'd like to see @DHowett-MSFT sign off as well here before merging.

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

This looks good! Thank you! Just a couple more small comments.

@DHowett-MSFT
Copy link
Contributor

I will merge with the following message when the builds complete.

This PR includes the code changes that enable users to set an initial position
(top left corner) and launch maximized. There are some corner cases:

1. Multiple monitors. The user should be able to set the initial position to
any monitors attached. For the monitors on the left side of the major monitor,
the initial position values are negative.

2. If the initial position is larger than the screen resolution and the window
is off-screen, the current solution is to check if the top left corner of the
window intersect with any monitors. If it is not, we set the initial position
to the top left corner of the nearest monitor.

3. If the user wants to launch maximized and provides an initial position, we
launch the maximized window on the monitor where the position is located.

# Testing

To test:
1. Check-out this branch and build on VS2019
2. Launch Terminal, and open Settings. Then close the terminal.
3. Add the following setting into Json settings file as part of "globals", just
after "initialRows":
  "initialPosition": "1000, 1000",
  "launchMode": "default"

My test data:
I have already tested with the following variables:
  1. showTabsInTitlebar true or false
  2. The initial position of the top left corner of the window
  3. Whether to launch maximized
  4. The DPI of the monitor

Test data combination:

Non-client island window (showTabsInTitlebar true)

1. Three monitors with the same DPI (100%), left, middle and right, with the
middle one as the primary, resolution: 1980 * 1200, 1920 * 1200, 1920 * 1080
    launchMode: default
      In-Screen test: (0, 0), (1000, 500), (2000, 300), (-1000, 400),
        (-100, 200), (-2000, 100), (0, 1119)
      out-of-screen:
        (200, -200): initialize to (0, 0)
        (200, 1500): initialize to (0, 0)
        (2000, -200): initialize to (1920, 0)
        (2500, 2000): initialize to (1920, 0)
        (4000 100): initialize to (1920, 0)
        (-1000, -100): initialize to (-1920, 0)
        (-3000, 100): initialize to (-1920, 0)
        (10000, -10000): initialize to (1920, 0)
        (-10000, 10000): initialize to (-1920, 0)
        (0, -10000): initialize to (0, 0)
        (0, -1):  initialize to (0, 0)
        (0, 1200):  initialize to (0, 0)
    launch mode: maximize
        (100, 100)
        (-1000, 100): On the left monitor
        (0, -2000): On the primary monitor
        (10000, 10000): On the primary monitor


2. Left monitor 200% DPI, primary monitor 100% DPI
    In screen: (-1900, 100), (-3000, 100), (-1000, 100)
    our-of-screen: (-8000, 100): initialize at (-1920, 0)
    launch Maximized:  (-100, 100): launch maximized on the left monitor
      correctly

3. Left monitor 100% DPI, primary monitor 200% DPI
    In-screen: (-1900, 100), (300, 100), (-800, 100), (-200, 100)
    out-of-screen: (-3000, 100): initialize at (-1920, 0)
    launch maximized: (100, 100), (-1000, 100)

For client island window, the test data is the same as above.

Issues:

1. If we set the initial position on the monitor with a different DPI as the
primary monitor, and the window "lays" across two monitors, then the window
still renders as it is on the primary monitor. The size of the window is
correct.

Closes #1043

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

Waiting on builds.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 17, 2019
@DHowett-MSFT DHowett-MSFT merged commit 35d7d20 into master Oct 17, 2019
@DHowett-MSFT DHowett-MSFT deleted the dev/kawa/1043-set-initial-position-for-the-terminal-spec-and-implementation branch October 17, 2019 04:51
@ghost
Copy link

ghost commented Oct 23, 2019

🎉Windows Terminal Preview v0.6.2951.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Be able to set an initial position for the terminal
6 participants