starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-09-24 16:05:21 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

[PR #5090] [MERGED] Process actions sync. on startup; don't dupe nonexistent profile #26103

New Issue
Closed
opened 2026-01-31 09:13:56 +00:00 by claunia · 0 comments
claunia commented 2026-01-31 09:13:56 +00:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/microsoft/terminal/pull/5090
Author: @zadjii-msft
Created: 3/23/2020
Status: ✅ Merged
Merged: 3/26/2020
Merged by: @DHowett-MSFT

Base: master ← Head: dev/migrie/b/pane-initialization-order

📝 Commits (10+)

  • a8bd503 I believe this is the bugfix
  • 648373d Try writing tests
  • 029b676 clean up these tests for PR
  • 0a6f72c Fix the tests o__o
  • 9849fe9 Merge remote-tracking branch 'origin/master' into dev/migrie/b/2455-with-new-tests
  • 963a830 Add the VERIFY_THROWS for dustin
  • 8ed39cd Merge branch 'master' into dev/migrie/b/2455-with-new-tests
  • 82ad8bb I think this will pull the settings from the control, but I haven't tested yet
  • eafe9c8 Wrap Application::Current in try/catch since that breaks tests
  • 908fd74 Try to clone the settings directly from the control

📊 Changes

11 files changed (+879 additions, -148 deletions)

View changed files

📝 src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp (+144 -0)
📝 src/cascadia/LocalTests_TerminalApp/TabTests.cpp (+328 -0)
📝 src/cascadia/TerminalApp/Pane.cpp (+80 -26)
📝 src/cascadia/TerminalApp/Pane.h (+7 -2)
📝 src/cascadia/TerminalApp/Tab.cpp (+56 -10)
📝 src/cascadia/TerminalApp/Tab.h (+12 -0)
📝 src/cascadia/TerminalApp/TerminalPage.cpp (+224 -107)
📝 src/cascadia/TerminalApp/TerminalPage.h (+14 -1)
📝 src/cascadia/TerminalApp/TerminalPage.idl (+1 -0)
📝 src/cascadia/TerminalControl/TSFInputControl.cpp (+7 -2)
📝 src/cascadia/TerminalControl/TermControl.cpp (+6 -0)

📄 Description

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

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/microsoft/terminal/pull/5090 **Author:** [@zadjii-msft](https://github.com/zadjii-msft) **Created:** 3/23/2020 **Status:** ✅ Merged **Merged:** 3/26/2020 **Merged by:** [@DHowett-MSFT](https://github.com/DHowett-MSFT) **Base:** `master` ← **Head:** `dev/migrie/b/pane-initialization-order` --- ### 📝 Commits (10+) - [`a8bd503`](https://github.com/microsoft/terminal/commit/a8bd5036f0d85d9910e56afb86664b8bca9c0d4e) I believe this is the bugfix - [`648373d`](https://github.com/microsoft/terminal/commit/648373d05cb24aadd49d522f2348994bf0bbd248) Try writing tests - [`029b676`](https://github.com/microsoft/terminal/commit/029b67633718ca525e313fdca20729d5f1519f61) clean up these tests for PR - [`0a6f72c`](https://github.com/microsoft/terminal/commit/0a6f72c6dea793f8ea0f2c229f07e8af98ebf8c5) Fix the tests o__o - [`9849fe9`](https://github.com/microsoft/terminal/commit/9849fe9b638676bb41b04da1c292f4c4eadb5992) Merge remote-tracking branch 'origin/master' into dev/migrie/b/2455-with-new-tests - [`963a830`](https://github.com/microsoft/terminal/commit/963a83024142d5ebabcafef0e64115f23ce62631) Add the VERIFY_THROWS for dustin - [`8ed39cd`](https://github.com/microsoft/terminal/commit/8ed39cd4ce6e926d1d272b04efc4310a29f21b46) Merge branch 'master' into dev/migrie/b/2455-with-new-tests - [`82ad8bb`](https://github.com/microsoft/terminal/commit/82ad8bba3c62996af805256661aad617ede1f2b5) I _think_ this will pull the settings from the control, but I haven't tested yet - [`eafe9c8`](https://github.com/microsoft/terminal/commit/eafe9c8fbce329988d5708c8a80bad8be3d2b079) Wrap Application::Current in try/catch since that breaks tests - [`908fd74`](https://github.com/microsoft/terminal/commit/908fd74c6762dae3cf7d1b0b545c7a1ae01b5d4d) Try to clone the settings directly from the control ### 📊 Changes **11 files changed** (+879 additions, -148 deletions) <details> <summary>View changed files</summary> 📝 `src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp` (+144 -0) 📝 `src/cascadia/LocalTests_TerminalApp/TabTests.cpp` (+328 -0) 📝 `src/cascadia/TerminalApp/Pane.cpp` (+80 -26) 📝 `src/cascadia/TerminalApp/Pane.h` (+7 -2) 📝 `src/cascadia/TerminalApp/Tab.cpp` (+56 -10) 📝 `src/cascadia/TerminalApp/Tab.h` (+12 -0) 📝 `src/cascadia/TerminalApp/TerminalPage.cpp` (+224 -107) 📝 `src/cascadia/TerminalApp/TerminalPage.h` (+14 -1) 📝 `src/cascadia/TerminalApp/TerminalPage.idl` (+1 -0) 📝 `src/cascadia/TerminalControl/TSFInputControl.cpp` (+7 -2) 📝 `src/cascadia/TerminalControl/TermControl.cpp` (+6 -0) </details> ### 📄 Description 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 `Pane`s 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 --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
claunia added the pull-request label 2026-01-31 09:13:56 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#26103
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?