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.

Sign up for GitHub

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

Jump to bottom

Fix ShowHide for DefTerm #13129

Merged
merged 1 commit into from
May 19, 2022
Merged

Fix ShowHide for DefTerm #13129

merged 1 commit into from
May 19, 2022

Conversation

zadjii-msft
Copy link
Member

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