Add a simple page for keybindings #9253

zadjii-msft · 2021-02-23T14:02:05Z

This was the only thing blocking me from signing off on #9224 in 1.7.

! CHANGE WARNING !
If we bind to T.S.M.Commands in XAML, then the compiler gets very
angry at us. It generates two different versions of
GetReferenceTypeMember_Icon in XamlTypeInfo.g.cpp. Presumably
because there's an Icon on a NavViewItem and an Icon on a Command. We
don't really know why. Fortunately, the fix is "rename Command::Icon" to
"Command::IconPath". It's dumb, but it works. Thanks for the help with
that one Carlos ☺️

Unblocks #9224

Probably just going to remove this entirely tbh

zadjii-msft · 2021-02-23T14:04:28Z

@carlos-zamora
One weird thing in this PR: Whenever you save the settings file, the entire list disappears. Presumably because we create a new Actions page which makes a new vector of filtered actions, but then never get the OnNavigatedTo to populate the list? I'm investigating, but if you have any ideas like "oh yea that's totally happened to me before. You've got to...", that would be much appreciated

EDIT:

Oh it looks like in the OnNavigatedTo, none of the commands have keychordtext yet.
The OnNavigatedTo event happens in the second half of TerminalPage::_RefreshUIForSettingsReload
which happens in the first half of TerminalPage::SetSettings, before TPage::_UpdateCommandsForPalette
TerminalPage::_UpdateCommandsForPalette is the guy that goes through and fills in the KeyChordText for commands. On first load, it's already been done by the time the Actions page loads. On hot reloads - not so much.
- it's a heavier piece of code that, but it's already on the UI thread for a hot-reload anyways so moving it shouldn't matter

EDIT 2: yep that worked

cinnamon-msft · 2021-02-23T17:24:47Z

My only feedback is to change the page name to Actions and then scoot the right column closer to the left a bit. Otherwise, this is fantastic. :)

carlos-zamora

I have a few design issues that I'm conflicted with because of a short release window:

nested commands should pop up in a hierarchical view
if a command doesn't have a name, it won't appear in the list, right?

We've basically created a command palette copy in the settings UI. But in that case, what value does it add? Why doesn't the user just open the command palette in another tab and get nested commands for free?

I'm not approving because I'm genuinely conflicted by the value this adds. But I'm not going to block it because it seems to be what we want. I'll circle back to how I feel about this in the afternoon.

carlos-zamora · 2021-02-23T17:44:07Z

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

+  <data name="Globals_KeybindingsLink.Content" xml:space="preserve">
+    <value>Open JSON settings</value>
+  </data>


nit: maybe to try and reduce loc burden, could we just reuse the Nav_OpenJson/Content?

carlos-zamora · 2021-02-23T17:48:45Z

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

+  <data name="Nav_Actions.Content" xml:space="preserve">
+    <value>Keybindings</value>
+  </data>


According to this design doc, I think it's supposed to be named "Keyboard". But I feel like we changed it at some point. @cinnamon-msft can you confirm what the name should be?

I'm torn on this now. I like Keyboard because it clearly shows the user where to find key bindings. However, our JSON uses Actions, so if someone wanted to edit their keys, would they know they'd have to edit the Actions array?

Okay I'm going with Actions for now. The later in the day we get, the harder it will be to change again

carlos-zamora · 2021-02-23T17:51:49Z

src/cascadia/TerminalSettingsEditor/Actions.xaml

+            <TextBlock x:Uid="Globals_KeybindingsDisclaimer"
+                       Style="{StaticResource DisclaimerStyle}"/>


(not @zadjii-msft's fault, but still relevant) This text block is not read by a screen reader (same with the disclaimer in base layer). I'll file a separate issue to track this one.

This is going to be important for us to fix before any settings UI goes stable. Why are they not read?

Moved to:
"Disclaimer" text boxes are not read in narrator #9255

carlos-zamora · 2021-02-23T17:55:55Z

src/cascadia/TerminalSettingsEditor/Actions.xaml

+
+                <ListView HorizontalAlignment="Stretch"
+                          VerticalAlignment="Stretch"
+                          SelectionMode="Single"


Suggested change

SelectionMode="Single"

SelectionMode="None"

IsItemClickEnabled="False"

Should we make it so that you can't interact with this? There's no reason for a user to select entries.

Sure, why not?

carlos-zamora · 2021-02-23T17:56:56Z

src/cascadia/TerminalSettingsEditor/Actions.xaml

+                          SelectionMode="Single"
+                          CanReorderItems="False"
+                          AllowDrop="False"
+                          ItemsSource="{x:Bind FilteredActions}"


Suggested change

ItemsSource="{x:Bind FilteredActions}"

ItemsSource="{x:Bind FilteredActions, Mode="OneWay"}"

We should bind this OneWay. This may have caused some mayhem for you in trying to figure out why it didn't repopulate?

src/cascadia/TerminalSettingsEditor/MainPage.cpp

carlos-zamora · 2021-02-23T18:36:58Z

src/cascadia/TerminalSettingsEditor/Actions.idl

+{
+    runtimeclass ActionsPageNavigationState
+    {
+        Microsoft.Terminal.Settings.Model.CascadiaSettings Settings;


It seems like we only need GlobalAppSettings to get access to the list of commands. We should pass that in instead of the whole settings object.

Sure, this was me being quick and dirty

I suspect we will eventually need the whole thing. Just like Profiles needs Color Schemes.

what we really need is a viewmodel.

src/cascadia/TerminalSettingsEditor/Actions.h

carlos-zamora · 2021-02-23T18:43:53Z

src/cascadia/TerminalSettingsEditor/Actions.xaml

+            looks quite redundant -->
+            <ContentPresenter x:Uid="Globals_Keybindings" Margin="0">
+
+                <ListView HorizontalAlignment="Stretch"


I feel like we should have a header for each column here. But that's so much extra work for a small release window. Plus additional loc burden.

src/cascadia/TerminalSettingsEditor/Actions.cpp

carlos-zamora

Ok. Sat down and thought about this more. I think what we have here is great for WT Preview. I'd like for it to get some kind of polish over the next month before it hits stable. But we'll just need some community feedback.

We'll also just have to have a conversation on how we want to transition from this read-only view to the actual one. I'm starting to think of some ideas for what that'll look like, but we really need @cinnamon-msft to fully design that out.

zadjii-msft · 2021-02-23T19:14:00Z

nested commands should pop up in a hierarchical view

The point of this page is to quickly display a list of all the keybindings. Not of all the actions/commands. Not to expose the structure of the command palette. I'm not aware of anyone who's got a key bound to a nested command - there certainly aren't any bound by default. I'm willing to accept the bug for now that "nested actions with a key bound don't show up in the list of keybindings". (probably easy to fix now tbh...)

if a command doesn't have a name, it won't appear in the list, right?

Correct. There's few and far between commands that don't have a generated name - usually that's due to parameters not being valid. I'll be shocked if someone reports "but muh command with a null name isn't displayed there". This is an artifact of me using the Commands map to get these keybindings, not the KeyMap. The Commands can't have null names, because the Name is the key. The KeyMap can have null names, but it's also not iterable?

We've basically created a command palette copy in the settings UI. But in that case, what value does it add? Why doesn't the user just open the command palette in another tab and get nested commands for free?

Because already, nobody knows about the command palette. That's pretty consistently something I mention and people go "oh, that's neat, didn't know about that". The point here is 99% to just say "these are the current keybindings, you can edit them in the file", rather than have people ask how they change their keybindings with the settings UI. The target audience here is the user who knows 10% about the terminal, who might not even know the Terminal has remappable keys. I'm guessing people that already know to go to the file to change their keybindings are in the 50% range. People who are actively using actions to add commands and nested commands are probably the top 90%. The 50% and 90% users don't really need this page at all.

I'm 100% okay throwing this out when the real UI lands. This is supposed to be throw-away code.

DHowett · 2021-02-23T19:28:14Z

Before adding icons consider that 0 of our built in actions have icons. I’d rather pass on additional complexity

DHowett · 2021-02-23T19:35:28Z

src/cascadia/TerminalSettingsModel/Command.idl

@@ -16,7 +16,7 @@ namespace Microsoft.Terminal.Settings.Model
        ActionAndArgs Action;
        String KeyChordText;

-        String Icon;
+        String IconPath;


but but but iuts' not a path
so sad about this :|

I'd really really love for this to not be totally insane.

IconSource? idk. When I get more time, I'm gonna set up a simple repro for the WinUI team to tell me exactly wtf is going on here.

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalSettingsEditor/Actions.cpp

DHowett · 2021-02-23T19:38:41Z

src/cascadia/TerminalSettingsEditor/Actions.idl

+{
+    runtimeclass ActionsPageNavigationState
+    {
+        Microsoft.Terminal.Settings.Model.CascadiaSettings Settings;


I suspect we will eventually need the whole thing. Just like Profiles needs Color Schemes.

what we really need is a viewmodel.

src/cascadia/TerminalSettingsEditor/Actions.xaml

DHowett · 2021-02-23T19:41:21Z

src/cascadia/TerminalSettingsEditor/Actions.xaml

+            <TextBlock x:Uid="Globals_KeybindingsDisclaimer"
+                       Style="{StaticResource DisclaimerStyle}"/>


This is going to be important for us to fix before any settings UI goes stable. Why are they not read?

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

zadjii-msft · 2021-02-23T20:53:20Z

works in HC:

ghost · 2021-02-23T23:37:13Z

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.

ghost · 2021-03-01T18:02:21Z

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

Handy links:

zadjii-msft added 7 commits February 22, 2021 17:02

what the fuck?

bc0a953

rename Command::Icon -> Command->IconPath, because XAML is a MADHOUSE

1985362

Update the icon

e4c4349

Filter commands we don't need

9086b15

Update the BG to match the command palette

9aaa1bf

Probably just going to remove this entirely tbh

remove some unused styles; add comments

aeb1093

that looks good to me

9316519

zadjii-msft added Product-Terminal The new Windows Terminal. Area-SettingsUI Anything specific to the SUI labels Feb 23, 2021

yep that worked

6e354e9

carlos-zamora reviewed Feb 23, 2021

View reviewed changes

carlos-zamora approved these changes Feb 23, 2021

View reviewed changes

DHowett reviewed Feb 23, 2021

View reviewed changes

zadjii-msft mentioned this pull request Feb 23, 2021

"Disclaimer" text boxes are not read in narrator #9255

Closed

some trivial wording nits

05d48ca

Align the tooltip, and change the text to what @cinnamon-msft wanted

764b07c

zadjii-msft assigned DHowett and cinnamon-msft Feb 23, 2021

zadjii-msft mentioned this pull request Feb 23, 2021

Untie _UpdateCommandsForPalette and _RefreshUIForSettingsReload #9256

Open

This sure don't make any sense, but it works ¯\_(ツ)_/¯

04cd412

DHowett approved these changes Feb 23, 2021

View reviewed changes

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

ghost merged commit f87596f into main Feb 23, 2021

ghost deleted the dev/migrie/actions-list branch February 23, 2021 23:37

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a simple page for keybindings #9253

Add a simple page for keybindings #9253

zadjii-msft commented Feb 23, 2021 •

edited by DHowett

Loading

zadjii-msft commented Feb 23, 2021 •

edited

Loading

cinnamon-msft commented Feb 23, 2021

carlos-zamora left a comment

carlos-zamora Feb 23, 2021

carlos-zamora Feb 23, 2021

cinnamon-msft Feb 23, 2021

zadjii-msft Feb 23, 2021

carlos-zamora Feb 23, 2021

DHowett Feb 23, 2021

zadjii-msft Feb 23, 2021

carlos-zamora Feb 23, 2021

zadjii-msft Feb 23, 2021

carlos-zamora Feb 23, 2021

carlos-zamora Feb 23, 2021

zadjii-msft Feb 23, 2021

DHowett Feb 23, 2021

carlos-zamora Feb 23, 2021

carlos-zamora left a comment

zadjii-msft commented Feb 23, 2021

DHowett commented Feb 23, 2021

DHowett Feb 23, 2021

zadjii-msft Feb 23, 2021

DHowett Feb 23, 2021

DHowett Feb 23, 2021

zadjii-msft commented Feb 23, 2021

ghost commented Feb 23, 2021

ghost commented Mar 1, 2021

		<TextBlock x:Uid="Globals_KeybindingsDisclaimer"
		Style="{StaticResource DisclaimerStyle}"/>

	SelectionMode="Single"
	SelectionMode="None"
	IsItemClickEnabled="False"

	ItemsSource="{x:Bind FilteredActions}"
	ItemsSource="{x:Bind FilteredActions, Mode="OneWay"}"

Add a simple page for keybindings #9253

Add a simple page for keybindings #9253

Conversation

zadjii-msft commented Feb 23, 2021 • edited by DHowett Loading

zadjii-msft commented Feb 23, 2021 • edited Loading

cinnamon-msft commented Feb 23, 2021

carlos-zamora left a 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

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

zadjii-msft commented Feb 23, 2021

DHowett commented Feb 23, 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

zadjii-msft commented Feb 23, 2021

ghost commented Feb 23, 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 Mar 1, 2021

zadjii-msft commented Feb 23, 2021 •

edited by DHowett

Loading

zadjii-msft commented Feb 23, 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.