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

Prevent resizing terminal to a lower-than-minimum width #8066

Merged
8 commits merged into from
Nov 13, 2020

Conversation

Don-Vito
Copy link
Contributor

@Don-Vito Don-Vito commented Oct 27, 2020

Until now, we relied on WM_SIZING to ensure that the island is not
downsized below minimal allowed dimensions. However, on some occasions
WM_WINDOWPOSCHANGED, e.g. when anchoring a window to the top/bottom of
the screen. This message will use dimensions obtained from
WM_GETMINMAXINFO. Until now we didn't override this value, falling back
to the defaults. As a result we got an inconsistent behavior (at least
when attaching the anchor).

I added logic very similar to the one we use in IslandWindow::_OnSizing
to the MINMAXINFO handler: snap the client area, add non client
exclusive are, consider DPI along the computation.

Validation Steps Performed

  • Manual testing of minimizing, maximizing, resizing, attaching
    different anchors, etc.

Closes #8026

@ghost ghost added Area-User Interface 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-3 A description (P3) Product-Terminal The new Windows Terminal. labels Oct 27, 2020
@zadjii-msft zadjii-msft self-assigned this Oct 28, 2020
@Don-Vito Don-Vito marked this pull request as ready for review October 28, 2020 13:51
@Don-Vito Don-Vito changed the title 8026: handle WM_GETMINMAXINFO message by configuring minimal island dimensions 8026: prevent resizing terminal to a lower width than minimum width Oct 30, 2020
@Don-Vito
Copy link
Contributor Author

@DHowett - no more hacktoberfest labeling is needed - the tree is already planted. We did it 🌳

@Don-Vito
Copy link
Contributor Author

@zadjii-msft - resolved conflicts and bumping 😊

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Played with this a bit this morning - this seems like it works well enough to me. Thanks for putting this together!

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

days since I pressed ctrl+enter thinking that it was "Approve": 0

@zadjii-msft zadjii-msft removed their assignment Nov 10, 2020
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Looks good to me! Only minor nit-level comments

@@ -2,6 +2,7 @@
// Licensed under the MIT license.

#include "pch.h"
#include <WinUser.h>
Copy link
Member

Choose a reason for hiding this comment

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

this might belong in the pch.h in this project with the other platform includes. I'm very surprised it isn't already included!

Comment on lines 286 to 287
// that might occur in the scenarios, where _OnSizing is bypassed.
// An example of such scenario, is anchoring the window to the top/bottom screen border
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// that might occur in the scenarios, where _OnSizing is bypassed.
// An example of such scenario, is anchoring the window to the top/bottom screen border
// that might occur in the scenarios where _OnSizing is bypassed.
// An example of such scenario is anchoring the window to the top/bottom screen border

extraneous commas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will never learn this - in my mother tongue the comma is required.. while in English it is not.

const auto nonClientSizeScaled = GetTotalNonClientExclusiveSize(dpix);
const auto scale = base::ClampedNumeric<float>(dpix) / USER_DEFAULT_SCREEN_DPI;

LPMINMAXINFO lpMinMaxInfo = reinterpret_cast<LPMINMAXINFO>(lParam);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LPMINMAXINFO lpMinMaxInfo = reinterpret_cast<LPMINMAXINFO>(lParam);
auto lpMinMaxInfo = reinterpret_cast<LPMINMAXINFO>(lParam);

(repetitive w/ the cast type)

@DHowett DHowett changed the title 8026: prevent resizing terminal to a lower width than minimum width prevent resizing terminal to a lower width than minimum width Nov 11, 2020
@DHowett DHowett changed the title prevent resizing terminal to a lower width than minimum width prevent resizing terminal to a lower-than-minimum width Nov 11, 2020
@DHowett DHowett changed the title prevent resizing terminal to a lower-than-minimum width Prevent resizing terminal to a lower-than-minimum width Nov 11, 2020
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Thank you! This is great.

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 13, 2020
@ghost
Copy link

ghost commented Nov 13, 2020

Hello @DHowett!

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 e3fcfcc into microsoft:main Nov 13, 2020
@Don-Vito Don-Vito deleted the fet/8026-minsize branch December 3, 2020 17:04
DHowett pushed a commit that referenced this pull request Jan 25, 2021
Until now, we relied on WM_SIZING to ensure that the island is not
downsized below minimal allowed dimensions. However, on some occasions
WM_WINDOWPOSCHANGED, e.g. when anchoring a window to the top/bottom of
the screen. This message will use dimensions obtained from
WM_GETMINMAXINFO. Until now we didn't override this value, falling back
to the defaults. As a result we got an inconsistent behavior (at least
when attaching the anchor).

I added logic very similar to the one we use in IslandWindow::_OnSizing
to the MINMAXINFO handler: snap the client area, add non client
exclusive are, consider DPI along the computation.

* Manual testing of minimizing, maximizing, resizing, attaching
  different anchors, etc.

Closes #8026

(cherry picked from commit e3fcfcc)
@ghost
Copy link

ghost commented Jan 28, 2021

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

Handy links:

@ghost
Copy link

ghost commented Jan 28, 2021

🎉Windows Terminal Preview v1.6.10272.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-User Interface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met hacktoberfest-accepted Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

possible to resize terminal to a lower width than minimum width
4 participants