search

microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95k stars 8.22k forks source link

Scenario: Windows Terminal 2.0 Process Model Improvements #5000

Closed zadjii-msft closed 1 year ago

zadjii-msft commented 4 years ago

This scenario is tracking a number of enhancements we'd like to make to the fundamental process model of the Windows Terminal in v2.0. The following deliverables all have something to do with the way the Terminal is launched, or the way connections or instances are managed, and as such, the solution to any one of these areas should make sure to take into account the others.

Window / Session Managment

Elevation

Default Terminal

Specs

TODOs

These are also tracked in https://github.com/microsoft/terminal/projects/5. This provides a nested list, to mentally track which things are dependent upon other things. This is basically a copy of a page of my notebook.

Window Management * [x] Window Management, PR: #8898 * [ ] Next/Prev window keybindings * [x] Windowing Behavior, PR: #9118 * [ ] Smart `wt -w 0` handling * [x] Name windows on commandline * [x] `nameWindow` action (#9662) * [x] `openWindowRenamer` action (#9662) * [x] window-level toast for displaying window ID, action: `idenfifyWindows` (#9523) - Originally I thought I'd need generic toasts for this, but I'm gonna just do this first, and come back to generic toasts * [x] Quake Mode has been moved to #8888 * [x] New Window Action PR: #9208

Tear Out

Currently planned state, notes:

See https://github.com/microsoft/terminal/issues/5000#issuecomment-1407110045 for the most up-to-date todo list. There were some dramatic changes to our plans in early 2023, which basically entirely obsoleted "Process Model 2" (where each termcontrol is its own process) in favor of "Process Model 3" (where all windows are all one process).

Original tear-out plans * [ ] `wt.exe` accepts `--content guid` on the commandline to start in content mode. Additionally, Make the `Control` able to have the `Core` OOP. * these two were originally different bullet points, but I think they need to be combined into one singular PR. They don't make sense alone. - [x] in content process mode, it registers `ContentProcess` for that GUID with `CoRegisterClassObject` - [x] `ContentProcess::Initialize(IControlSettings, IConnection)` instantiates a new `ControlInteractivity` (in the content process) (if one hasn't already been initialized) - [x] `ContentProcess::RequestSwapChainHandle(pid)` will duplicate the handle if the pid is different - [x] `wt` properly tracks the lifetime of its content. When that's discarded by the last window process, we'll exit. - [x] Do the above line, but better. Now we're using a lock and an event, which is _ew_. Instead, we should do the Conhost `ExitThread()` thing on the main thread, and then ExitProcess() when the content process is dtor'd. - [x] Move all the chaos I've introduced in `main.cpp` into its own dedicated file/class or something. - [x] An embedding process needs a way to know that the content process is ready, better than a `Sleep(2000)` - A handle to an event is probably the cleanest way - have the window create the event, then pass the handle in `CreateProcess` & on the commandline to `wt --content {guid} --signal {handle}`, then have the content process set the event when it's ready. - 📝 in ~`dev/migrie/oop/the-whole-thing`~ `dev/migrie/oop/infinity-war` - [x] The `Connection` is made in the core - [x] When the Core signals that the swapchain changed, and the TermControl asks what the swapchain handle is, the Control passes in it's PID. If the PID isn't the same, `DuplicateHandle` the swapchain to that PID. - [x] The sample is updated with a version of the Control that's also OOP - [x] If the content process dies, the control needs to be able to display a message box. - [x] Make sure UIA signaling still works (`accevent.exe` in the SDK, `C:\Program Files (x86)\Windows Kits\10\bin\10.0.19041.0\x64`) - [x] Make sure that reading the contents of the buffer with Accessibility Insights still works - 💩 - `InteractivityAutomationPeer` extends `Windows.UI.Xaml.Automation.Peers.AutomationPeer`. But we're making the interactivity peer in the content process, not in the control layer. - When we construct the `IAP`, we throw instantly, because the `WUX..AutomationPeer` can't be created off the main thread. - How do we only expose the `WUX..AutomationPeer` via `TermControl`, but not `IAP`? - [x] The `_raiseContentDied` should display the error internal to the TermControl. Kinda like the renderer warning, but for this, we need to kill the `SwapChainPanel` too. - in ~`dev/migrie/oop/the-whole-thing`~ `dev/migrie/oop/infinity-war` - Relevant diff: 23a19c58181b489b10a2049232f23b033dbdab8d...5c17603a94a76d47c4427f49493bca4eeb8fa774 - [x] Add a velocity flag to gate behind dev builds only. Probably safe to just gate the `--content` flag for now. - [ ] Guard calls to `_interactivity`, `_core` in `TermControl`. Those objects might go out of scope at any moment, so pretty much all access needs to be try/caught. - we may want to do a function-level try, with a CATCH_RAISE_DIALOG() macro we write to raise the connection died notice. Might be the most minimal diff. - Could likely be a follow up to original PR. - There's a silly idea in `dev/migrie/oop/2/cptn-marvel`. I really hate it, it's a HUGE amount of churn. - [x] **There's a lot of debug spew** that makes me think the OOP core does something off the main thread. - BriAl and I synced - this is nothing to worry about. This is basically just COM logging "i'm about to increment the refcount" - Still trying to find a viable way to suppress that logging. - [x] What the hell did I do with the automation peer. Was what was in `main` correct? Revisit the original PRs for inspiration. - Did it ever work? there's seemingly no record of it working in 23a19c58181b489b10a2049232f23b033dbdab8d...5c17603a94a76d47c4427f49493bca4eeb8fa774 - #10051 has a fairly different implementation than the above diff - I may have simply forgotten that narrator / UIA uses exceptions for control flow, which makes debugging a pain - [x] `ControlCore::_IsClosing` TODO! - [x] `ContentProcess::RequestSwapChainHandle` tracelogging TODO! - [x] `TermControlAutomationPeer::GetLocalizedControlTypeCore` that string used to come from localized resources, it can't anymore. - [x] The `Control` properly manages the lifetime of the `HANDLE` duplicated to it. A `wil::unique_handle`? - [x] **IMPORTANT**: For whatever reason, when the second window attaches to the content, it renders basically nothing. Input gets sent just fine, but the output doesn't render.. Fixed in 7b3ca8332 - 📝 Now moved to [`dev/migrie/oop/2/infinity-war`](https://github.com/microsoft/terminal/compare/dev/migrie/oop/2/infinity-war) - 👀 in #12938 * [x] `TermControl`s need to have a local `TerminalSettings` instance. (Related: #7219) - What a shockingly simple oversight. We did all this work to get the connections in the same proc as the `TermControl`, we totally forgot the Settings. - [x] Make sure updating/changing settings works fine across processes - [x] Make sure previewing the color scheme still works. That works with some parent/child trickery that won't work OOP. - 📝 in `dev/migrie/oop/ragnarok` * [ ] Some misc cleanup before endgame: - [ ] `existingConnection` in `NewTab` was redundant / unused - [ ] `_SplitPane` renamed to `_SplitPaneActiveTab` etc. - [ ] Refactoring to allow the TerminalTab to be initialized without a control - [ ] Other stuff to, to minimize the diff - not started, in `dev/migrie/oop/2/black-widow` cause this one's not really consequential, is it now. * [ ] Make the app able to do the OOP instantiation. - Notes in https://github.com/microsoft/terminal/issues/5000#issuecomment-1137653858 * [x] Make the sample able to pop a control into a new window. - ✔️ in `dev/migrie/oop/the-whole-thing`. I didn't pop it into a new window, just made it so multiple windows could connect to the same one. * [ ] Defterm is out of proc. - [ ] DEFTERM: Incoming connections need to be tossed to the `ContentProcess`. Guh. - not started in `dev/migrie/oop/2/captain-falcon`. * [x] Tabs can be serialized to a json blob, with their tab & pane structure. - See #766, because this is highly relevant - This may have already been done for me by #10972 - [x] Serialize `TerminalTab`s - [x] Serialize `SettingsTab`s * [ ] `move-pane -w ` to move a pane/tab to another Terminal window. This will prove we can move panes without the whole instantiation of a new window process. - Notes in https://github.com/microsoft/terminal/issues/5000#issuecomment-1183694124 - 📝 in [`dev/migrie/oop/wandavision`](https://github.com/microsoft/terminal/compare/81a5a736fef41bd90339c36a6cc4d7a339b6badf...dev/migrie/oop/wandavision) (link to relevant diff from `dev/migrie/oop/endgame`) * [ ] When a tab is torn out of a Terminal window, it can be dropped & re-attached onto another terminal window - Notes in https://github.com/microsoft/terminal/issues/5000#issuecomment-1185447944 - 📝 in `dev/migrie/oop/2/loki` * [ ] When a tab is torn out of a Terminal window, it can be dropped somewhere else and turned into a new terminal window - not started yet, in `dev/migrie/oop/no-way-home` * [ ] When a tab is torn out, we'll create a new window for the tabs left behind, and move the current window around. - We'll need to still be able to drop this window on an existing window somehow. - Use EvanK's prototype here. We're gonna need to extend the drag bar over the TabView and do everything ourselves - [ ] #12616 - not started yet, in `dev/migrie/oop/multiverse-of-madness`

other notes

Just moving these lower in the body to make managing this doc easier

This is a pseudo design that Dustin and I discussed. There's a lot more work that needs to be done here, but I need to save this somewhere outside of a Teams chat history ``` --- struct TerminalControl: winrt::implements {} // XAML Control // calls: interactivity.PointerMoved(Point{x, y}); // calls: interactivity.PointerClicked(Point{x, y}); // links: TerminalCore.lib <----> or consumes TerminalCore.dll from RT ^^^ TerminalControl.dll ^^^ --- struct WPFTerminalControl { HWND _h; } // calls: interactivity.PointerMoved(Point{x, y}); // calls: interactivity.PointerClicked(Point{x, y}); // links: TerminalCore.lib ^^^ WPFTerminalControl.dll ^^^ vvv TerminalCore.lib vvv // Cancelled. TerminalCore.lib or .dll struct Coord {}; struct PointerInfo {}; struct KeyInfo {}; struct TerminalControlInteractivity; struct ControlCore; // < implements ... something? // this should just be TerminalCore? struct TerminalCore; // < implements the "conhosty APIs (ITerminalDispatch, etc.) // links: dx.lib buffer.lib types.lib vvv TerminalCore.UnitTests.dll vvv // contains: interactivity tests // contains: core tests // links: TerminalCore.lib ``` * [x] Split `TerminalControl` into lib and dll * [x] Split `TermControl` into `Control.TermControl` and `Control.ControlCore` and `Control.Interactivity` * [ ] Add some `Core.ControlCore` tests from things in #7001 * [ ] Create a WinRT boundary on `Core.Interactivity` between it and `TermControl` * [ ] Enable `Core.ControlCore` to exist out-of-proc with XAML, or in-proc * This might involve creating a scratch project to consume an in-proc one and an out-of-proc one * [ ] the Core can create connections, or have them passed in. (so an OOP core can have a connection in it's own process) * [ ] Much more. This is just what's immediately on my radar.
_4-30-21_: I need to do the DX `HANDLE` thing before I can do the rest of the projection boundary. The projection boundary is in `dev/migrie/interactivity-projection-000`, which is almost done now. `dev/migrie/oop-scratch-4` has the final state of the OOP prototyping on this element. In that branch, * `HostAndProcess::CreateSwapChain` creates a swapchain with `DCompositionCreateSurfaceHandle`. * It attaches it to the `SwapChainPanel` with `ISwapChainPanelNative2::SetSwapChainHandle`. * It duplicates that handle to the content process, and calls `HostClass::ThisIsInsane` * Later on, the `HostClass` passes that handle to the `DxEngine` So we'll need to do basically that, but a little different, to handle all the actual edge cases we have.
pre-1.10 era work * [x] First, update `DxEngine` to accept the handle, but don't otherwise change anything (#10023) * ~~Then, change `TermControl` to create the handle with `DCompositionCreateSurfaceHandle`, hook it up to the `SwapChainPanel` with `ISwapChainPanelNative2::SetSwapChainHandle`.~~ - `DxEngine::_CreateDeviceResources` will need to trigger a callback in the `Core`, that will then ask the `TermControl` to create a new swapchain for it. - Wait no that's wrong. the `DxEngine` will always be making the swapchains, because it knows when it needs a new one. * [x] `DxEngine` will always make new swapchains, when needed, with `DCompositionCreateSurfaceHandle`. It'll raise an event when that happens. The `Core` will handle that, then raise an event the control will listen to. The `Control` will handle that event, then ask the `Core` for the new swapchain `HANDLE`, as a `uint64_t`. The Control will use that `uint64_t` to attach to the `SwapChainPanel` via `ISwapChainPanelNative2::SetSwapChainHandle`. - This is also the way it is in (#10023) - 👀 in #10023 * [x] Then we'll come back to the rest of the projection - 👀 in #10051 * [x] At this point, we should make a sample app that's just a grid with two spaces in it. One for an in-proc term control, and one for an out of proc one. We won't do the OOP one yet. - 👀 in #10067 * [x] Merge #10115 into #10067 * [x] Enable the Connection able to be created OOP from the rest of the TerminalPage. The content process will be the one to instantiate the connection. - [x] The Core is passed a (guid? String?) of a type of connection to spawn, and an `IConnectionSettings` object. The Core instantiates a type of connection based on that guid/string, and passes the `IConnectionSettings` from the window process to that new constructor. - [x] How does the azure connection work? I think that one's implemented in TerminalApp. If it's a String, we could pass the winrt name, `ITerminalConnection conn = RoCreateInstance("TerminalApp.AzureConnection"); conn.Create(iConnectionSettings);` , right? - No, turns out the Azure connection is in TerminalConnection. So this was actually fine the whole time. - Whatever, we're keeping the `RoGetActivationFactory` thing though, for future needs. - [ ] How does the debug tap work? - ~~Switch from `RoActivateInstance`&`Initialize` to `RoGetActivationFactory`~~ - > Even neglecting the Windows Runtime, this isn’t even possible in C++ or C#. - [x] Reattach event handlers that I disabled for performance's sake - [x] Merge #10115 into this branch, or into `dev/migrie/oop/connection-factory-rebase`, which is attempting to rebase these changes onto `dev/migrie/oop/sample-project` - 📝 in [`dev/migrie/oop/connection-factory`](https://github.com/microsoft/terminal/compare/dev/migrie/oop/the-whole-thing...dev/migrie/oop/connection-factory) * [x] 🦶 **PUNTED** ~Move the Core, the Interactivity, into `Core.dll`. We'll need that for:~ - We may not actually need this. It'd be _nice_ to not have to load _all_ the settings, but we could live with it.
conioh commented 3 years ago

Elevation

  • [x] #1032 Mixed Elevation

    • This is a hard problem, and I'm making no firm commitments that we'll be able to solve it for sure in 2.0.
    • The goal of this deliverable is to find a safe way that we can support both elevated (High-IL) and unelevated (Medium-IL) tabs, panes, etc all in the same window.
    • Ideally, if a user creates a new elevated tab from an unelevated window, then all the unelevated tabs would seamlessly appear in a new elevated HWND hosting both unelevated and elevated tabs simultaneously.
    • Elevated connections can't be hosted in an unelevated HWND, because that allows for a trivial escalation-of-privilege vector utilizing the Terminal.
    • This problem should arguably be investigated first. If this is something that's technically possible, it will certainly have an influence on the way the rest of these deliverables are architected.
    • ❌ Resolution: This is not going to be technically achievable, unfortunately.

  • [ ] #632 Open a Profile as elevated

    • If the above is not achievable technically, then the user should still be able to specify a way to force a profile to open elevated.

    • If mixed elevation isn't possible:

    • If the current window isn't currently elevated, open a new window when someone's profile is marked "elevated: true"

    • If the current window is elevated, open in the same window.

What does this mean re. https://github.com/microsoft/terminal/issues/6893#issuecomment-657554241 and https://github.com/microsoft/terminal/issues/3581#issuecomment-581624314 etc.?

Will each window belong to a different process? Otherwise I don't understand what does "elevated window" even means.

And what if "elevation" actually mean "OTS elevation" rather than "AAM elevation", since the non-elevated user is not an admin? Currently at least that works. I hope the new model won't somehow break that.

@DHowett-MSFT @miniksa

zadjii-msft commented 3 years ago

Currently, yes, each terminal window is its own process. Each WT process has one window, so when you're running WT elevated, the window itself is elevated. Unfortunately, nothing in the new process model is going to have an affect on elevated drag drop. The team that owns the platform drag/drop APIs has specifically told us that drag/drop is unsupported in an elevated HWND.

Furthermore, we're not going to be messing with mixed elevation as a part of this effort anymore, because that turned out to be technically less possible than we hoped. So nothing about that story is about to change.

conioh commented 3 years ago

Maybe I'm missing something, but I just opened two elevated instances of WordPad, and dragged some text, a picture and an OLE Paint drawing from one to the other. And inside each document.

If by "platform" you mean UWP/Store/XAML/whatever-you-call-it-today, I just launched WinDbg Preview, installed from the Store, uses XAML and all that jazz, and managed to rearrange the windows in the elevated instance by dragging.

DHowett commented 3 years ago

Specifically: "UWP XAML" (not WPF, which WinDbg Preview uses) uses the modern WinRT drag/drop APIs. Those APIs require a data broker process. That broker just doesn't work from an elevated context. Terminal, by way of it using "UWP XAML", is subject to this specific limitation.

namazso commented 3 years ago

About the Mixed Elevation part: Is there anything preventing possibility of a feature to launch medium integrity shells from high integrity wt instance? I know this is rather limited in comparison to the original plans, however should be much easier to support

gerardog commented 3 years ago

to launch medium integrity shells from high integrity wt instance

I would like to hear what others think about this approach. This can be achieved running gsudo -i medium on a wt tab or by creating a profile for it. There is also MediumPlus Integrity, which is higher than Medium (no medium process can tamper with it) but still hasn't admin acesss... (gsudo -i MediumPlus)

Rosefield commented 3 years ago

Just as something that I saw mentioned as a comment on the spec, but not incorporated, I think the idea of having a separated elevated-settings.json file makes a lot of sense as a counterpart to elevanted-state.json. This gives a number of benefits

zadjii-msft commented 3 years ago

separated elevated-settings.json file makes a lot of sense as a counterpart to elevanted-state.json.

I don't think this is a terrible idea. How do we deal with users who are currently using their unelevated settings in admin windows though? Would users need to maintain basically two copies of their settings file?

Maybe we only use the elevated-settings file if we find it? If the file doesn't exist, then just fall back to the normal settings.

I guess it might make more sense if the elevated-settings could be layered on top of settings.json (a la #2303, #6502, #2933)

Rosefield commented 3 years ago

I wasn't thinking in terms of migration as much as qol, but I did also consider layering as an approach or adding a "create from unelevated settings" option in the settings UI. I didn't think thoroughly about the security implications (neither are technically worse than what exists now, but it could be better) so I didn't want to suggest something.

andrisi commented 3 years ago

I'd say use settings.json as a base and override/merge values with anything in the elevated.json. Changed values or added sessions.

namazso commented 3 years ago

i agree, layering / overriding is the best choice, in either direction. although checking if elevated-settings.json exists, then using that and overriding with user one might make more sense so that

The file can be admin writeable only to help prevent malicious modification

is not pointless

zadjii-msft commented 2 years ago

\

📝 dev/migrie/oop/2/endgame

📝 in dev/migrie/oop/2/endgame Relevant diff (probably): 5c17603a94a76d47c4427f49493bca4eeb8fa774...81a5a736fef41bd90339c36a6cc4d7a339b6badf

_MakePane usage:

2022-06-22

No this is dumb. * `TerminalPage::_SplitPane` and `TerminalPage::_CreateNewTabFromPane` both take a `shared_ptr`, created from `_MakePane`. * Could they be trivially replaced by async versions? * `NewTabFromPane` * `resume_bg` * await `MakePane().get()` * create content process * resume_fg * create control from content * create pane *
Make content is async. Creating the pane can be sync on the main thread * `IAsyncAction _CreateContentProcess(profile, controlSettings)` * `std::shared_ptr _MakePaneFromContent(ContentProcess content, auto controlSettings, auto profile)` * // MUST BE ON FG THREAD * create control from content * create pane * `_CreateNewTabFromPane` becomes: `winrt::fire_and_forget TerminalPage::_AsyncCreateNewTab(IAsyncAction initContentProc)` * `resume_bg` * `auto content = initContentProc.get()` * `resume_fg` * `auto pane = _MakePaneFromContent()` * `tab = make_self(pane)` * `_InitializeTab(newTabImpl)` * And this is called like: `_AsyncCreateNewTab(_CreateContentProcess(profile, controlSettings));` * `_SplitPaneOnTab` becomes: `winrt::fire_and_forget TerminalPage::_AsyncSplitPaneOnTab(TerminalTab& tab, const SplitDirection splitDirection, const float splitSize, IAsyncAction initContentProc)` * calculate split type * unzoom * resume BG * `auto content = initContentProc.get()` * `resume_fg` * `auto newPane = _MakePaneFromContent()` * `tab.SplitPane(*realSplitType, splitSize, newPane);` * focus the thing * `_SplitPaneActiveTab` becomes: * if (!focusedTab) ... * `_AsyncCreateNewTab` * else * `_AsyncSplitPaneOnTab(focusedTab,...)`

2022-06-23

TODO

zadjii-msft commented 2 years ago

\

📝 dev/migrie/oop/2/wandavision

move-pane -w <id> to move a pane/tab to another Terminal window. This will prove we can move panes without the whole instantiation of a new window process. original diff at: in dev/migrie/oop/wandavision (link to relevant diff from dev/migrie/oop/endgame) diff from endgame: https://github.com/microsoft/terminal/compare/dev/migrie/oop/2/endgame...dev/migrie/oop/2/wandavision

Original prototype:

TODO

After this is all done, we definitely could plop the string into a DataPackage and unwrap that to drop tabs.

As of 35c82d37f: (commented out, there are better gifs below)

zadjii-msft commented 2 years ago
### 📝 `dev/migrie/oop/2/loki` * [x] add a `TabDragStarting` to the TabView. In there, stash the serialized tab under the "content" key * [x] `TabStripDragOver` handler on TabView - look for the "content" key, and if it's there, accept it as a move * [x] `TabView.TabStripDrop`: get the content, deserialize, and fire it off to the action handler * [x] Make sure we can call back to the original window to remove the tab. ![loki-with-detach-too](https://user-images.githubusercontent.com/18356694/179283633-c9ff1a99-4774-4890-9932-00197042895c.gif)
Suncatcher commented 2 years ago

I don't know if my issue related to the process model improvements, but I guess so. If I post it in a wrong thread, please move where needed.

Why don't you implement a per-tab execution policy for Windows Terminal? It would be very nice to have separate execution policies for different tabs, for everydays tasks Undefined policy is quite sufficient, but for some special development task like creating Python virtual environment I may need Unrestricted policy. It may be implemented by hotkey, or holding Ctrl-Shift on tab creation, whatever.

In the #632 you discussed why it is bad to have a mix of (non)elevated tabs, but here is a different thing. Changing execution policy and acquiring elevated permissions is not the same thing, and one can be done without another.

DHowett commented 2 years ago

@Suncatcher Thanks for the request. It's a good idea! The catch here, though, is that the execution policy is a property of the thing that is running inside terminal, right? I think it's actually specifically a PowerShell concept... and I don't believe there's a way using the Windows API to influence the execution policy of any instances of PowerShell under a process tree using CreateProcess (or its friends.) Even if there were, it wouldn't mean anything to cmd.exe or Cygwin or nushell or yori, simply because they don't have an equivalent concept. Perl does, somewhat, have a restricted mode. I don't know if that's equivalent... and perl doesn't see much interactive use.

The easiest way to accomplish this today is going to be to add new profiles that use PowerShell native arguments to set the execution policy you want, and open those profiles from the New Tab dropdown menu.

As an example...

image image 
            {
                "commandline": "\"C:\\Program Files\\PowerShell\\7\\pwsh.exe\" -NoExit -Command \"Set-ExecutionPolicy Restricted -Scope Process\"",
                "guid": "{d4207635-ac89-4c36-803f-7566ab0e243b}",
                "hidden": false,
                "icon": "🔐",
                "name": "PowerShell (Restricted Policy)",
                "startingDirectory": "%USERPROFILE%",
                "background": "#330000"
            },
Suncatcher commented 2 years ago

thanks, didn't know this capability to set PS arguments per profile. Will use it.

zadjii-msft commented 1 year ago

Process Model v3

Changes as of 2023

Everything is one process.

Proof-of-concept branch: https://github.com/microsoft/terminal/compare/main...dev/migrie/f/process-model-v3-test-0

Each of the headers here is an atomic PR plan, similar to the way I was doing the oop/2/ branches for PMv2. The goal being minimizing the individual code deltas.

dev/migrie/oop/3/foreword - #14825

Do this first, so that we can eventually have an AppLogic without necessitating a whole "window". Necessitated by the following train of thought:

TerminalApp::AppLogic needs to get instantiated after IslandWindow::Initialize (currently called in AppHost::Initialize) We need an AppLogic to get at the settings to see if we want a window at all. So now we need a window to determine if we need to create a window. That's stupid. Can we avoid doing any XAML work before we get at the settings? Will that work?

dev/migrie/oop/3/ainulindale #14843

https://github.com/microsoft/terminal/compare/dev/migrie/oop/3/foreword...dev/migrie/oop/3/ainulindale

What if we started somewhere sane?

dev/migrie/oop/3/valaquenta

https://github.com/microsoft/terminal/compare/dev/migrie/oop/3/ainulindale...dev/migrie/oop/3/valaquenta

dev/migrie/oop/3/quenta-silmarillion #14866

https://github.com/microsoft/terminal/compare/dev/migrie/oop/3/valaquenta...dev/migrie/oop/3/quenta-silmarillion

This would be the equivalent of dev/migrie/oop/2/wandavision, see https://github.com/microsoft/terminal/issues/5000#issuecomment-1183694124

dev/migrie/oop/3/akallabeth

https://github.com/microsoft/terminal/compare/dev/migrie/oop/3/quenta-silmarillion...dev/migrie/oop/3/akallabeth

dev/migrie/oop/3/an-unexpected-party

https://github.com/microsoft/terminal/compare/dev/migrie/oop/3/akallabeth...dev/migrie/oop/3/an-unexpected-party

zadjii-msft commented 1 year ago

In case I lose this over the long weekend:

Actual order of events I ge

sequenceDiagram
    participant Source
    participant Target

    Note Left of Source: _onTabDragStarting
    Source ->> Target: Drag tab
    Note right of Target: _onTabStripDragOver
    Target ->> Target: AcceptedOperation(DataPackageOperation::Move)
    Source --> Target: Release mouse (to drop)
    Note right of Target: _onTabStripDrop
    Target -->> Source: 
    Note Left of Source: TabViewTabDragStartingEventArgs<br>.OperationCompleted
    Note Left of Source: _onTabDroppedCompleted

You'll note: There's no message that the source gets prior to the target's TabStripDrop to confirm that the drop worked. So, we can't detach the content from the source before the target would already take it.

FURTHERMORE

There's no event in the target after the source gets the acceptance. So there's no way for the target to wait for the source to detach first.

LASTLY

There's no way for the target to communicate back to the source. The e.Data() is null on the target - only the DataView() is accessible. That's immutable (obviously). And OperationCompleted and TabDropCompleted give you didly.

The incredibly dumb solution I'm considering

sequenceDiagram
    participant Source
    participant Target
    participant Monarch

    Note Left of Source: _onTabDragStarting
    Source --> Source: stash dragged content
    Source --> Source: pack window ID into DataPackage

    Source ->> Target: Drag tab
    Note right of Target: _onTabStripDragOver
    Target ->> Target: AcceptedOperation(DataPackageOperation::Move)

    Source --> Target: Release mouse (to drop)

    Note right of Target: _onTabStripDrop
    Target --> Target: get WindowID from DataPackage
    Target -) Monarch: Request that WindowID sends content to us<br>RequestRecieveContent
    Monarch -) Source: Tell to send content to Target.Id<br>RequestSendContent, SendContent
    Source --> Source: detach our content
    Source -) Monarch: RequestMoveContent(stashed, target.id)
    Monarch -) Target: AttachContent(stashed)

    # Target -->> Source: 
    # Note Left of Source: TabViewTabDragStartingEventArgs<br>.OperationCompleted
    # Note Left of Source: _onTabDroppedCompleted

Pass the source's window ID in the package. When the target accepts it, have the target ask the monarch to ask the source to send its content to the target. Extremely dumb.

Other notes

This API is particularly painful for the kinds of UI experiences we want.

reference

zadjii-msft commented 1 year ago