Skip to content

No longer load content dialogs when there is already one being shown#12517

Merged
5 commits merged intomainfrom
dev/pabhoj/close_tabs_dialog
Feb 18, 2022
Merged

No longer load content dialogs when there is already one being shown#12517
5 commits merged intomainfrom
dev/pabhoj/close_tabs_dialog

Conversation

@PankajBhojwani
Copy link
Contributor

Summary of the Pull Request

Somehow, the controls v2 update caused an issue where if you as much as load a content dialog when there's already one open, we get holes in the terminal window (#12447)

This commit introduces logic to TerminalPage to check whether there is a content dialog open before we try to load another one.

PR Checklist

Validation Steps Performed

Can no longer repro #12447

@ghost ghost added Area-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Feb 17, 2022
@PankajBhojwani PankajBhojwani added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 18, 2022
@ghost
Copy link

ghost commented Feb 18, 2022

Hello @PankajBhojwani!

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 788d33c into main Feb 18, 2022
@ghost ghost deleted the dev/pabhoj/close_tabs_dialog branch February 18, 2022 22:31
<ContentDialog x:Name="ControlNoticeDialog"
x:Uid="ControlNoticeDialog"
x:Load="False"
CloseButtonClick="_DialogCloseClick"
Copy link
Member

Choose a reason for hiding this comment

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

hey does this still work when the user presses Esc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! We still end up in TerminalPage::_DialogCloseClick upon hitting Esc

zadjii-msft added a commit that referenced this pull request Mar 4, 2022
After the dialog is displayed, always clear it out. If we don't, we won't be able to display another!

* regressed in #12517.
* [x] Fixes #12622.
ghost pushed a commit that referenced this pull request Mar 10, 2022
After the dialog is displayed, always clear it out. If we don't, we won't be able to display another!

* regressed in #12517.
* [x] Fixes #12622.
DHowett pushed a commit that referenced this pull request Mar 10, 2022
…12517)

## Summary of the Pull Request
Somehow, the controls v2 update caused an issue where if you as much as _load_ a content dialog when there's already one open, we get holes in the terminal window (#12447)

This commit introduces logic to `TerminalPage` to check whether there is a content dialog open before we try to load another one.

## PR Checklist
* [x] Closes #12447
* [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.
* [x] I work here

## Validation Steps Performed
Can no longer repro #12447

(cherry picked from commit 788d33c)
DHowett pushed a commit that referenced this pull request Mar 10, 2022
After the dialog is displayed, always clear it out. If we don't, we won't be able to display another!

* regressed in #12517.
* [x] Fixes #12622.

(cherry picked from commit 2e78ebf)
zadjii-msft pushed a commit that referenced this pull request Mar 10, 2022
…12517)

Somehow, the controls v2 update caused an issue where if you as much as _load_ a content dialog when there's already one open, we get holes in the terminal window (#12447)

This commit introduces logic to `TerminalPage` to check whether there is a content dialog open before we try to load another one.

* [x] Closes #12447
* [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.
* [x] I work here

Can no longer repro #12447
zadjii-msft added a commit that referenced this pull request Mar 10, 2022
After the dialog is displayed, always clear it out. If we don't, we won't be able to display another!

* regressed in #12517.
* [x] Fixes #12622.
@ghost
Copy link

ghost commented Mar 25, 2022

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

Handy links:

zadjii-msft added a commit that referenced this pull request Apr 6, 2022
ghost pushed a commit that referenced this pull request Apr 6, 2022
…#12840)

After switching to ControlsV2, it seems that
delay-loading a dialog causes the ContentDialog to be assigned a
Height equal to it's content size. If we DON'T assign the
ContentDialog a Row, I believe it's assigned Row 0 by default. So,
when the dialog gets opened, the dialog seemingly causes a giant
hole to appear in the body of the app.

Assigning all the dialogs to Row 2 (where the rest of the content
is) makes the "hole" appear in the same space as the rest of the
TabContent, fixing the issue.

Note that the actual content in a content dialog gets parented to
the PopupRoot, so it actually always appeared in the correct place, it's
just this weird hole that appeared in Row 0.


* [x] Closes #12775
  * See also: 
    * #12202 was fixed by #12208
    * #12447 was fixed by #12517
* [x] Tested manually
* [x] Reverts #12625
* [x] Reverts #12517
DHowett pushed a commit that referenced this pull request Apr 6, 2022
…#12840)

After switching to ControlsV2, it seems that
delay-loading a dialog causes the ContentDialog to be assigned a
Height equal to it's content size. If we DON'T assign the
ContentDialog a Row, I believe it's assigned Row 0 by default. So,
when the dialog gets opened, the dialog seemingly causes a giant
hole to appear in the body of the app.

Assigning all the dialogs to Row 2 (where the rest of the content
is) makes the "hole" appear in the same space as the rest of the
TabContent, fixing the issue.

Note that the actual content in a content dialog gets parented to
the PopupRoot, so it actually always appeared in the correct place, it's
just this weird hole that appeared in Row 0.

* [x] Closes #12775
  * See also:
    * #12202 was fixed by #12208
    * #12447 was fixed by #12517
* [x] Tested manually
* [x] Reverts #12625
* [x] Reverts #12517

(cherry picked from commit c4e5ebf)
Service-Card-Id: 80266902
Service-Version: 1.12
DHowett pushed a commit that referenced this pull request Apr 6, 2022
…#12840)

After switching to ControlsV2, it seems that
delay-loading a dialog causes the ContentDialog to be assigned a
Height equal to it's content size. If we DON'T assign the
ContentDialog a Row, I believe it's assigned Row 0 by default. So,
when the dialog gets opened, the dialog seemingly causes a giant
hole to appear in the body of the app.

Assigning all the dialogs to Row 2 (where the rest of the content
is) makes the "hole" appear in the same space as the rest of the
TabContent, fixing the issue.

Note that the actual content in a content dialog gets parented to
the PopupRoot, so it actually always appeared in the correct place, it's
just this weird hole that appeared in Row 0.

* [x] Closes #12775
  * See also:
    * #12202 was fixed by #12208
    * #12447 was fixed by #12517
* [x] Tested manually
* [x] Reverts #12625
* [x] Reverts #12517

(cherry picked from commit c4e5ebf)
Service-Card-Id: 80266903
Service-Version: 1.13
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-SettingsUI Anything specific to the SUI 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. Priority-1 A description (P1) Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Another hole in the Terminal when opening a dialog

4 participants