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.

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

Duplicating a tab/pane without a valid profile should still duplicate the settings from that tab #5047

Open
zadjii-msft opened this issue Mar 20, 2020 · 10 comments · Fixed by #10982
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Milestone

Comments

@zadjii-msft
Copy link
Member

zadjii-msft commented Mar 20, 2020

This is a follow-up from PR #4429, and bug #2455

I actually don't hate the idea of just duplicating the settings for the existing control. In that scenario, we'd probably just ignore the settings for that profile in the actual file, and just use whatever settings the control currently has. This might require some plumbing, but it's doable.

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.

As of #4429, we'll silently do nothing. We should instead get the Profile from the Pane/Tab that we're duplicating, and use that to build the settings instead.


note to self: this is the "duplicate the NewTerminalArgs, not just the Profile" thread

@zadjii-msft zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) labels Mar 20, 2020
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Mar 20, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Mar 20, 2020
@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Mar 20, 2020
@DHowett-MSFT
Copy link
Contributor

Yonking triage.

DHowett-MSFT pushed a commit that referenced this issue Mar 26, 2020
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
@zadjii-msft
Copy link
Member Author

Oh hey, this would be related to #6689. If we can get the current settings from the control, to be able to make a change and "preview" them, then we'll be able to get the settings to be able to duplicate the tab/pane w/o the profile

@DHowett DHowett self-assigned this Aug 17, 2021
@DHowett
Copy link
Member

DHowett commented Aug 17, 2021

I'm taking this, as I ended up replacing the use and storage of GUIDs with Profile object references. You can now duplicate a tab of a deleted profile (!)

DHowett added a commit that referenced this issue Aug 18, 2021
This commit also moves to storing a reference to the active profile
inside Pane and propagating it out of Pane through Tab. This lets us
duplicate a pane even if its profile is missing, and gives us the
freedom in the future to return a "base" profile (;))

Related to #5047.
DHowett added a commit that referenced this issue Aug 18, 2021
This commit also moves to storing a reference to the active profile
inside Pane and propagating it out of Pane through Tab. This lets us
duplicate a pane even if its profile is missing, and gives us the
freedom in the future to return a "base" profile (;))

Related to #5047.
@ghost ghost added the In-PR This issue has a related PR label Aug 20, 2021
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Aug 23, 2021
DHowett added a commit that referenced this issue Aug 23, 2021
Right now, we store GUIDs in panes and most of the functions for interacting
with profiles on the settings model take GUIDs and look up profiles.

This pull request changes how we store and look up profiles to prefer profile
objects. Panes store strong references to their originating profiles, which
simplifies settings lookup for CloseOnExit and the bell settings. In fact,
deleting a pane's profile no longer causes it to forget which CloseOnExit
setting applies to it. Duplicating a pane that is hosting a deleted profile
(#5047) now duplicates the profile, even though it is otherwise unreachable.

This makes the world more consistent and allows us to _eventually_ support panes
hosting profiles that do not have GUIDs that can be looked up in the profile
list. This is a gateway to #6776 and #10669, and consolidating the profile
lookup logic will help with #10952.

PR #10588 introduced TerminalSettings::CreateWithProfile and made
...CreateWithProfileByID a thin wrapper over top it, which looked up the profile
by GUID before proceeding. It has also been removed, as its last caller is gone.

Closes #5047
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Aug 23, 2021
@DHowett DHowett reopened this Aug 23, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 23, 2021
@DHowett DHowett removed the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Aug 23, 2021
ghost pushed a commit that referenced this issue Aug 25, 2021
This pull request introduces our first use of the "base" profile as an
actual profile. Incoming commandlines from `wt foo` *and* default
terminal handoffs will be hosted in the base profile.

**THIS IS A BREAKING CHANGE** for user behavior.

The original behavior where commandlines were hosted in the "default"
profile (in most cases, Windows PowerShell) led to user confusion: "why
does cmd use my powershell icon?" and "why does the title say
PowerShell?". Making this change unifies the user experience so that we
can land commandline detection in #10952.

Users who want the original behavior can get it back for commandline
invocation by specifying a profile using the `-p` argument, as in `wt -p
PowerShell -- cmd`.

As a temporary stopgap, users who attempt to duplicate the base profile
will get their specified default profile until we land #5047.

This feature is hidden behind the same feature flag that controls the
visibility of base/"Defaults" in the settings UI.

Fixes #10669
Related to #6776
@ghost
Copy link

ghost commented Aug 31, 2021

🎉This issue was addressed in #10982, which has now been successfully released as Windows Terminal Preview v1.11.2421.0.:tada:

Handy links:

@zadjii-msft
Copy link
Member Author

@DHowett why'd we reopen this? I thought we did this in #10982?

@DHowett
Copy link
Member

DHowett commented Oct 5, 2021

Sorta. Right now we duplicate the profile, but what we really should do is duplicate the NewTerminalArgs (or equivalent, a version we have re-created from the active settings thanks to Rosefield's persistence work) that spawned the pane. That way, we also duplicate any other live settings and the commandline used to spawn the application (if it wasn't the one that came from the profile)

@zadjii-msft
Copy link
Member Author

From @cpriest in #12760:

Adding on to this ticket because my issue is similar.

If I launch windows terminal with custom arguments such as wt.exe ssh ..., then duplicate tab, split pane, none of them work. This is in the same vein as above in that it should exec the same ssh.exe ... in the new tab / pane.

Right now, a new tab opens and then dies immediately.

@zadjii-msft
Copy link
Member Author

Steps to reproduce

Not entirely sure which actions are relevant, and i have a hard time reproducing with explicit actions, so i'll outline my flow

  • Plug laptop into dock
  • Start windows terminal (pwsh profile). (suppressApplicationTitle is set to "true" in the settings.json file)
  • Launch new tab(s) in a new window, specifying titles for tabs

wt.exe -w 2 --title 6000 -p cmd cmd /c "cd \"c:\projectdirectory\" &\"C:\Program Files\dotnet\dotnet.exe\" run --urls \"http://*:6000\" &pause"

  • Some time later, unplug laptop from dock and close
  • Take laptop elsewhere (some amount of commute time), and plug into new dock.
  • Most likely, wait until the following day before opening the laptop again

Expected Behavior

Tab titles should be retained

Actual Behavior

Tab titles revert to the command. In the example, tab title changes to "cmd"

@plutonium-239
Copy link

Coming from #17337
Hello, acc to this comment, this feature was introduced in v1.11 Preview, but the release notes don't mention duplicating the tab while keeping the same directory (and is also not in v1.20 stable).

I'm sorry for not understanding, but is that not what was planned/how it is supposed to work?

@eight04
Copy link

eight04 commented Aug 15, 2024

@plutonium-239 Here is the tutorial: #13195 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants