-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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 running a commandline in another WT window #8898
Conversation
…ompile a String[]
…!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
…safely tear it down. It _seems_ like it.
…ndow should be. It just always says 0 for now, but in the future it could actually give us useful info.
@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? |
@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. |
@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 |
@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 |
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// If we fail to find the monarch, | ||
// stay in this jail until we do. | ||
TraceLoggingWrite(g_hRemotingProvider, | ||
"WindowManager_ExceptionInCtor", | ||
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
givenID = result.Id().Value(); | ||
} | ||
|
||
// TraceLogging doesn't have a good solution for logging an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not only that, the trace logging macros explicitly tell you "DO NOT BE CREATIVE". So this is fine.
// 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, well, with this explanation, maybe we don't need a timeout on the other one?
Hello @zadjii-msft! Because this pull request has the 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 (
|
} | ||
catch (...) | ||
{ | ||
TraceLoggingWrite(g_hRemotingProvider, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the fallback error handler should deal with this, but.. wecan do it ourselves too :)
// 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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this actually work? if StartingDirectory() is an absolute path or UNC path? what about if it's ""
, the empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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
🎉 Handy links: |
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:
--window,-w <window-id>
argument towt.exe
, to allow a commandline to be given to another window.window-id
is0
, run the given commands in the current window.window-id
is a negative number, run the commands in a new Terminal window.window-id
is the ID of an existing window, then run the commandline in that window.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.window-id
is omitted, then create a new window.References
PR Checklist
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 changeThere are limitations, and there are plenty of things to work on in the future:
-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.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
GetID
,GetPID
calls that aren't try/caught 😬Monarch.cpp
Peasant.cpp
WindowManager.cpp
AppHost.cpp
wt -w 2
, and2
is hung, then does the monarch get hung waiting on the hung peasant?