Skip to content

Don't open a hole in the terminal window when pasting#12208

Merged
2 commits merged intomainfrom
dev/migrie/b/12202-gaping-holes-when-pasting
Jan 27, 2022
Merged

Don't open a hole in the terminal window when pasting#12208
2 commits merged intomainfrom
dev/migrie/b/12202-gaping-holes-when-pasting

Conversation

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Jan 20, 2022 •
edited
Loading

Turns out, this bug only repros in Controls version 2. I'm not sure why, but it didn't repro only on main. So this fix does nothing until #11720 merges.

This PR prevents us from setting properties on the paste warning dialog unless we actually need to paste. 5f9c551 proves that settings these properties is what would cause the bug in the first place.

I went a step further and cleaned this up a bit. This was always a little weird, having to get the BracketedPasteEnabled for the active control on the UI thread before we actually display the warning. In the post-#5000 future where going back to the control like this would be a x-proc hop, I figured I should just skip that entirely and plumb the BracketedPaste state out in the initial request.

See also: #12241 which would introduce #12202 on its own.

@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-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels Jan 20, 2022
@j4james
Copy link
Collaborator

j4james commented Jan 21, 2022

I'm assuming the "whole" in your PR title was meant to be "hole".

@zadjii-msft
Copy link
Member Author

zadjii-msft commented Jan 21, 2022

I'm assuming the "whole" in your PR title was meant to be "hole".

yep, yea. Thanks 😅

@zadjii-msft zadjii-msft changed the title Don't open a whole in the terminal window when pasting Don't open a hole in the terminal window when pasting Jan 21, 2022
DHowett
DHowett reviewed Jan 21, 2022
View reviewed changes
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.

I don't totally understand how this fixes it . . . what if bracketed paste ISN'T enabled?

I am having trouble reproducing this issue to begin with, so I'm doubly baffled.

@lhecker lhecker self-requested a review January 24, 2022 02:55
@zadjii-msft
Copy link
Member Author

zadjii-msft commented Jan 25, 2022

I don't totally understand how this fixes it . . . what if bracketed paste ISN'T enabled?

I am having trouble reproducing this issue to begin with, so I'm doubly baffled.

Honestly, it makes no sense. Seemingly, the issue arises from doing

                FindName(L"MultiLinePasteDialog").try_as<WUX::Controls::ContentDialog>();
                ClipboardText().Text(text);
                ClipboardContentScrollViewer().ScrollToVerticalOffset(0);

Without the call to

warningResult = co_await _ShowMultiLinePasteWarningDialog()

I couldn't fix the issue by commenting out any single line of the first three. I could however repro even w/o Bracketed Paste just by commenting out the co_await line.

Actually, just calling FindName(L"MultiLinePasteDialog").try_as<WUX::Controls::ContentDialog>(); WITHOUT showing the dialog seems to cause this. I honestly don't know more than that.

@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Jan 27, 2022
@ghost ghost requested a review from miniksa January 27, 2022 23:56
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 27, 2022
@ghost ghost requested review from PankajBhojwani, carlos-zamora and leonMSFT January 27, 2022 23:56
@ghost
Copy link

ghost commented Jan 27, 2022

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 95770ed into main Jan 27, 2022
@ghost ghost deleted the dev/migrie/b/12202-gaping-holes-when-pasting branch January 27, 2022 23:56
@mailinglists35 mailinglists35 mentioned this pull request Feb 6, 2022
Closed
@zadjii-msft zadjii-msft mentioned this pull request Feb 9, 2022
Closed
This was referenced Apr 6, 2022
Closed
Merged
ghost pushed a commit that referenced this pull request Apr 6, 2022
@zadjii-msft
Manually put our ContentDialogs in the tab content, rather than above (…
c4e5ebf 
…#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
@zadjii-msft @DHowett
Manually put our ContentDialogs in the tab content, rather than above (…
d0dfcad 
…#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
@zadjii-msft @DHowett
Manually put our ContentDialogs in the tab content, rather than above (…
173de03 
…#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

Reviewers

@DHowett DHowett DHowett approved these changes

@lhecker lhecker Awaiting requested review from lhecker

@miniksa miniksa Awaiting requested review from miniksa

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

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

Assignees

No one assigned

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-0 Bugs that we consider release-blocking/recall-class (P0) 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.

Pasting multiple lines in bracketed paste mode creates a giant transparent hole

4 participants

@zadjii-msft @j4james @DHowett @lhecker