Skip to content

Account for the window borders when restoring from fullscreen#10737

Merged
2 commits merged intomainfrom
dev/migrie/b/10199-fullscreen-jiggles-quake-mode
Jul 28, 2021
Merged

Account for the window borders when restoring from fullscreen#10737
2 commits merged intomainfrom
dev/migrie/b/10199-fullscreen-jiggles-quake-mode

Conversation

@zadjii-msft
Copy link
Member

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

PR Checklist

Validation Steps Performed

The repro in the bug was fairly straightforward. It doesn't happen anymore.

@ghost ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Jul 20, 2021
@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Jul 20, 2021
// We want to make sure the window is restored within the bounds of the
// monitor we're on, but it's totally fine if the invisible borders are
// outside the monitor.
rcWorkAdjusted.left -= ncSize.width<long>() / 2;
Copy link
Member

Choose a reason for hiding this comment

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

The optimizer will probably catch it... but I'm not a huge fan of doing the division twice...

Copy link
Member Author

Choose a reason for hiding this comment

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

meh, okay

Copy link
Member

Choose a reason for hiding this comment

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

I mean whatever. I'm not your mom.

if (rcRestore.left < rcWorkAdjusted.left)
{
OffsetRect(&rcRestore, rcWork.left - rcRestore.left, 0);
OffsetRect(&rcRestore, rcWorkAdjusted.left - rcRestore.left, 0);
Copy link
Member

@lhecker lhecker Jul 22, 2021

Choose a reason for hiding this comment

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

This fails to work properly if rcRestore covers a larger area than rcWorkAdjusted doesn't it? Is that alright?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so... I think the way it already was, the whole point was clipping rcRestore inside of rcWork. So I think it only ever did something when rcWork was smaller than rcRestore.

Now we're just making rcWork a little bigger, so we can keep the invisible borders off the monitor

@miniksa
Copy link
Member

miniksa commented Jul 22, 2021

No automerge so Mike can decide whether I am or am not his mom and also respond to Leonard's comment.

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 28, 2021
@ghost
Copy link

ghost commented Jul 28, 2021

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit f058b08 into main Jul 28, 2021
@ghost ghost deleted the dev/migrie/b/10199-fullscreen-jiggles-quake-mode branch July 28, 2021 22:19
DHowett pushed a commit that referenced this pull request Aug 25, 2021
## 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.
@ghost
Copy link

ghost commented Aug 31, 2021

🎉Windows Terminal Preview v1.10.2383.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Aug 31, 2021

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-UserInterface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Priority-2 A description (P2) Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Quake Mode has incorrect window size after toggling fullscreen mode

4 participants