Add an action for identifying windows #9523

zadjii-msft · 2021-03-17T20:03:23Z

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 ("Toast" notifications #8592) displaying the name and ID of the window, and is bound by default.
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.

That's being tracked upstream on IsLightDismissEnabled TeachingTip in Xaml Islands dismisses itself immediately if the window is unfocused when the Tip is opened 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

Closes https://github.com/microsoft/terminal/projects/5#card-51431492
I work here
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

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

WE LIVE IN A MAD WORLD. Teaching tips are exactly the UI we want, but they just don't fucking work man. WE want them to light dismiss, but if the window is inactive, and you have ILDE=true, then the tip immediately dismisses itself. So inactive windows need to not enable light dismiss. ALSO we need inactive windows to not focus something when the tip dismisses itself. Like, focus was _nowhere_ when we started. We need to toss the focus back to _nowhere_.

(cherry picked from commit aa51c07)

(cherry picked from commit fa26f7f)

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

add a test, and add liberal comments.

…Windows # Conflicts: # src/cascadia/Remoting/WindowManager.cpp # src/cascadia/TerminalApp/Resources/en-US/Resources.resw

…Windows

src/cascadia/TerminalApp/AppLogic.cpp

src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp

src/cascadia/TerminalApp/WindowNameConverter.cpp

src/cascadia/TerminalApp/Toast.h

carlos-zamora · 2021-03-18T20:35:10Z

src/cascadia/TerminalApp/TerminalPage.xaml

+                         Title="{x:Bind WindowId, Mode=OneWay, Converter={StaticResource WindowIdConverter}}"
+                         Subtitle="{x:Bind WindowName, Mode=OneWay, Converter={StaticResource WindowNameConverter}}">


Shouldn't we reverse these? Title = Name, subtitle = number? Because the user explicitly assigned a name, so we should present that information first?

Oh see, I was mostly thinking about the (majority) case where windows aren't named:

<unnamed-window>

Window: 1

vs

Window: 1

<unnamed-window>

src/cascadia/TerminalApp/AppActionHandlers.cpp

DHowett

I'm blocking on the whole mostly because of the converters -- I don't see the need to proliferate them; I'd like to reduce their use to "never" as they waste memory by simply existing (they're created as part of xaml resource allocation) -- and perhaps the lazy load dialog thing.

src/cascadia/Remoting/Monarch.cpp

src/cascadia/TerminalApp/AppActionHandlers.cpp

src/cascadia/TerminalApp/AppLogic.cpp

src/cascadia/TerminalApp/ShortcutActionDispatch.h

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalApp/WindowNameConverter.cpp

DHowett · 2021-03-19T21:40:39Z

src/cascadia/TerminalApp/Toast.cpp

+    _timer.Tick([weakThis](auto&&...) {
+        if (auto self{ weakThis.lock() })
+        {
+            self->_timer.Stop();
+            self->_tip.IsOpen(false);
+        }
+    });
+    _timer.Start();


does this ensure the timer will not fire twice overlapping?

It currently doesn't. That's a good point.

src/cascadia/WindowsTerminal/AppHost.cpp

tools/GenerateHeaderForJson.ps1

…Windows # Conflicts: # src/cascadia/TerminalApp/AppLogic.cpp # src/cascadia/TerminalApp/TerminalPage.cpp # src/cascadia/TerminalApp/TerminalPage.h

This reverts commit 8b6e0bc.

carlos-zamora

This looks great! Just added some suggestions and other thoughts.

carlos-zamora · 2021-03-29T17:28:54Z

src/cascadia/TerminalApp/TerminalPage.xaml

+                         Title="{x:Bind WindowIdForDisplay}"
+                         Subtitle="{x:Bind WindowNameForDisplay}">


Do we need Mode=OneWay here? I guess it's fine for Window ID because that only gets set once per instance, but once renaming goes in, we'll need to make Window Name Mode=OneWay, right?

I don't think so, since WindowNameForDisplay only has a getter, and I'm using a totally different toast for renaming

So, OneWay means "listen to PropertyChanged and call the Getter again when it happens"

TwoWay means "this control can call the setter"

The default is OneTime, which is "Call the getter when constructed." 😄

carlos-zamora · 2021-03-29T18:51:25Z

src/cascadia/TerminalApp/TerminalPage.cpp

+    // WindowName is a otherwise generic WINRT_OBSERVABLE_PROPERTY, but it needs
+    // to raise a PropertyChanged for WindowNameForDisplay, instead of
+    // WindowName.
+    winrt::hstring TerminalPage::WindowName() const noexcept
+    {
+        return _WindowName;
+    }
+    void TerminalPage::WindowName(const winrt::hstring& value)
+    {
+        if (_WindowName != value)
+        {
+            _WindowName = value;
+            _PropertyChangedHandlers(*this, Windows::UI::Xaml::Data::PropertyChangedEventArgs{ L"WindowNameForDisplay" });
        }
    }


You could probably set up a PropertyChangedHandler in the ctor and listen for WindowName changed, then fire a WindowNameForDisplay changed event. But like, it probably ends up being the same amount of work as this haha. This is just an option if you really wanted to keep the WINRT_OBSERVABLE_PROPERTY

Meh, yea, I decided against that b/c I thought at runtime, that would end up being less performant (raise one event, handle it, raise another), rather than just skipping the step.

carlos-zamora · 2021-03-29T18:53:49Z

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

+  </data>
+  <data name="UnnamedWindowName" xml:space="preserve">
+    <value>unnamed window</value>
+    <comment>text used to identify when a window hasn't been assigned a name by the user</comment>


Suggested change

<comment>text used to identify when a window hasn't been assigned a name by the user</comment>

<comment>Text used to identify when a window hasn't been assigned a name by the user</comment>

carlos-zamora · 2021-03-29T18:57:20Z

src/cascadia/Remoting/Monarch.cpp

+        auto callback = [](auto&& p, auto&& /*id*/) {
+            p.DisplayWindowId();
+        };


Wait... if we're not using id, should we just remove it from the first param in _forAllPeasantsIgnoringTheDead?

Mike: writes generic function

Reviewers: is it possibly too generic?

Meh, I presumed a future user of this helper might want it.

…Windows

ghost · 2021-03-30T16:08:00Z

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.

## Summary of the Pull Request This PR adds support for renaming windows. ![window-renaming-000](https://user-images.githubusercontent.com/18356694/113034344-9a30be00-9157-11eb-9443-975f3c294f56.gif) ![window-renaming-001](https://user-images.githubusercontent.com/18356694/113034452-b5033280-9157-11eb-9e35-e5ac80fef0bc.gif) It does so through two new actions: * `renameWindow` takes a `name` parameter, and attempts to set the window's name to the provided name. This is useful if you always want to hit <kbd>F3</kbd> and rename a window to "foo" (READ: probably not that useful) * `openWindowRenamer` is more interesting: it opens a `TeachingTip` with a `TextBox`. When the user hits Ok, it'll request a rename for the provided value. This lets the user pick a new name for the window at runtime. In both cases, if there's already a window with that name, then the monarch will reject the rename, and pop a `Toast` in the window informing the user that the rename failed. Nifty! ## References * Builds on the toasts from #9523 * #5000 - process model megathread ## PR Checklist * [x] Closes https://github.com/microsoft/terminal/projects/5#card-50771747 * [x] I work here * [x] Tests addded (and pass with the help of #9660) * [ ] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments I'm sending this PR while finishing up the tests. I figured I'll have time to sneak them in before I get the necessary reviews. > PAIN: We can't immediately focus the textbox in the TeachingTip. It's > not technically focusable until it is opened. However, it doesn't > provide an even tto tell us when it is opened. That's tracked in > microsoft/microsoft-ui-xaml#1607. So for now, the user _needs_ to > click on the text box manually. > We're also not using a ContentDialog for this, because in Xaml > Islands a text box in a ContentDialog won't recieve _any_ keypresses. > Fun! ## Validation Steps Performed I've been playing with ```json { "keys": "f1", "command": "identifyWindow" }, { "keys": "f2", "command": "identifyWindows" }, { "keys": "f3", "command": "openWindowRenamer" }, { "keys": "f4", "command": { "action": "renameWindow", "name": "foo" } }, { "keys": "f5", "command": { "action": "renameWindow", "name": "bar" } }, ``` and they seem to work as expected

## Summary of the Pull Request Make sure that the window renamer and other toasts follow the requested app theme. We accomplish this by doing something similar to what we do with ContentDialogs. Since TeachingTips aren't in the same XAML root, we have to traverse the entire tree upwards setting RequestedTheme. If we don't, then we'll update the background color of the TeachingTip, but not the text inside it. ## References * Added in #9662 and #9523 ## PR Checklist * [x] Closes #9717 * [x] I work here * [n/a] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed Tested with system theme light & dark, and `theme` set to `light, dark, and unset, and verified that they worked as expected.

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

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

Handy links:

zadjii-msft added 23 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

macros are life

ec97c43

THIS NEEDS TO GO TO THE PARENT

fa26f7f

Bind the labels to the actual TerminalPage object

001f545

bind the name, id down to the actual page

9270e0f

add an action for just identifying one single window

1f0dceb

THIS SHOULD GO TO THE PARENT BRANCH

e238daa

(cherry picked from commit aa51c07)

THIS NEEDS TO GO TO THE PARENT

27d12a2

(cherry picked from commit fa26f7f)

yea you better believe the Toast just worked

9d6f47f

feedback from review

c202257

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.

Merge branch 'dev/migrie/f/name-windows-3' into dev/migrie/f/identify…

0799a19

…Windows # Conflicts: # src/cascadia/Remoting/WindowManager.cpp # src/cascadia/TerminalApp/Resources/en-US/Resources.resw

That's better

1105328

Cleanup for review

97b4935

Merge branch 'dev/migrie/f/name-windows-3' into dev/migrie/f/identify…

765c969

…Windows

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

b66503c

…Windows

zadjii-msft added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Mar 17, 2021

zadjii-msft added 2 commits March 17, 2021 15:48

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

b7703d2

…Windows

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

d035e50

…Windows

carlos-zamora requested changes Mar 18, 2021

View reviewed changes

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

DHowett requested changes Mar 19, 2021

View reviewed changes

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

zadjii-msft self-assigned this Mar 23, 2021

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

zadjii-msft added 6 commits March 24, 2021 13:45

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

d64aa61

…Windows # Conflicts: # src/cascadia/TerminalApp/AppLogic.cpp # src/cascadia/TerminalApp/TerminalPage.cpp # src/cascadia/TerminalApp/TerminalPage.h

I'm overcomplicating this

8b6e0bc

Revert "I'm overcomplicating this"

7c775d5

This reverts commit 8b6e0bc.

Mainly: delayload the TeachingTip

b1b94f6

This is the thing dustin suggested, and I like it

31cb1b7

This will make @DHowett happy

5904c58

zadjii-msft assigned DHowett and carlos-zamora and unassigned zadjii-msft Mar 26, 2021

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

carlos-zamora approved these changes Mar 29, 2021

View reviewed changes

zadjii-msft unassigned carlos-zamora Mar 29, 2021

zadjii-msft added 2 commits March 30, 2021 06:40

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

2ff4c60

…Windows

runformat

ff2ce4b

DHowett approved these changes Mar 30, 2021

View reviewed changes

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

ghost merged commit 03ea0f4 into main Mar 30, 2021

ghost deleted the dev/migrie/f/identifyWindows branch March 30, 2021 16:08

zadjii-msft mentioned this pull request Mar 30, 2021

Add support for renaming windows #9662

Merged

4 tasks

This was referenced Apr 6, 2021

Add identifyWindow(s?) to the schema #9726

Merged

Match the RequestedTheme of our TeachingTips to the set theme #9732

Merged

zadjii-msft mentioned this pull request Apr 9, 2021

Scenario: Windows Terminal 2.0 Process Model Improvements #5000

Closed

80 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an action for identifying windows #9523

Add an action for identifying windows #9523

zadjii-msft commented Mar 17, 2021

carlos-zamora Mar 18, 2021

zadjii-msft Mar 18, 2021

DHowett left a comment

DHowett Mar 19, 2021

zadjii-msft Mar 24, 2021

carlos-zamora left a comment

carlos-zamora Mar 29, 2021

zadjii-msft Mar 29, 2021 •

edited

Loading

DHowett Mar 29, 2021

carlos-zamora Mar 29, 2021

zadjii-msft Mar 29, 2021

carlos-zamora Mar 29, 2021

carlos-zamora Mar 29, 2021

DHowett Mar 29, 2021

zadjii-msft Mar 29, 2021

ghost commented Mar 30, 2021

ghost commented Apr 14, 2021

		Title="{x:Bind WindowId, Mode=OneWay, Converter={StaticResource WindowIdConverter}}"
		Subtitle="{x:Bind WindowName, Mode=OneWay, Converter={StaticResource WindowNameConverter}}">

		Title="{x:Bind WindowIdForDisplay}"
		Subtitle="{x:Bind WindowNameForDisplay}">

	<comment>text used to identify when a window hasn't been assigned a name by the user</comment>
	<comment>Text used to identify when a window hasn't been assigned a name by the user</comment>

Add an action for identifying windows #9523

Add an action for identifying windows #9523

Conversation

zadjii-msft commented Mar 17, 2021

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

<unnamed-window>

Window: 1

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft Mar 29, 2021 • edited Loading

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

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

zadjii-msft Mar 29, 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.