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

Add support for tab tearoff and tab merge #1256

Open
Tracked in #5000
DHowett-MSFT opened this issue Jun 14, 2019 · 20 comments
Open
Tracked in #5000

Add support for tab tearoff and tab merge #1256

DHowett-MSFT opened this issue Jun 14, 2019 · 20 comments
Labels
Area-User Interface Issue-Feature Product-Terminal
Milestone

Comments

@DHowett-MSFT
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT commented Jun 14, 2019

I'm repurposing this for tab reordering and demoting it to a Task. We'll track the (much larger) workitem to do tab tearoff as a deliverable for vBacklog.
Notes/reasoning:

  • Tab reordering is easy.
  • Tab tearoff requires us to answer the following questions:
    • WT.exe is a multi-instance app. How do we drag tabs between different processes?
      • Should WT be multi-instance? We may want to single-instance it.
        • If we want to single-instance it we need to design "new window" (part of #1051)
    • We still need to design "new window" for tearoff-without-merge because we need a drop target for the old tab.

Originally posted by @DHowett-MSFT in #443 (comment)

@msftbot msftbot bot added Needs-Triage Needs-Tag-Fix labels Jun 14, 2019
@DHowett-MSFT DHowett-MSFT added Area-User Interface Issue-Feature Product-Terminal and removed Needs-Triage labels Jun 14, 2019
@DHowett-MSFT DHowett-MSFT added this to Spec Needed ❓ in Specification Tracker via automation Jun 14, 2019
@msftbot msftbot bot removed the Needs-Tag-Fix label Jun 14, 2019
@DHowett-MSFT DHowett-MSFT added this to the Terminal Backlog milestone Jun 14, 2019
@paradonym
Copy link

@paradonym paradonym commented Jun 26, 2019

also to have a sort of way to manage many open tabs... If I have 3 WSL tabs 1 cmd and two powershells tabbed it get's out of the UI at the right side of the tab bar (so I can't close the last tabs unless I close the first ones).

@cdmihai
Copy link

@cdmihai cdmihai commented Sep 25, 2019

This would be so useful. I constantly open new tabs on some focused work, start splitting panes within the tab, and then at some point decide to move the whole tab and panes (right now I just recreate the panes and their directories from scratch) to a new terminal window in a new virtual desktop.

@adgustaf
Copy link

@adgustaf adgustaf commented Mar 23, 2020

Hi,

Any progress on this? Just wanted to say I find this to be very useful.

Thanks for all your hard work on this product!

@DHowett-MSFT
Copy link
Contributor Author

@DHowett-MSFT DHowett-MSFT commented Mar 23, 2020

Thanks for asking! Unfortunately, there hasn’t been any progress. When there is, this issue will be the first to be updated about it.

@adgustaf
Copy link

@adgustaf adgustaf commented Mar 23, 2020

Thanks for the info! Okay I'll be following this issue :)

@microsoft microsoft unlocked this conversation Jul 21, 2021
PankajBhojwani pushed a commit that referenced this issue Jul 21, 2021
## Summary of the Pull Request

This forces the `TermControl` to only use `ControlCore` and `ControlInteractivity` via their WinRT projections. We want this, because WinRT projections can be used across process boundaries. In the future, `ControlCore` and `ControlInteractivity` are going to be living in a different process entirely from `TermControl`. By enforcing this boundary now, we can make sure that they will work seamlessly in the future.

## References
* Tear-out: #1256
* Megathread: #5000
* Project: https://github.com/microsoft/terminal/projects/5

## PR Checklist
* [x] Closes https://github.com/microsoft/terminal/projects/5#card-50760270
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

Most all this was just converting pure c++ types to winrt types when possible. I've added a couple helper projections with `til` converters, which made most of this really easy.

The "`MouseButtonState` needs to be composed of `Int32`s instead of `bool`s" is MENTAL. I have no idea why this is, but when I had the control OOP in the sample, that would crash when trying to de-marshal the bools. BODGY.

The biggest changes are in the way the UIA stuff is hooked up. The UiaEngine needs to be attached directly to the `Renderer`, and it can't be easily projected, so it needs to live next to the `ControlCore`. But the `TermControlAutomationPeer` needed the `UiaEngine` to help implement some interfaces.

Now, there's a new layer we've introduced. `InteractivityAutomationPeer` does the `ITextProvider`, `IControlAccessibilityInfo` and the `IUiaEventDispatcher` thing. `TermControlAutomationPeer` now has a 
`InteractivityAutomationPeer` stashed inside itself, so that it can ask the interactivity layer to do the real work. We still need the `TermControlAutomationPeer` though, to be able to attach to the real UI tree.

## Validation Steps Performed

The terminal behaves basically the same as before.

Most importantly, I whipped out Accessibility Insights, and the Terminal looks the same as before.
PankajBhojwani pushed a commit that referenced this issue Jul 21, 2021
#### ⚠️ targets #10051 

## Summary of the Pull Request

This PR does one big, primary thing. It removes all the constructors from any TerminalConnections, and changes them to use an `Initialize` method that accepts a `ValueSet` of properties.

Why?

For the upcoming window/content process work, we'll need the content process to be able to initialize the connection _in the content process_. However, the window process will be the one that knows what type of connection to make. Enter `ConnectionInformation`. This class will let us specify the class name of the type we want to create, and a set of settings to use when initializing that connection.

**IMPORTANT**: As a part of this, the constructor for a connection must have 0 arguments. `RoActivateInstance` lets you just conjure a WinRT type just by class name, but that class must have a 0 arg ctor. Hence the need for `Initialize`, to actually pass the settings.

We're using a `ValueSet` here because it's basically a json blob, with more steps. In the future, when extension authors want to have custom connections, we can always deserialize the json into a `ValueSet`, pass it to their connection's `Initialize`, and let then get what they need out of it.

## References
* Tear-out: #1256
* Megathread: #5000
* Project: https://github.com/microsoft/terminal/projects/5

## PR Checklist
* [x] Closes https://github.com/microsoft/terminal/projects/5#card-50760298
* [x] I work here
* [n/a] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

`ConnectionInformation` was included as a part of this PR, to demonstrate how this will eventually be used. `ConnectionInformation` is not _currently_ used.

## Validation Steps Performed

It still builds and runs.
alexshcer pushed a commit to alexshcer/terminal that referenced this issue Jul 27, 2021
#### ⚠️ This targets microsoft#10051

## Summary of the Pull Request

This PR creates a `Samples` solution which can be used for quickly testing our particular WinUI + Xaml Islands + wapproj setup, with a `TermControl`. This lets us quickly prototype and minimally repro xaml islands bugs, but also lets us iterate quickly on changes in the process model. I'll be using this in the future to prove that the out-of-proc control works (for tear-out), without having to tear up the entire `TerminalApp` all at once.

While I'll be leaning on this extensively for tear-out prototyping, I've also found myself wanting this kind of sample sln many times in the past. We run into bugs all the time where we're not sure if they're XAML Islands bugs or Terminal bugs. However, standing up a scratch sln with MUX hooked up, and a `XamlApplication` from the toolkit, and XAML Islands is time consuming. This sample sln should let us validate if bugs are XI bugs or Terminal bugs much easier.

## References
* Tear-out: microsoft#1256
* Megathread: microsoft#5000
* Project: https://github.com/microsoft/terminal/projects/5

## PR Checklist
* [x] Closes one bullet point of https://github.com/microsoft/terminal/projects/5#card-50760312
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

This is _largely_ a copy-pasta of our existing projects, in massively simplified form. I'm gonna wontfix most comments on the code that was copy-pasta'd. If it's bad here, it's probably also bad in the real version in `OpenConsole.sln`.

* I made an entirely new `.sln` for this, so that these samples wouldn't need to build in CI. They're not critical code, they're literally just for prototyping.
* `TerminalAppLib` & `TerminalApp` became `SampleAppLib` and `SampleApp`. This is just a simple application hosting a single `TermControl`. It references the winmds and dlls from the main `OpenConsole.sln`, but in a way that would be more like a project consuming a nuget feed of the control, rather than a `ProjectReference`.
  - It still does all the `App.xaml` and `Toolkit.XamlApplication` stuff that TerminalApp does, so that it can load MUX resources, and do MUX-y things.
* `WindowsTerminal` became `WindowExe`. I tore out all of the `NonClientIslandWindow` stuff - this is just a window with a xaml island.
* `CascadiaPackage` became `Package`. It does the vclibs hack again for the `TerminalConnection` dlls (because this package can't actually determine that `TerminalConnection.dll` requires `cprest*.dll`), as well as for `windowsterminal.exe` (which we'll need in the future for out-of-proc controls). I got rid of all the Dev/Preview/Release asset machinations as well.

Wherever possible, I changed filenames slightly so that they won't get accitdentally opened when someone tries to open a file by name in their editor (**cough** sublime's <kbd>ctrl+p</kbd> menu **cough**).

## Validation Steps Performed

The sample app launches, and displays a TermControl. What else do you want? <sup>(_rhetorical, not a prompt for a wishlist_)</sup>

(cherry picked from commit 30d6cf4)
msftbot bot pushed a commit that referenced this issue Aug 9, 2021
#### ⚠️ targets #10051

## Summary of the Pull Request

This updates our `ThrottledFunc`s to take a dispatcher parameter. This means that we can use the `Windows::UI::Core::CoreDispatcher` in the `TermControl`, where there's always a `CoreDispatcher`, and use a `Windows::System::DispatcherQueue` in `ControlCore`/`ControlInteractivity`. When running in-proc, these are always the _same thing_. However, out-of-proc, the core needs a dispatcher queue that's not tied to a UI thread (because the content proces _doesn't have a UI thread!_). 

This lets us get rid of the output event, because we don't need to bubble that event out to the `TermControl` to let it throttle that update anymore. 

## References
* Tear-out: #1256
* Megathread: #5000
* Project: https://github.com/microsoft/terminal/projects/5

## PR Checklist
* [x] This is a part of #1256
* [x] I work here
* [n/a] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

Fortunately, `winrt::resume_foreground` works the same on both a `CoreDispatcher` and a `DispatcherQueue`, so this wasn't too hard!

## Validation Steps Performed

This was validated in `dev/migrie/oop/the-whole-thing` (or `dev/migrie/oop/connection-factory`, I forget which), and I made sure that it worked both in-proc and x-proc. Not only that, _it wasn't any slower_!This reverts commit 04b751f.
@kczx3
Copy link

@kczx3 kczx3 commented Oct 11, 2021

@zadjii-msft Would you be able to provide a brief update of the progress towards these features? I can see there's definitely been activity and commits where this issue has been referenced but a nice couple sentence paragraph of your progress would be very uplifting. Thanks for your time!

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Oct 12, 2021

More specific details, including my todo list are in #5000. I'm hoping to do the control settings rewrite this release (dev/migrie/oop/ragnarok), and hopefully some of the other foundation work before the end of the year (dev/migrie/oop/infinity-war). Hopefully we can get "move-pane -w <id> to move a pane/tab to another Terminal window" early next year, and followed shortly by tearing actual tabs out. This is still my main priority, just one that keeps getting sidelined by other things that can ship in a single release cycle. We've got the end of the year coming up and things always slow way down around then, and there's some time dedicated in the near future to improving our tooling (which should help accelerate overall code velocity)

msftbot bot pushed a commit that referenced this issue Dec 1, 2021
## Summary of the Pull Request

Currently, the TermControl and ControlCore recieve a settings object that implements `IControlSettings`. They use for this for both reading the settings they should use, and also storing some runtime overrides to those settings (namely, `Opacity`). The object they recieve currently is a `T.S.M.TerminalSettings` object, as well as another `TerminalSettings` object if the user wants to have an `unfocusedAppearance`. All these are all hosted in the same process, so everything is fine and dandy. 

With the upcoming move to having the Terminal split into multiple processes, this will no longer work. If the `ControlCore` in the Content Process is given a pointer to a `TerminalSettings` in a certain Window Process, and that control is subsequently moved to another window, then there's no guarantee that the original `TerminalSettings` object continues to exist. In this scenario, when window 1 is closed, now the Core is unable to read any settings, because the process that owned that object no longer exists. 

The solution to this issue is to have the `ControlCore`'s own their own copy of the settings they were created with. that way, they can be confident those settings will always exist. Enter `ControlSettings`, a dumb struct for just storing all the contents of the Settings. I used x-macros for this, so that we don't need to copy-paste into this file every time we add a setting. 

Changing this has all sorts of other fallout effects:
* Previewing a scheme/anything is a tad bit more annoying. Before, we could just sneak the previewed scheme into a `TerminalSettings` that lived between the settings we created the control with, and the settings they were actually using, and it would _just work_. Even explaining that here, it sounds like magic, because it was. However, now, the TermControl can't use a layered `TerminalSettings` for the settings anymore. Now we need to actually read out the current color table, and set the whole scheme when we change it. So now there's also a `Microsoft.Terminal.Core.Scheme` _struct_ for holding that data. 
  - Why a `struct`? Because that will go across the process boundary as a blob, rather than as a pointer to an object in the other process. That way we can transit the whole struct from window to core safely. 
* A TermControl doesn't have a `IControlSettings` at all anymore - it initalizes itself via the settings in the `Core`. This will be useful for tear-out, when we need to have the `TermControl` initialize itself from just a `ControlCore`, without being able to rebuild the settings from scratch.
* The `TabTests` that were written under the assumption that the Control had a layered `TerminalSettings` obviously broke, as they were designed to. They've been modified to reflect the new reality.
* When we initialize the Control, we give it the settings and the `UnfocusedAppearance` all at once. If we don't give it an `unfocusedAppearance`, it will just use the focused appearance as the unfocused appearance.
* The Control no longer can _write_ settings to the `ControlSettings`. We don't want to be storing things in there. Pretty much everything we set in the control, we store somewhere other than in the settings object itself. However, `opacity` and `useAcrylic`, we need to store in a handy new `RUNTIME_SETTING` property. We can write those runtime overrides to those properties.  
* We no longer store the color scheme for a pane in the persisted state. I'm tracking that in #9800. I don't think it's too hard to add back, but I wanted this in front of eyes sooner than later.

## References

* #1256
* #5000
* #9794 has the scheme previewing in it.
* #9818 is WAY more possible now.

## PR Checklist
* [x] Surprisingly there wasn't ever a card or issue for this one. This was only ever a bullet point in #5000. 
* A bunch of these issues were fixed along the way, though I never intended to fix them:
  * [x] Closes #11571
  * [x] Closes #11586
  * [x] Closes #7219
  * [x] Closes #11067
  * [x] I think #11623 actually ended up resolving this one, but I'm double tapping on it here: Closes #5703
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

Along the way I tried to clean up code where possible, but not too agressively. 

I didn't end up converting the various `MockTerminalSettings` classes used in tests to the x macros quite yet. I wanted to merge this with #11416 in `main` before I went too crazy.

## Validation Steps Performed

* [x] Scheme previewing works
* [x] Adjusting the font size works
* [x] focused/unfocused appearances still work
* [x] mouse-wheeling opacity still works
* [x] acrylic & cleartype still does the right thing
* [x] saving the settings still works
* [x] going wild on sliding the opacity slider in the settings doesn't crash the terminal
* [x] toggling retro effects with a keybinding still works
* [x] toggling retro effects with the command palette works
* [x] The matrix of (`useAcrylic(true,false)`)x(`opacity(50,100)`)x(`antialiasingMode(cleartype, grayscale)`) works as expected. Slightly changed, falls back to grayscale more often, but looks more right.
@zadjii-msft zadjii-msft removed this from the Terminal v2.0 milestone Jan 4, 2022
@zadjii-msft zadjii-msft added this to the 22H1 milestone Jan 4, 2022
@j4james j4james mentioned this issue Jan 30, 2022
@patrikhuber
Copy link

@patrikhuber patrikhuber commented Mar 12, 2022

I just tried to drag a tab from one WT to another, fully expecting it to work. And I ended up here! :-) Another big thumbs up from me for this feature, looking forward to it, it will be really useful.

@lipanpan-hub
Copy link

@lipanpan-hub lipanpan-hub commented Apr 1, 2022

期待这个功能的进展

@MattBDev
Copy link

@MattBDev MattBDev commented May 10, 2022

I hope that this feature will not be limited to Windows 11 and will be backwards compatible with Windows 10. A lot of businesses and users are still on Windows 10 and likely will be for the time being.

@zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented May 11, 2022

@MattBDev Don't worry, no part of how I plan on implementing tearoff is restricted to Windows 11. Something quite dramatic would have to come up in the next couple months to change that.

DHowett pushed a commit that referenced this issue May 18, 2022
A bad merge, that actually revealed a horrible bug.

There was a secret conflict between the code in #12526 and #12515. 69b77ca was a bad merge that hid just how bad the issue was. Fixing the one line `nullptr`->`this` in `InteractivityFactory` resulted in a window that would flash uncontrollably, as it minimized and restored itself in a loop. Great. 

This can seemingly be fixed by making sure that the conpty window is initially created with the owner already set, rather than relying on a `SetParent` call in post. This does pose some complications for the #1256 future we're approaching. However, this is a blocking bug _now_, and we can figure out the tearout/`SetParent` thing in post. 

* fixes #13066.
* Tested with the script in that issue.
* Window doesn't flash uncontrollably.
* `gci | ogv` still works right
* I work here.
* Opening a new tab doesn't spontaneously cause the window to minimize
* Restoring from minimized doesn't yeet focus to an invisible window
* Opening a new tab doesn't yeet focus to an invisible window
* There _is_ a viable way to call `GetAncestor` s.t. it returns the Terminal's hwnd in Terminal, and the console's in Conhost


The `SW_SHOWNOACTIVATE` change is also quite load bearing. With just `SW_NORMAL`, the pseudo window (which is invisible!) gets activated whenever the terminal window is restored from minimized. That's BAD.


There's actually more to this as well. 


Calling `SetParent` on a window that is `WS_VISIBLE` will cause the OS to hide the window, make it a _child_ window, then call `SW_SHOW` on the window to re-show it. `SW_SHOW`, however, will cause the OS to also set that window as the _foreground_ window, which would result in the pty's hwnd stealing the foreground away from the owning terminal window. That's bad.

`SetWindowLongPtr` seems to do the job of changing who the window owner is, without all the other side effects of reparenting the window. 

Without `SetParent`, however, the pty HWND is no longer a descendant of the Terminal HWND, so that means `GA_ROOT` can no longer be used to find the owner's hwnd. For even more insanity, without `WS_POPUP`, none of the values of `GetAncestor` will actually get the terminal HWND. So, now we also need `WS_POPUP` on the pty hwnd. To get at the Terminal hwnd, you'll need

```c++
GetAncestor(GetConsoleWindow(), GA_ROOTOWNER)
```
DHowett pushed a commit that referenced this issue May 19, 2022
A bad merge, that actually revealed a horrible bug.

There was a secret conflict between the code in #12526 and #12515. 69b77ca was a bad merge that hid just how bad the issue was. Fixing the one line `nullptr`->`this` in `InteractivityFactory` resulted in a window that would flash uncontrollably, as it minimized and restored itself in a loop. Great.

This can seemingly be fixed by making sure that the conpty window is initially created with the owner already set, rather than relying on a `SetParent` call in post. This does pose some complications for the #1256 future we're approaching. However, this is a blocking bug _now_, and we can figure out the tearout/`SetParent` thing in post.

* fixes #13066.
* Tested with the script in that issue.
* Window doesn't flash uncontrollably.
* `gci | ogv` still works right
* I work here.
* Opening a new tab doesn't spontaneously cause the window to minimize
* Restoring from minimized doesn't yeet focus to an invisible window
* Opening a new tab doesn't yeet focus to an invisible window
* There _is_ a viable way to call `GetAncestor` s.t. it returns the Terminal's hwnd in Terminal, and the console's in Conhost

The `SW_SHOWNOACTIVATE` change is also quite load bearing. With just `SW_NORMAL`, the pseudo window (which is invisible!) gets activated whenever the terminal window is restored from minimized. That's BAD.

There's actually more to this as well.

Calling `SetParent` on a window that is `WS_VISIBLE` will cause the OS to hide the window, make it a _child_ window, then call `SW_SHOW` on the window to re-show it. `SW_SHOW`, however, will cause the OS to also set that window as the _foreground_ window, which would result in the pty's hwnd stealing the foreground away from the owning terminal window. That's bad.

`SetWindowLongPtr` seems to do the job of changing who the window owner is, without all the other side effects of reparenting the window.

Without `SetParent`, however, the pty HWND is no longer a descendant of the Terminal HWND, so that means `GA_ROOT` can no longer be used to find the owner's hwnd. For even more insanity, without `WS_POPUP`, none of the values of `GetAncestor` will actually get the terminal HWND. So, now we also need `WS_POPUP` on the pty hwnd. To get at the Terminal hwnd, you'll need

```c++
GetAncestor(GetConsoleWindow(), GA_ROOTOWNER)
```

(cherry picked from commit 77215d9)
Service-Card-Id: 82170678
Service-Version: 1.14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issue-Feature Product-Terminal
Projects
Specification Tracker
  
Spec Accepted! 🎉
Development

No branches or pull requests