Add support for naming windows with the `-w` parameter #9300

zadjii-msft · 2021-02-26T11:34:51Z

This finishes the implementation of --window to also accept a string
as the "name" of the window. So you can say

wt -w foo new-tab
wt -w foo split-pane

and have both those commands execute in the same window, the one named
"foo". This is just slightly more ergonomic than manually using the IDs
of windows. In the future, I'll be working on renaming windows, and
displaying these names.

--window,-w <window-id>

Run these commands in the given Windows Terminal session. This enables opening
new tabs, splits, etc. in already running Windows Terminal windows.

If window-id is 0, run the given commands in the current window.

If window-id is a negative number, or the reserved name new, run the
commands in a new Terminal window.

If window-id is the ID or name of an existing window, then run the
commandline in that window.

If window-id is not the ID or name of an existing window, create a new
window. That window will be assigned the ID or name provided in the
commandline. The provided subcommands will be run in that new window.

If window-id is omitted, then obey the value of windowingBehavior when
determining which window to run the command in.

Before this PR, I think we didn't actually properly support assigning
the id with wt -w 12345. If 12345 didn't exist, it would make a new
window, but just assign it the next id, not assign it 12345.

References

Validation Steps Performed

Ran tests
Messed with naming windows, working as expected.

Closes https://github.com/microsoft/terminal/projects/5#card-51431478

The history of this had gotten way, way too long. It included everything since I started working on this

(cherry picked from commit fa26f7f)

carlos-zamora

Just a thought (for after this PR), what if we had window names appear as the window titles? That'd be kinda neat.

Most of my comments are small things, honestly.

src/cascadia/TerminalApp/AppLogic.cpp

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

src/cascadia/Remoting/WindowManager.cpp

DHowett · 2021-03-12T18:41:27Z

Testing- can I leave comments? Michael can't.

miniksa · 2021-03-12T19:16:55Z

src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp

@@ -1595,58 +1595,129 @@ namespace TerminalAppLocalTests
    {
        {
            std::vector<winrt::hstring> args{ L"wt.exe" };
-            VERIFY_ARE_EQUAL(WindowingBehaviorUseNew,
-                             appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew));
+            auto result = appImpl::AppLogic::_doFindTargetWindow({ args }, WindowingMode::UseNew);


It might be helpful to a future debugger to leave either a general comment or a Log comment per block here as to the general gist of each test section.

miniksa · 2021-03-12T19:17:53Z

src/cascadia/Remoting/Monarch.cpp

+            try
+            {
+                auto otherName = p.WindowName();
+                if (otherName == name)


Case sensitive compare? Are you sure? Lots of people love to be lazy on the capitalization when doing a command line thing and I suspect that might apply to -w.

src/cascadia/Remoting/Monarch.cpp

src/cascadia/Remoting/FindTargetWindowArgs.h

src/cascadia/Remoting/Monarch.cpp

DHowett · 2021-03-15T17:15:59Z

src/cascadia/Remoting/Monarch.cpp

            if (auto targetPeasant{ _getPeasant(windowID) })
            {
                auto result{ winrt::make_self<Remoting::implementation::ProposeCommandlineResult>(false) };
-
+                result->WindowName(targetWindowName);


Interesting. We look up the ID from the name, and then .. tell it that the window name is the name we got the ID from?

huh. I suppose that only really applies in the "we failed to propose the commandline, make a new window instead" case. I'll add a test

src/cascadia/TerminalApp/AppCommandlineArgs.h

src/cascadia/TerminalApp/AppCommandlineArgs.cpp

DHowett · 2021-03-15T17:24:28Z

src/cascadia/TerminalApp/AppLogic.cpp

+                if (parsedTarget == "new")
+                {
+                    return winrt::make<FindTargetWindowResult>(WindowingBehaviorUseNew);
+                }
+                else if (parsedTarget == "last")
+                {
+                    return winrt::make<FindTargetWindowResult>(WindowingBehaviorUseExisting);
+                }


should we lift these somewhere, or map them somehow?

should we reserve _ names here?

Oh like, just always return UseNew for those? That's an interesting idea - I was figuring we wouldn't do anything special for now, and just document "We reserve the right to use names starting with _ for whatever we want in the future. Don't use '_' as the first character of a window name in any scripts"

Eh, idc. Just a thought.

DHowett · 2021-03-15T17:25:55Z

src/cascadia/UnitTests_Remoting/RemotingTests.cpp

+        // to remove 1 while looking for "two". Technically, we shouldn't be
+        // relying on any sort of ordering for an unordered_map iterator, but
+        // this one just so happens to work.
+        VERIFY_ARE_EQUAL(p2->GetID(), m0->_lookupPeasantIdForName(L"two"));


Actually, I'd rather we search for one -- that will guarantee that we trip over its corpse.

oh, the point of this test is to check if looking for a peasant that's still alive correctly prunes the dead peasant. LookupNamedPeasantWhenItDied does exactly what you're looking for 😉

DHowett · 2021-03-15T17:26:38Z

src/cascadia/inc/WindowingBehavior.h

+constexpr int32_t WindowingBehaviorUseNew{ -1 };
+constexpr int32_t WindowingBehaviorUseExisting{ -2 };
+constexpr int32_t WindowingBehaviorUseAnyExisting{ -3 };
+constexpr int32_t WindowingBehaviorUseName{ -4 };


scary! What if they pass -w -4?

IIRC, for negative numbers, we figure out that it's a number, then clamp the number to [-1, INT_MAX], then return it, so we never parse -4 as a string

…dows-3 # Conflicts: # src/cascadia/Remoting/FindTargetWindowArgs.h # src/cascadia/Remoting/ProposeCommandlineResult.h

add a test, and add liberal comments.

src/cascadia/UnitTests_Remoting/RemotingTests.cpp

DHowett · 2021-03-17T14:09:56Z

src/cascadia/UnitTests_Remoting/RemotingTests.cpp

+    void RemotingTests::_findTargetWindowByNameHelper(const winrt::Windows::Foundation::IInspectable& /*sender*/,
+                                                      const winrt::Microsoft::Terminal::Remoting::FindTargetWindowArgs& args)
+    {
+        const auto arguments = args.Args().Commandline();


the args' args' args, we store in arguments. Not a nit, just a fun read.

DHowett

Nailed my concerns down and then some. Thanks!

ghost · 2021-03-17T19:27:58Z

Hello @DHowett!

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.

## Summary of the Pull Request This is a follow up to #9300. Now that we have names on our windows, it would be nice to see who is named what. So this adds two actions: * `identifyWindow`: This action will pop up a little toast (#8592) displaying the name and ID of the window, and is bound by default. ![identify-window-toast-000](https://user-images.githubusercontent.com/18356694/111529085-bf710580-872f-11eb-8880-b0b617596cfc.gif) * `identifyWindows`: This action will request that ALL windows pop up that toast. This is meant to feel like the "Identify" button on the Windows display settings. However, sometimes, it's wonky. ![teaching-tip-dismiss-001](https://user-images.githubusercontent.com/18356694/111529292-fe06c000-872f-11eb-8d4a-5688e4ce1175.gif) That's being tracked upstream on microsoft/microsoft-ui-xaml#4382 Because it's so wonky, we won't bind that by default. Maybe if we get that fixed, then we'll change the default binding from `identifyWindow` to `identifyWindows` ## References ## PR Checklist * [x] Closes https://github.com/microsoft/terminal/projects/5#card-51431492 * [x] I work here * [x] Tests added/passed * [ ] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments You may note that there are some macros to make interacting with lots and lots of actions easier. There's a lot of boilerplate whenever you need to make a new action, so I thought: "Can we make that easier?" Turns out you can make it a _LOT_ easier, but that work is still behind another PR after this one. Get excited

ghost · 2021-04-14T17:09:33Z

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

Handy links:

ghost · 2021-05-25T19:06:15Z

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

Handy links:

zadjii-msft added 5 commits February 25, 2021 10:39

Rebase all the changes on main

1c7da00

The history of this had gotten way, way too long. It included everything since I started working on this

fix a bug and fix the tests

97818c6

fix tests

2ec9415

Good ole Java

e13e1e7

finish that test

136ce6d

zadjii-msft added Area-Commandline wt.exe's commandline arguments Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Feb 26, 2021

zadjii-msft added this to the Terminal v1.8 milestone Feb 26, 2021

THIS NEEDS TO GO TO THE PARENT

27d12a2

(cherry picked from commit fa26f7f)

carlos-zamora approved these changes Mar 3, 2021

View reviewed changes

zadjii-msft self-assigned this Mar 3, 2021

feedback from review

c202257

zadjii-msft removed their assignment Mar 4, 2021

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

ghost requested review from miniksa, DHowett, leonMSFT and PankajBhojwani March 4, 2021 15:19

miniksa approved these changes Mar 12, 2021

View reviewed changes

DHowett requested changes Mar 15, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Mar 15, 2021

zadjii-msft added 3 commits March 16, 2021 16:03

initial review feedback

eaaa204

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

9f54229

…dows-3 # Conflicts: # src/cascadia/Remoting/FindTargetWindowArgs.h # src/cascadia/Remoting/ProposeCommandlineResult.h

Address PR concerns

bba09e3

add a test, and add liberal comments.

zadjii-msft assigned DHowett Mar 17, 2021

DHowett reviewed Mar 17, 2021

View reviewed changes

src/cascadia/UnitTests_Remoting/RemotingTests.cpp Outdated Show resolved Hide resolved

DHowett reviewed Mar 17, 2021

View reviewed changes

That's better

1105328

DHowett approved these changes Mar 17, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 17, 2021

ghost merged commit 43c469f into main Mar 17, 2021

ghost deleted the dev/migrie/f/name-windows-3 branch March 17, 2021 19:28

zadjii-msft mentioned this pull request Mar 17, 2021

Add an action for identifying windows #9523

Merged

4 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for naming windows with the `-w` parameter #9300

Add support for naming windows with the `-w` parameter #9300

zadjii-msft commented Feb 26, 2021 •

edited by DHowett

carlos-zamora left a comment

DHowett commented Mar 12, 2021

miniksa Mar 12, 2021

miniksa Mar 12, 2021

DHowett Mar 15, 2021

zadjii-msft Mar 16, 2021

DHowett Mar 15, 2021

DHowett Mar 15, 2021

zadjii-msft Mar 16, 2021

DHowett Mar 17, 2021

DHowett Mar 15, 2021

zadjii-msft Mar 16, 2021

DHowett Mar 15, 2021

zadjii-msft Mar 16, 2021

DHowett Mar 17, 2021

DHowett left a comment

ghost commented Mar 17, 2021

ghost commented Apr 14, 2021

ghost commented May 25, 2021

Add support for naming windows with the -w parameter #9300

Add support for naming windows with the -w parameter #9300

Conversation

zadjii-msft commented Feb 26, 2021 • edited by DHowett

--window,-w <window-id>

References

Validation Steps Performed

carlos-zamora left a comment

Choose a reason for hiding this comment

DHowett commented Mar 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Mar 17, 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.

ghost commented Apr 14, 2021

ghost commented May 25, 2021

Add support for naming windows with the `-w` parameter #9300

Add support for naming windows with the `-w` parameter #9300

zadjii-msft commented Feb 26, 2021 •

edited by DHowett

`--window,-w <window-id>`

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.