Add support for running a commandline in another WT window by zadjii-msft · Pull Request #8898 · microsoft/terminal

zadjii-msft · 2021-01-26T17:38:18Z

Summary of the Pull Request

If you're reading this PR and haven't signed off on #8135, go there first.

This provides the basic parts of the implementation of #4472. Namely:

We add support for the --window,-w <window-id> argument to wt.exe, to allow a commandline to be given to another window.
- If window-id is 0, run the given commands in the current window.
- If window-id is a negative number, run the commands in a new Terminal window.
- If window-id is the ID of an existing window, then run the commandline in that window.
- If window-id is not the ID of an existing window, create a new window. That window will be assigned the ID provided in the commandline. The provided subcommands will be run in that new window.
- If window-id is omitted, then create a new window.

References

Spec: Spec for Windows Terminal Window Management #8135
Megathread: Scenario: Windows Terminal 2.0 Process Model Improvements #5000
Project: projects/5

PR Checklist

Closes Add support for wt.exe to run commands in an existing Terminal Window #4472
I work here
Tests added/passed
Requires documentation to be updated - sure does

Detailed Description of the Pull Request / Additional comments

Note that wt -w 1 -d c:\foo cmd.exe does work, by causing window 1 to change

There are limitations, and there are plenty of things to work on in the future:

We don't support names for windows yet
We don't support window glomming by default, or a setting to configure what happens when -w is omitted. I thought it best to lay the groundwork first, then come back to that.
-w 0 currently just uses the "last activated" window, not "the current". There's more follow-up work to try and smartly find the actual window we're being called from.
Basically anything else that's listed in projects/5.

I'm cutting this PR where it currently is, because this is already a huge PR. I believe the remaining tasks will all be easier to land, once this is in.

Validation Steps Performed

I've been creating windows, and closing them, and running cmdlines for a while now. I'm gonna keep doing that while the PR is open, till no bugs remain.

TODOs

There are a bunch of GetID, GetPID calls that aren't try/caught 😬
- Monarch.cpp
- Peasant.cpp
- WindowManager.cpp
- AppHost.cpp
If the monarch gets hung, then you can't launch any Terminals 😨 We should handle this gracefully.
- Proposed idea: give the Monarch some time to respond to a proposal for a commandline. If there's no response in that timeframe, this window is now a hermit, outside of society entirely. It can't be elected Monarch. It can't receive command lines. It has no ID.
  - Could we gracefully recover from such a state? maybe, probably not though.
  - Same deal if a peasant hangs, it could end up hanging the monarch, right? Like if you do wt -w 2, and 2 is hung, then does the monarch get hung waiting on the hung peasant?
- After talking with @miniksa, we're gonna punt this from the initial implementation. If people legit hit this in the wild, we'll fix it then.

…ompile a String[]

…then back!

…!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

…safely tear it down. It _seems_ like it.

….dll

…galactic-empire

…ndow should be. It just always says 0 for now, but in the future it could actually give us useful info.

awakecoding · 2021-02-08T15:41:39Z

@zadjii-msft I would be interested in reparenting Windows Terminal inside another Win32 window, either with the full terminal or just a single tab. We've been doing this successfully in the past in Remote Desktop Manager with powershell.exe and regular console programs, but our generic reparenting code doesn't work with wt.exe. I know this is a separate feature request, but it would be related to the current one as a further improvement. A quick search for "reparenting" didn't yield interesting tickets, would you know if there is a ticket already covering this feature request?

zadjii-msft · 2021-02-08T15:47:55Z

@awakecoding the closest thing to that discussion we've had is over in #7084. That's kinda what you're looking for? If it's not, feel free to open a new issue to track that discussion.

jsoref · 2021-02-08T18:18:10Z

@zadjii-msft: you're going to need to merge w/ main and resolve the file rename (sorry).

Also, did you want someone to fix the case on those PNGs?

zadjii-msft · 2021-02-08T18:25:22Z

@jsoref yea I know, thanks. I figured I'd let whoever the second is on this PR give a chance to review it so I only need so switch branches once. Guess I'm just being lazy lol.

I don't really mind the PNG thing personally, I suppose I was mostly just worried that the change from PNG to png would get Windows all confused, that the files aren't actually different. I guess I didn't know if that mattered with extensions or just filenames. I might be totally misremembering though. If you want to do it, feel free to submit a PR and I'll happily sign off ☺️

jsoref · 2021-02-09T04:46:55Z

I think it's actually dangerous to change just the case of a filename -- so best practice is to pick a different filename (in addition to switching to lowercase): https://stackoverflow.com/questions/1793735/change-case-of-a-file-on-windows/15640313#15640313

…t-galactic-empire

miniksa

Good enough. There's a lot of moving parts here. I left some brief comments, but I don't think it's anything that can't be solved by adding to your TODO pile. We need SOMETHING to build from to get this done, and this looks like a decent enough base.

src/cascadia/Remoting/Monarch.idl

miniksa · 2021-02-09T22:50:35Z

src/cascadia/Remoting/WindowManager.cpp

+                // If we fail to find the monarch,
+                // stay in this jail until we do.
+                TraceLoggingWrite(g_hRemotingProvider,
+                                  "WindowManager_ExceptionInCtor",
+                                  TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE));


I agree. I got bit in the butt in several things about rendering with having an infinite retry loop. I think you should try 5-10 times with perhaps a wait/delay between them and if you still can't do it, give up.

miniksa · 2021-02-09T23:00:33Z

src/cascadia/Remoting/WindowManager.cpp

+                givenID = result.Id().Value();
+            }
+
+            // TraceLogging doesn't have a good solution for logging an


Not only that, the trace logging macros explicitly tell you "DO NOT BE CREATIVE". So this is fine.

miniksa · 2021-02-09T23:02:38Z

src/cascadia/Remoting/WindowManager.cpp

-        // TODO:projects/5 Spawn a thread to wait on the monarch, and handle the election
+        // Try to add us to the monarch. If that fails, try to find a monarch
+        // again, until we find one (we will eventually find us)
+        while (true)


Hm, well, with this explanation, maybe we don't need a timeout on the other one?

ghost · 2021-02-10T11:28:03Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett · 2021-02-10T18:26:06Z

src/cascadia/Remoting/Monarch.cpp

+        }
+        catch (...)
+        {
+            TraceLoggingWrite(g_hRemotingProvider,


the fallback error handler should deal with this, but.. wecan do it ourselves too :)

DHowett · 2021-02-10T18:39:08Z

src/cascadia/TerminalApp/TerminalPage.cpp

+            // restored the CWD to it's original value.
+            std::wstring cwdString{ wil::GetCurrentDirectoryW<std::wstring>() };
+            std::filesystem::path cwd{ cwdString };
+            cwd /= settings.StartingDirectory().c_str();


Will this actually work? if StartingDirectory() is an absolute path or UNC path? what about if it's "", the empty string?

https://en.cppreference.com/w/cpp/filesystem/path/append

// On Windows, path("foo") / "C:/bar"; // the result is "C:/bar" (replaces) path("foo") / "C:"; // the result is "C:" (replaces) path("C:") / ""; // the result is "C:" (appends, without separator) path("C:foo") / "/bar"; // yields "C:/bar" (removes relative path, then appends) path("C:foo") / "C:bar"; // yields "C:foo/bar" (appends, omitting p's root-name)

Dustin L. Howett (3) * Move CharToKeyEvents (and friends) into InteractivityBase (GH-9106) * Update Cascadia Code to 2102.03 (GH-9088) * verison: bump to 1.7 on main Josh Soref (1) * ci: update to Spell check to 0.0.17a (CC-9014) Leonard Hecker (3) * Fixed GH-5205: Ctrl+Alt+2 doesn't send ^[^@ (CC-5272) * Fix issues in tests.xml and OpenConsole.psm1 (CC-9011) * Fix GH-8458: Handle all Ctrl-key combinations (CC-8870) Mike Griese (1) * Add support for running a commandline in another WT window (GH-8898) Michael Niksa (1) * Teach the renderer to keep thread alive if engine requests it (GH-9091) Lachlan Picking (1) * Fix shader time input (CC-8994) PankajBhojwani (1) * Separate runtime TerminalSettings from profile-TerminalSettings (CC-8602) Chester Liu (2) * Add support for paste filtering and bracketed paste mode (CC-9034) * Add support for chaining OSC 10-12 (CC-8999) Related work items: MSFT-31692939

Adds support for the `windowingBehavior` global setting. This setting controls how mutiple instances of `wt` behave in the absence of the `-w` parameter. This setting has three values: * `"useNew"`: (default) Multiple `wt` invocations (without the `-w` param) always create new windows. * `"useAnyExisting"`: When starting a new `wt`, we'll instead default to any existing windows. `wt -w -1` will still create new windows. * `"useExisting"`: Similar to `useAnyExisting`, but limits to windows on the current desktop. The IVirtualDesktopManager interface is _very_ limited. Hence why we have to track the HWNDs manually, and ask if they're on the current desktop. ## Validation Steps Performed I've been playing with it for a week now. References #5000 References projects/5 References #8898 Spec'd in #8135 Closes #2227 Closes https://github.com/microsoft/terminal/projects/5#card-51431448 Closes https://github.com/microsoft/terminal/projects/5#card-51431433

Finally implements the `newWindow` action. It does so by `ShellExecute`ing `wt.exe` with commandline args corresponding to the ones that would create the same `NewTerminalArgs`. This works with #8898 and #9118 to allow new windows (even with `windowingBehavior: useExisting`) This is taken from my auto-elevate branch, hence the references to elevation References #5000 References projects/5 References #8898 References #9118 Closes #1051

ghost · 2021-03-01T18:04:12Z

🎉Windows Terminal Preview v1.7.572.0 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft added 30 commits December 16, 2020 07:59

Get all the projects created and hooked up to the sln

3bef7bb

Yank all the M/P files, this builds?!

1f52d35

Shockingly, this works, it works elevated, and it works unpackaged

5a9cdc8

This won't work, but I'm committing this becaus I finally got it to c…

36539cf

…ompile a String[]

whoop, we pass the commandline from the peasant, to the monarch, and …

27ace16

…then back!

HOLY SHIT I GOT THE COMANDLINE TO EXECUTE IN THE CURRENT WINDOW!!!!!!…

9a41647

…!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

add a note to future me

5cabcfb

This works as a unittest, but not a local test. That's batty

03bfc6e

this macro makes me feel dirty

590b9ff

LocalTests_Remoting -> UnitTests_Remoting

0579b24

finish renaming this test

a3faed6

Clean up a lot for review

9c6eac4

cleanup Peasant for review

0103331

Final cleanup for review

b4fe1bf

_final_ final cleanup for review

d08e65c

pr nits

e101efd

This seems to work to create a thread listening for the monarch, and …

e1402d8

…safely tear it down. It _seems_ like it.

it's hard to believe he's gone / were it so easy

f978a9c

Oh yea actually remove the thing

5939636

last commit before the holidays

c088895

Merge remote-tracking branch 'origin/main' into dev/migrie/f/remoting…

fa2df47

….dll

Enable audit mode

0f5c24f

nits

921d915

Merge branch 'dev/migrie/f/remoting.dll' into dev/migrie/f/the-first-…

00184e7

…galactic-empire

Ask the TerminalApp to parse the commandline, and tell us what the wi…

658db6b

…ndow should be. It just always says 0 for now, but in the future it could actually give us useful info.

Holy bajesus, this works like a charm

977db46

Add a note about commiting suicide

bcbef34

notes, comments, cleanup

813dbc6

activate windows

0f0df5e

add some tracelogging

9fc2f0e

j4james mentioned this pull request Feb 6, 2021

Add protocol handler to open new tab in existing Windows Terminal #9061

Closed

zadjii-msft mentioned this pull request Feb 8, 2021

Add Protocol (URI) Activation #4842

Open

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Feb 8, 2021

ghost requested review from PankajBhojwani, leonMSFT and miniksa February 8, 2021 22:26

Merge remote-tracking branch 'origin/main' into dev/migrie/f/the-firs…

c29fbe7

…t-galactic-empire

miniksa approved these changes Feb 9, 2021

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 10, 2021

ghost merged commit 03ebe51 into main Feb 10, 2021

ghost deleted the dev/migrie/f/the-first-galactic-empire branch February 10, 2021 11:28

schlamar mentioned this pull request Feb 10, 2021

Shell extension "Open Windows Terminal here" should start in current window #9098

Closed

DHowett reviewed Feb 10, 2021

View reviewed changes

ghost removed the Needs-Second It's a PR that needs another sign-off label Feb 10, 2021

DHowett reviewed Feb 10, 2021

View reviewed changes

zadjii-msft mentioned this pull request Feb 11, 2021

Add support for the windowingBehavior setting #9118

Merged

This was referenced Feb 18, 2021

Add support for the newWindow action #9208

Merged

Scenario: Windows Terminal 2.0 Process Model Improvements #5000

Closed

ghost mentioned this pull request Mar 1, 2021

Add support for wt.exe to run commands in an existing Terminal Window #4472

Closed

zadjii-msft mentioned this pull request Apr 7, 2021

Setting startingDirectory to WSL path causes indefinite application hang #9541

Closed

This pull request was closed.

Conversation

zadjii-msft commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

TODOs

Uh oh!

awakecoding commented Feb 8, 2021

Uh oh!

zadjii-msft commented Feb 8, 2021

Uh oh!

jsoref commented Feb 8, 2021

Uh oh!

zadjii-msft commented Feb 8, 2021

Uh oh!

jsoref commented Feb 9, 2021

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miniksa Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

miniksa Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

miniksa Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

ghost commented Feb 10, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

DHowett Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

zadjii-msft commented Jan 26, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.