Process actions sync. on startup; don't dupe nonexistent profile #5090

zadjii-msft · 2020-03-23T21:26:08Z

This PR has evolved to encapsulate two related fixes that I can't really
untie anymore.

#2455 - Duplicating a tab that doesn't exist anymore

This was the bug I was originally fixing in #4429.

When the user tries to duplicateTab with a profile that doesn't exist
anymore (like might happen after a settings reload), don't crash.

As I was going about adding tests for this, got blocked by the fact that
the Terminal couldn't open any panes while the TerminalPage was size
0x0. This had two theoretical solutions:

Fake the TerminalPage into thinking it had a real size in the test -
probably possible, though I'm unsure how it would work in practice.
Change Panes to not require an ActualWidth, ActualHeight on
initialization.

Fortuately, the second option was something else that was already on my
backlog of bugs.

#4618 - wt command-line can't consistently parse more than one arg

Presently, the Terminal just arbitrarily dispatches a bunch of handlers
to try and handle all the commands provided on the commandline. That's
lead to a bunch of reports that not all the commands will always get
executed, nor will they all get executed in the same order.

This PR also changes the TerminalPage to be able to dispatch all the
commands sequentially, all at once in the startup. No longer will there
be a hot second where the commands seem to execute themselves in from of
the user - they'll all happen behind the scenes on startup.

This involved a couple other changes areound the TerminalPage

I had to make sure that panes could be opened at a 0x0 size. Now they
use a star sizing based off the percentage of the parent they're
supposed to consume, so that when the parent does get laid out,
they'll take the appropriate size of that parent.
I had to do some math ahead of time to try and calculate what a
SplitState::Automatic would be evaluated as, despite the fact that
we don't actually know how big the pane will be.
I had to ensure that focus-tab commands appropriately mark a single
tab as focused while we're in startup, without roundtripping to the
Dispatcher thread and back

References

#4429 - the original PR for #2455
#5047 - a follow-up task from discussion in #4429
#4953 - a PR for making panes use star sizing, which was immensly
helpful for this PR.

Detailed Description of the Pull Request / Additional comments

CascadiaSettings::BuildSettings can throw if the GUID doesn't exist.
This wraps those calls up with a try/catch.

It also adds a couple tests - a few SettingsTests for try/catching
this state. It also adds a XAML-y test in TabTests that creates a
TerminalPage and then performs som UI-like actions on it. This test
required a minor change to how we generate the new tab dropdown - in the
tests, Application::Current() is not a TerminalApp::App, so it
doesn't have a Logic() to query. So wrap that in a try/catch as well.

While working on these tests, I found that we'd crash pretty agressively
for mysterious reasons if the TestHostApp became focused while the test
was running. This was due to a call in
TSFInputControl::NotifyFocusEnter that would callback to
TSFInputControl::_layoutRequested, which would crash on setting the
MaxSize of the canvas to a negative value. This PR includes a hotfix
for that bug as well.

Validation Steps Performed

Manual testing with a lot of commands in a commandline
run the tests
Team tested in selfhost

Closes #2455
Closes #4618

You need to initialize a TerminalPage weirdly in the test, including _not_ checking App.Logic(), since the App doesn't have a Logic()

…ith-new-tests

# Conflicts: # src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp # src/cascadia/TerminalApp/TerminalPage.cpp

… tested yet

Okay this is harder, but maybe not impossible, but probably impossible in v1. The Profile knows how to build the connection settings, and the ControlSettings, etc. However, when the settings reloads like in this bug, we'll remove all the old profiles, and build a new list. If we wanted the case 1 behavior, we could wait until the Profile is a winrt object, then have each Pane hold a strong ref to the Profile that it's hosting. Then it would be trivial to be able to duplicate the settings from it. I believe that work is tracked in #3998. How palatable is doing case 3 now (to fix the crash), then filing a follow up to do case 1 once #3998 is done?

… haven't tested yet" This reverts commit 82ad8bb.

This reverts commit 908fd74.

…nitialization-order

…create the first tab :O

…nitialization-order

…t-pane` doesn't seem to work anymore

…itializing

DHowett-MSFT · 2020-03-23T21:50:55Z

Does this fix the minor visual issue where you do wt ; ; ; ; ; and the tabs all pop into existence slowly?

zadjii-msft · 2020-03-23T21:54:44Z

Does this fix the minor visual issue where you do wt ; ; ; ; ; and the tabs all pop into existence slowly?

Yes, yes it does :)

carlos-zamora

Most of the initialization stuff is pretty foreign for me. I still have to review the Tests but I'll do that later.

src/cascadia/TerminalApp/Pane.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalControl/TSFInputControl.cpp

zadjii-msft · 2020-03-24T14:51:19Z

Sorry all for submitting the PR before pushing the last code cleanup commit 😅 Things should be much better now

DHowett-MSFT

Let's do it. I'll cut a SH build when this is in.

DHowett-MSFT · 2020-03-24T22:22:19Z

I'm producing a selfhost test build with this pre-merged; it is likely ready, but I'll let you make that call.

zadjii-msft · 2020-03-25T13:32:42Z

Note to the team: I'm giving this like 48 hours of selfhost before we ship it. Hence why I haven't AutoMerge'd this yet ☺️

DHowett-MSFT · 2020-03-25T17:54:41Z

This feels so much better. We should just hashtag shipit

…nitialization-order

DHowett-MSFT · 2020-03-25T23:00:11Z

@jsoref this may be what you were talking about on the other thread; I’m not getting a green check here despite merging in master?

jsoref · 2020-03-25T23:20:20Z

@DHowett-MSFT : hmm, that isn't the right explanation for this. @german-one 's action is running in his fork (which is good) -- https://github.com/german-one/terminal/actions. The action should also be running on branches in this repo, but it doesn't appear to be: https://github.com/Microsoft/terminal/actions.

Fwiw, roughly the same configuration is in play here: https://github.com/check-spelling/examples-testing/actions and it's working:
check-spelling/examples-testing@0a7e8a2

Is it possible that these branches also have tags? If so, we might try removing:
https://github.com/microsoft/terminal/blob/master/.github/workflows/spelling.yml#L6-L7

I added that because for our repositories we were experiencing double builds in github.

jsoref · 2020-03-25T23:35:02Z

@DHowett-MSFT : can you try making a branch of https://github.com/jsoref/terminal/pull/new/drop-tags-ignore in Microsoft/terminal? (no PR, just pull that branch into your repo and push it into github with a branch name)

DHowett-MSFT · 2020-03-25T23:36:39Z

6f717ae

…nitialization-order

DHowett-MSFT · 2020-03-25T23:57:06Z

See, this is why I wanted to run spell on this branch :D

ghost · 2020-04-22T17:13:48Z

🎉Windows Terminal Preview v0.11.1121.0 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft added 30 commits January 31, 2020 11:34

I believe this is the bugfix

a8bd503

Try writing tests

648373d

You need to initialize a TerminalPage weirdly in the test, including _not_ checking App.Logic(), since the App doesn't have a Logic()

clean up these tests for PR

029b676

Fix the tests o__o

0a6f72c

Merge remote-tracking branch 'origin/master' into dev/migrie/b/2455-w…

9849fe9

…ith-new-tests

Add the VERIFY_THROWS for dustin

963a830

Merge branch 'master' into dev/migrie/b/2455-with-new-tests

8ed39cd

# Conflicts: # src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp # src/cascadia/TerminalApp/TerminalPage.cpp

I _think_ this will pull the settings from the control, but I haven't…

82ad8bb

… tested yet

Wrap Application::Current in try/catch since that breaks tests

eafe9c8

Revert "I _think_ this will pull the settings from the control, but I…

4561b9a

… haven't tested yet" This reverts commit 82ad8bb.

Revert "Try to clone the settings directly from the control"

9eaee52

This reverts commit 908fd74.

fix the test so it compiles again

e952c0d

add a note on what's going wrong here

07050b5

I have no idea why but this seemed to work to initialize the panes

4bb6803

immediately focus new panes

499b003

this works to alternate auto snapping, but maybe there's a better way

95bb854

precompute auto sizing correctly

a058181

Some code cleanup "before" 5pm

998318a

I somehow totally blew up the test?

f3948d8

Merge remote-tracking branch 'origin/master' into dev/migrie/b/pane-i…

332a577

…nitialization-order

okay this fixes this test

1dffef4

try manually cleaning up the page, but this doesn't work

e2e45a9

cleanup the test

808868f

This is a ton of colde cleanup ; wtd split-pane is broken, doesn't …

6208f43

…create the first tab :O

Merge remote-tracking branch 'origin/master' into dev/migrie/b/pane-i…

7e0685f

…nitialization-order

this fixes just a bare wt split-pane; `wt; ; ; focus-tab -t 1; spli…

09ab62a

…t-pane` doesn't seem to work anymore

this fixes focus-tab during startup, but it's kinda janky

ce2b948

god bless, I fixed the tests

4306389

add an Initialized event so the tests can know when we're finished in…

2b3b517

…itializing

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 23, 2020

carlos-zamora reviewed Mar 23, 2020

View reviewed changes

leonMSFT reviewed Mar 23, 2020

View reviewed changes

src/cascadia/TerminalControl/TSFInputControl.cpp Show resolved Hide resolved

zadjii-msft added 2 commits March 24, 2020 09:47

Tons of non-test code cleanup

393492b

wacky--;

a1c7f7e

carlos-zamora approved these changes Mar 24, 2020

View reviewed changes

DHowett-MSFT approved these changes Mar 24, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into dev/migrie/b/pane-i…

1a15db3

…nitialization-order

Merge remote-tracking branch 'origin/master' into dev/migrie/b/pane-i…

34391c7

…nitialization-order

spelling

c3facb5

DHowett-MSFT changed the title ~~Process startup commands synchronously on startup, and don't crash when the user tries to duplicate a tab with a profile that no longer exists~~ Process actions sync. on startup; don't dupe nonexistent profile Mar 26, 2020

DHowett-MSFT merged commit b3fa88e into master Mar 26, 2020

DHowett-MSFT deleted the dev/migrie/b/pane-initialization-order branch March 26, 2020 00:03

zadjii-msft mentioned this pull request Mar 30, 2020

TryDuplicateBadTab and TryDuplicateBadPane unit tests are hanging when run locally #5169

Closed

DHowett-MSFT mentioned this pull request Apr 3, 2020

Split pane from command prompt yields inconsistent results #4430

Closed

This was referenced Apr 22, 2020

wt command-line cannot consistently parse more than one argument #4618

Closed

Bug Report: Cannot find profile --> Crash #2455

Closed

nbabanov mentioned this pull request Apr 24, 2020

[wt.exe] Add support for appending a commandline to a profile's commandline #5528

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process actions sync. on startup; don't dupe nonexistent profile #5090

Process actions sync. on startup; don't dupe nonexistent profile #5090

zadjii-msft commented Mar 23, 2020 •

edited by DHowett-MSFT

Loading

DHowett-MSFT commented Mar 23, 2020

zadjii-msft commented Mar 23, 2020

carlos-zamora left a comment

zadjii-msft commented Mar 24, 2020

DHowett-MSFT left a comment

DHowett-MSFT commented Mar 24, 2020

zadjii-msft commented Mar 25, 2020

DHowett-MSFT commented Mar 25, 2020

DHowett-MSFT commented Mar 25, 2020

jsoref commented Mar 25, 2020

jsoref commented Mar 25, 2020

DHowett-MSFT commented Mar 25, 2020

DHowett-MSFT commented Mar 25, 2020

ghost commented Apr 22, 2020

Process actions sync. on startup; don't dupe nonexistent profile #5090

Process actions sync. on startup; don't dupe nonexistent profile #5090

Conversation

zadjii-msft commented Mar 23, 2020 • edited by DHowett-MSFT Loading

References

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

DHowett-MSFT commented Mar 23, 2020

zadjii-msft commented Mar 23, 2020

carlos-zamora left a comment

Choose a reason for hiding this comment

zadjii-msft commented Mar 24, 2020

DHowett-MSFT left a comment

Choose a reason for hiding this comment

DHowett-MSFT commented Mar 24, 2020

zadjii-msft commented Mar 25, 2020

DHowett-MSFT commented Mar 25, 2020

DHowett-MSFT commented Mar 25, 2020

jsoref commented Mar 25, 2020

jsoref commented Mar 25, 2020

DHowett-MSFT commented Mar 25, 2020

DHowett-MSFT commented Mar 25, 2020

ghost commented Apr 22, 2020

zadjii-msft commented Mar 23, 2020 •

edited by DHowett-MSFT

Loading