Fix restore window position when exiting fullscreen by ekoschik · Pull Request #9737 · microsoft/terminal

ekoschik · 2021-04-07T03:49:58Z

Summary of the Pull Request

This change cleans up the Fullscreen implementation for both conhost and Terminal, improving the restore position (where the window goes when exiting fullscreen).

Prior to this change the window wasn't guaranteed to restore somewhere on the window's current monitor when exiting fullscreen. With this change the window will restore always to its current monitor, at a reasonable location (and will 'double restore' (to fullscreen->maximize->restore) after monitor changes while fullscreen, which is the expected user behavior.

References

PR Checklist

Closes Exiting fullscreen positions window to unexpected place if monitor changed #9746
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema 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

Detailed Description of the Pull Request / Additional comments

A fullscreen window's monitor can change.

Win+Shift+left/right migrates a window between monitors.
User could open settings, display, and move the monitor or change its DPI.
The monitor could be unplugged.
The session could be remote and be disconnected.

A fullscreen window stores a 'restore position' when entering fullscreen, used to move the window back 'where it was'. BUT, its unexpected for the window to exit fullscreen and jump to another monitor. This means its previous position must be migrated from the old monitor's work area to the new monitor's work area.

If a window is maximized, it is sized to the work area. Like with fullscreen, a maximized window has a 'restore position', though unlike with fullscreen the restore position for maximized is stored by the system itself. Migration in cases where a maximized (or fullscreen) window's monitor changes is also taken care of by the system. To restore 'safely' to maximized (after changing window styles) a window must only SetWindowPos(SWP_FRAMECHANGED). While technically a maximized window that becomes fullscreen 'is still maximized' (from Win32's perspective), its prudent to also ShowWindow(SW_MAXIMIZED) prior to SWP_FRAMECHANGED (to explicitly make the window maximized).

If not restoring to maximized, the restore position is adjusted by the new/ old work area. Additionally, the new/ old window DPI is used to adjust the size of the window by the DPI change (keeping the window's logical size the same).

The work area origin is checked first (shifting window rect by the change in origin)
The DPI is checked next, changing right/ bottom (size only)
Each edge of the window is compared against the corresponding edge of the work area, nudging the window back on-screen if hanging offscreen. By shifting right before left, bottom before top, the top-left is guaranteed on-screen.

Validation Steps Performed

Tried it out. Seemed to work on my machine.
Jk, ran conhost/ terminal on mixed DPI system, max (or not), fullscreen, win+shift+left/ exit fullscreen/ maximize. Monitor unplug, etc.

ghost · 2021-04-07T03:50:10Z

All CLA requirements met.

DHowett · 2021-04-07T14:10:13Z

@ekoschik If you link up your account at the OSS portal (repos.opensource.microsoft.com, link tab) you won't have to accept the CLA.

github-actions · 2021-04-07T20:17:35Z

Misspellings found, please review:

hotkeys
nmake

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"aef aspnet boostorg BSODs Cac COINIT dahall DEFAPP DEFCON fde fea fmtlib HPCON isocpp mintty msvcrtd NVDA pinam QOL remoting Unk unte vcrt what3words xamarin "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/ed1cd32f1fb0c681b05998efdee124d6c314a07d.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"cac coinit defapp hotkeys hpcon MSVCRTD nmake Remoting unk "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

github-actions · 2021-04-07T20:20:17Z

Misspellings found, please review:

hotkeys

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"aef aspnet boostorg BSODs Cac COINIT dahall DEFAPP DEFCON fde fea fmtlib HPCON isocpp mintty msvcrtd NVDA pinam QOL remoting Unk unte vcrt what3words xamarin "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/ed1cd32f1fb0c681b05998efdee124d6c314a07d.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"cac coinit defapp hotkeys hpcon MSVCRTD Remoting unk "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

DHowett · 2021-04-07T20:28:34Z

src/interactivity/win32/windowproc.cpp

-        if (IsInFullscreen())
-        {
-            // If we're a full screen window, completely ignore what the DPICHANGED says as it will be bigger than the monitor and
-            // instead just ensure that the window is still taking up the full screen.
-            SetIsFullscreen(true);
-        }
-        else


Will any of these changes regress our windowing downlevel from 21xxx? Or is this "more correct" everywhere?

Fair concern. The short answer is this change should work properly on all down-level releases.

Re the WM_DPICHANGED message, whenever this message is sent (from any release) anything other than calling SetWindowPos with the RECT from lParam simply isn't supported. If this was ever necessary, it was because of a bug that (I believe) has since been fixed.

I'd be happy to convince you further (either in the open or in a teams chat) :).

DHowett · 2021-04-12T16:16:32Z

(Sorry, we've been busy with the coming 1.8/1.7 pair of releases; We'll review this asap.)

zadjii-msft

Okay this all seems reasonable to me. It also makes the logic in those methods way easier to follow. Thanks!

zadjii-msft · 2021-04-13T11:36:54Z

@msftbot make sure either @DHowett or @miniksa signs off on this

ghost · 2021-04-13T11:36:59Z

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @DHowett or @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

miniksa

Looks good to me. Nothing like a user32k expert to fix up your window message handling, so I trust @ekoschik here.

Evan, if you're confused on fixing up the spell check bot that's blocking the merge, let me know.

miniksa · 2021-04-13T16:27:11Z

src/cascadia/WindowsTerminal/IslandWindow.cpp

+    // If the window DPI has changed, re-size the stored position by the change in DPI. This
+    // ensures the window restores to the same logical size (even if to a monitor with a different
+    // DPI/ scale factor).
+    UINT dpiWindow = GetDpiForWindow(hWnd);


Suggested change

UINT dpiWindow = GetDpiForWindow(hWnd);

UINT const dpiWindow = GetDpiForWindow(hWnd);

miniksa · 2021-04-13T16:33:46Z

WELP OK SPELL CHECK DOES NOT BLOCK MERGE. Guess I'm fixing it now.

DHowett

Signing off after the fact -- this is great. Thanks so much @ekoschik

ekoschik · 2021-04-13T21:04:42Z

Yayyy!!! Thanks so much, folks!

## Summary of the Pull Request This change cleans up the Fullscreen implementation for both conhost and Terminal, improving the restore position (where the window goes when exiting fullscreen). Prior to this change the window wasn't guaranteed to restore somewhere on the window's current monitor when exiting fullscreen. With this change the window will restore always to its current monitor, at a reasonable location (and will 'double restore' (to fullscreen->maximize->restore) after monitor changes while fullscreen, which is the expected user behavior.  ## References  ## PR Checklist * [x] Closes #9746 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema 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  ## Detailed Description of the Pull Request / Additional comments A fullscreen window's monitor can change. - Win+Shift+left/right migrates a window between monitors. - User could open settings, display, and move the monitor or change its DPI. - The monitor could be unplugged. - The session could be remote and be disconnected. A fullscreen window stores a 'restore position' when entering fullscreen, used to move the window back 'where it was'. BUT, its unexpected for the window to exit fullscreen and jump to another monitor. This means its previous position must be migrated from the old monitor's work area to the new monitor's work area. If a window is maximized, it is sized to the work area. Like with fullscreen, a maximized window has a 'restore position', though unlike with fullscreen the restore position for maximized is stored by the system itself. Migration in cases where a maximized (or fullscreen) window's monitor changes is also taken care of by the system. To restore 'safely' to maximized (after changing window styles) a window must only `SetWindowPos(SWP_FRAMECHANGED)`. While technically a maximized window that becomes fullscreen 'is still maximized' (from Win32's perspective), its prudent to also `ShowWindow(SW_MAXIMIZED)` prior to `SWP_FRAMECHANGED` (to explicitly make the window maximized). If not restoring to maximized, the restore position is adjusted by the new/ old work area. Additionally, the new/ old window DPI is used to adjust the size of the window by the DPI change (keeping the window's logical size the same). - The work area origin is checked first (shifting window rect by the change in origin) - The DPI is checked next, changing right/ bottom (size only) - Each edge of the window is compared against the corresponding edge of the work area, nudging the window back on-screen if hanging offscreen. By shifting right before left, bottom before top, the top-left is guaranteed on-screen.  ## Validation Steps Performed Tried it out. Seemed to work on my machine. Jk, ran conhost/ terminal on mixed DPI system, max (or not), fullscreen, win+shift+left/ exit fullscreen/ maximize. Monitor unplug, etc. (cherry picked from commit bc1ff0b) (cherry picked from commit 14a7e45)

## Summary of the Pull Request This change cleans up the Fullscreen implementation for both conhost and Terminal, improving the restore position (where the window goes when exiting fullscreen). Prior to this change the window wasn't guaranteed to restore somewhere on the window's current monitor when exiting fullscreen. With this change the window will restore always to its current monitor, at a reasonable location (and will 'double restore' (to fullscreen->maximize->restore) after monitor changes while fullscreen, which is the expected user behavior.  ## References  ## PR Checklist * [x] Closes #9746 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema 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  ## Detailed Description of the Pull Request / Additional comments A fullscreen window's monitor can change. - Win+Shift+left/right migrates a window between monitors. - User could open settings, display, and move the monitor or change its DPI. - The monitor could be unplugged. - The session could be remote and be disconnected. A fullscreen window stores a 'restore position' when entering fullscreen, used to move the window back 'where it was'. BUT, its unexpected for the window to exit fullscreen and jump to another monitor. This means its previous position must be migrated from the old monitor's work area to the new monitor's work area. If a window is maximized, it is sized to the work area. Like with fullscreen, a maximized window has a 'restore position', though unlike with fullscreen the restore position for maximized is stored by the system itself. Migration in cases where a maximized (or fullscreen) window's monitor changes is also taken care of by the system. To restore 'safely' to maximized (after changing window styles) a window must only `SetWindowPos(SWP_FRAMECHANGED)`. While technically a maximized window that becomes fullscreen 'is still maximized' (from Win32's perspective), its prudent to also `ShowWindow(SW_MAXIMIZED)` prior to `SWP_FRAMECHANGED` (to explicitly make the window maximized). If not restoring to maximized, the restore position is adjusted by the new/ old work area. Additionally, the new/ old window DPI is used to adjust the size of the window by the DPI change (keeping the window's logical size the same). - The work area origin is checked first (shifting window rect by the change in origin) - The DPI is checked next, changing right/ bottom (size only) - Each edge of the window is compared against the corresponding edge of the work area, nudging the window back on-screen if hanging offscreen. By shifting right before left, bottom before top, the top-left is guaranteed on-screen.  ## Validation Steps Performed Tried it out. Seemed to work on my machine. Jk, ran conhost/ terminal on mixed DPI system, max (or not), fullscreen, win+shift+left/ exit fullscreen/ maximize. Monitor unplug, etc. (cherry picked from commit bc1ff0b)

## Summary of the Pull Request This change cleans up the Fullscreen implementation for both conhost and Terminal, improving the restore position (where the window goes when exiting fullscreen). Prior to this change the window wasn't guaranteed to restore somewhere on the window's current monitor when exiting fullscreen. With this change the window will restore always to its current monitor, at a reasonable location (and will 'double restore' (to fullscreen->maximize->restore) after monitor changes while fullscreen, which is the expected user behavior.  ## References  ## PR Checklist * [x] Closes microsoft#9746 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema 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  ## Detailed Description of the Pull Request / Additional comments A fullscreen window's monitor can change. - Win+Shift+left/right migrates a window between monitors. - User could open settings, display, and move the monitor or change its DPI. - The monitor could be unplugged. - The session could be remote and be disconnected. A fullscreen window stores a 'restore position' when entering fullscreen, used to move the window back 'where it was'. BUT, its unexpected for the window to exit fullscreen and jump to another monitor. This means its previous position must be migrated from the old monitor's work area to the new monitor's work area. If a window is maximized, it is sized to the work area. Like with fullscreen, a maximized window has a 'restore position', though unlike with fullscreen the restore position for maximized is stored by the system itself. Migration in cases where a maximized (or fullscreen) window's monitor changes is also taken care of by the system. To restore 'safely' to maximized (after changing window styles) a window must only `SetWindowPos(SWP_FRAMECHANGED)`. While technically a maximized window that becomes fullscreen 'is still maximized' (from Win32's perspective), its prudent to also `ShowWindow(SW_MAXIMIZED)` prior to `SWP_FRAMECHANGED` (to explicitly make the window maximized). If not restoring to maximized, the restore position is adjusted by the new/ old work area. Additionally, the new/ old window DPI is used to adjust the size of the window by the DPI change (keeping the window's logical size the same). - The work area origin is checked first (shifting window rect by the change in origin) - The DPI is checked next, changing right/ bottom (size only) - Each edge of the window is compared against the corresponding edge of the work area, nudging the window back on-screen if hanging offscreen. By shifting right before left, bottom before top, the top-left is guaranteed on-screen.  ## Validation Steps Performed Tried it out. Seemed to work on my machine. Jk, ran conhost/ terminal on mixed DPI system, max (or not), fullscreen, win+shift+left/ exit fullscreen/ maximize. Monitor unplug, etc.

## Summary of the Pull Request When we're restoring from fullscreen, we do a little adjustment to make sure to clamp the window bounds within the bounds of the active monitor. We unfortunately didn't account for the size of the non-client area (the invisible borders around our 1px border). This didn't matter most of the time, but if the window was within ~8px of the side of the monitor (any side), then restoring from fullscreen would actually move it to the wrong place. As it turns out, the `_quake` window is within ~8px of the edges of the monitor _very often_. ## References * regressed in #9737 ## PR Checklist * [x] Closes #10199 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed The repro in the bug was fairly straightforward. It doesn't happen anymore.

fix fullscreen save restore position, conhost and terminal

dfb55bf

DHowett requested a review from miniksa April 7, 2021 14:09

DHowett added the Product-Conhost For issues in the Console codebase label Apr 7, 2021

Evan Koschik added 2 commits April 7, 2021 13:13

cleanup, remove WM_DPICHANGED special case in conhost

20c7f1a

remove change to vs code settings

a8b27fd

DHowett reviewed Apr 7, 2021

View reviewed changes

ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Apr 8, 2021

zadjii-msft approved these changes Apr 13, 2021

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 13, 2021

miniksa approved these changes Apr 13, 2021

View reviewed changes

ghost merged commit bc1ff0b into microsoft:main Apr 13, 2021

DHowett reviewed Apr 13, 2021

View reviewed changes

zadjii-msft mentioned this pull request Jul 20, 2021

Account for the window borders when restoring from fullscreen #10737

Merged

3 tasks

zadjii-msft mentioned this pull request Dec 9, 2021

strange bar appear while maximized #8384

Closed

zadjii-msft mentioned this pull request Dec 5, 2022

White line at bottom when using "Automatically hide the taskbar" #12959

Open

This pull request was closed.

	UINT dpiWindow = GetDpiForWindow(hWnd);
	UINT const dpiWindow = GetDpiForWindow(hWnd);

Conversation

ekoschik commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

ghost commented Apr 7, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett commented Apr 7, 2021

Uh oh!

github-actions bot commented Apr 7, 2021

Misspellings found, please review:

Uh oh!

github-actions bot commented Apr 7, 2021

Misspellings found, please review:

Uh oh!

DHowett Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

ekoschik Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett commented Apr 12, 2021

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Apr 13, 2021

Uh oh!

ghost commented Apr 13, 2021

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

miniksa Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

miniksa commented Apr 13, 2021

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

ekoschik commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ekoschik commented Apr 7, 2021 •

edited

Loading

ghost commented Apr 7, 2021 •

edited by ghost

Loading

ekoschik Apr 8, 2021 •

edited

Loading