Skip to content

Fix ShowHide for DefTerm#13129

Merged
DHowett merged 1 commit intomainfrom
dev/migrie/b/13066-for-defterm
May 19, 2022
Merged

Fix ShowHide for DefTerm#13129
DHowett merged 1 commit intomainfrom
dev/migrie/b/13066-for-defterm

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented May 19, 2022

Well this one feels dumb.

Make sure to also initially set the visibility of ConPTY windows created for DefTerm connections.

@zadjii-msft
Fis ShowHide for DefTerm
34f24e2 
Well this one feels dumb.

Make sure to also initially set the visibility of ConPTY windows created for DefTerm connections.

* [x] Closes #13066 for real.
* [x] tested manually.
@ghost ghost added Area-Windowing Window frame, quake mode, tearout Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels May 19, 2022
@zadjii-msft zadjii-msft added Area-DefApp and removed Product-Conpty For console issues specifically related to conpty Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Area-Windowing Window frame, quake mode, tearout labels May 19, 2022
@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label May 19, 2022
@ghost ghost requested review from PankajBhojwani, carlos-zamora and lhecker May 19, 2022 19:02
@DHowett DHowett changed the title Fis ShowHide for DefTerm Fil ShowHide for DefTerm May 19, 2022
@DHowett DHowett changed the title Fil ShowHide for DefTerm Fix ShowHide for DefTerm May 19, 2022
@ghost ghost added Area-Windowing Window frame, quake mode, tearout Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels May 19, 2022
lhecker
lhecker reviewed May 19, 2022
View reviewed changes
src/cascadia/TerminalConnection/ConptyConnection.cpp
Comment on lines +344 to +347
if (_initialVisibility)
{
THROW_IF_FAILED(ConptyShowHidePseudoConsole(_hPC.get(), _initialVisibility));
}
Copy link
Member

@lhecker lhecker May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm a sucker for perf, but I think it'd be more robust to always call that function.
IMO this code is currently similar to:

if (dimensions != COORD{ 120, 30 })
{
    THROW_IF_FAILED(ConptyResizePseudoConsole(_hPC.get(), dimensions));
}

But maybe it makes sense to do it this way of course... 🤔

@DHowett
Copy link
Member

DHowett commented May 19, 2022

@msftbot merge this in 1 minutes

@DHowett DHowett merged commit d82af93 into main May 19, 2022
DHowett pushed a commit that referenced this pull request May 19, 2022
@zadjii-msft @DHowett
Fis ShowHide for DefTerm (#13129)
20a6c70 
Well this one feels dumb.

Make sure to also initially set the visibility of ConPTY windows created for DefTerm connections.

* [x] Closes #13066 for real.
* [x] tested manually.

(cherry picked from commit d82af93)
Service-Card-Id: 82146739
Service-Version: 1.14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DHowett DHowett DHowett approved these changes

@lhecker lhecker lhecker approved these changes

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

Assignees

No one assigned

Labels

Area-DefApp Area-Windowing Window frame, quake mode, tearout 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-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[1.14] ShowWindow(SW_MINIMIZE) doesn't work anymore

3 participants

@zadjii-msft @DHowett @lhecker