Make Actions page editable

[carlos-zamora]

Summary of the Pull Request

This PR lays the foundation for a new Actions page in the Settings UI as designed in #6900. The Actions page now leverages the ActionMap to display all of the key bindings and allow the user to modify the associated key chord or delete the key binding entirely.

References

#9621 - ActionMap
#9926 - ActionMap serialization
#9428 - ActionMap Spec
#6900 - Actions page
#9427 - Actions page design doc

Detailed Description of the Pull Request / Additional comments

Settings Model Changes

• Command::Copy() now copies the ActionAndArgs
• ActionMap::RebindKeys() handles changing the key chord of a key binding. If a conflict occurs, the conflicting key chord is overwritten.
• ActionMap::DeleteKeyBinding() "deletes" a key binding by binding "unbound" to the given key chord.
• ActionMap::KeyBindings() presents another view (similar to NameMap) of the ActionMap. It specifically presents a map of key chords to commands. It is generated similar to how NameMap is generated.

Editor Changes

• Actions.xaml is mainly split into two parts:
  • ListView (as before) holds the list of key bindings. We could explore the idea of an items repeater, but the ListView seems to provide some niceties with regards to navigating the list via the key board (though none are selectable).
  • DataTemplate is used to represent each key binding inside the ListView. This is tricky because it is bound to a KeyBindingViewModel which must provide all context necessary to modify the UI and the settings model. We cannot use names to target UI elements inside this template, so we must make the view model smart and force updates to the UI via changes in the view model.
• KeyBindingViewModel is a view model object that controls the UI and the settings model.

There are a number of TODOs in Actions.cpp will be long-term follow-ups and would be nice to have. This includes...

• a binary search by name on Actions::KeyBindingList
• presenting an error when the provided key chord is invalid.

Demo

[github-actions]

Misspellings found, please review:

• Btn
• Desctiption
• seemlessly
• whos

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"actionmap reserialize Unregistering "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/0e8d5f24c4cf80e104cf6d16ae004c54015c13ce.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"Btn Desctiption seemlessly unregistering whos "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

• ... misspelled, then please correct them instead of using the command.
• ... names, please add them to .github/actions/spelling/dictionary/names.txt.
• ... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
• ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
• ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

[zadjii-msft]

Yea okay, I'm fine with like 99% of it, I just have a couple questions, and I think it needs more comments

[zadjii-msft]

capturing everything by value here seems sus, but it's probably fine? The only things getting captured are already members of the SUI, and it's not like this will ever get triggered after the SUI is destroyed, so yea this is probably fine

[zadjii-msft]

Suggested change

	for (uint32_t i = 0; i < _KeyBindingList.Size(); ++i)
	{
	const auto& kbdVM{ _KeyBindingList.GetAt(i) };
	for (const auto& kbdVM : _KeyBindingList)
	{

right?

[zadjii-msft]

oh never mind, we do need the index below

[carlos-zamora]

Yeah. We could use ContainerFromItem, but I figured ContainerFromIndex would be faster here since we're already iterating over all of the key binding view models anyways.

[carlos-zamora]

Bug Bash Notes (5/12)

• Performance
  • Clicking Actions in the left bar of Settings takes about 3 seconds to load and doesn't cache/keep it if I leave page and come back ( 3 more seconds )
    • This was moved to Epic: Introduce Actions page to the Settings UI #6900 as a follow-up task.
• Layout and Style
  • Trash button is cut off
    • No Repro: attempted to repro on my surface book and main dev machine with different scaling.
  • make buttons the same height as the text box
  • make accept button have accent color
  • "ctrl+shift+w" has no right side padding to match the left side padding. 'W' is cut off
    • Won't Fix: this is just the style XAML has when right-aligning the text. Approved by Kayla.
• Bugs
  • Move focus doesn't display direction
    • root cause: Introduce serialization for actions #9926. Fixed there
  • Narrator reads out "4 of 64" on the list item, instead of what we're focused on
  • Narrator says "button" for edit buttons
• Features
  • need a key binding editor (instead of parsing)
    • This was moved to Epic: Introduce Actions page to the Settings UI #6900 as a follow-up task.
  • support enter/esc in key chord text box
• Serialization
  • "the unbound bug" (coined by me)
    • Edit switchToTab to win+shift+q. Save. Edit switchToTab to ctrl+shift+t.
    • "unbound": "win+shift+q" appears but doesn't need to exist.
    • This was moved to Epic: Introduce Actions page to the Settings UI #6900 as a follow-up task.

[carlos-zamora]

Still need to come back to work on that performance issue. It seems doable, but I'll need a bit more help so I'll leave that for Monday.

Added a nice background to the list view item when it's in edit mode. An annoying problem is that it'll only appear if you're using the Windows OS theme. But that's due to #9716.

[leonMSFT]

Does a new flyout get created every time there's a conflicting command? Could there be a way to reuse it?

[carlos-zamora]

I wish! As mentioned in the comments below:

1. The data template prevents us from getting a control by name. So we have no way of targeting the button and attaching a pre-saved flyout.
2. The flyout needs to appear sometimes. So we can't just build it in XAML, because then we don't have control over when it appears or not.

[leonMSFT]

Ah I see, dang that's pretty annoying 😢

[leonMSFT]

I'm unsure if this will work, but you might be able to give the button a flyout without having it automagically open the flyout on every button press by putting the flyout in FlyoutBase.AttachedFlyout. It's an attached property so this particular property can be set on just about any FrameworkElement, but IIRC it doesn't behave the same as if you set the Flyout property on Button.

https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.controls.primitives.flyoutbase?view=winrt-19041#xaml-attached-properties - should tell you more about how to use this particular attached property

This is more an experimental idea though, not something I'd block over 😄

e.g.

<Button>
    <FlyoutBase.AttachedFlyout>
        <Flyout>
        </Flyout>
    </FlyoutBase.AttachedFlyout>
</Button>

[zadjii-msft]

Yea okay you touched everything I was worried about.

[ghost]

Hello @carlos-zamora!

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]

Questions mostly around function rather than form. Form looks excellent, and I like how it feels.

[DHowett]

i'm surprised that this takes the oldkeys and not the action -- shouldn't we enforce that the caller is providing an action?

[carlos-zamora]

So, oldKeys can be used as a primary key the way it's written now.

If we wanted to switch over to using ActionAndArgs, we could make the API look more like this:

void RegisterKeyBinding(ActionAndArgs, KeyChord);
void EraseKeyBinding(ActionAndArgs, KeyChord);

// For later...
void SetActionForKeyBinding(KeyChord, ActionAndArgs);
void SetName(ActionAndArgs, String);

I'm intentionally trying to make it so that we don't work with Command outside of ActionMap.

[ghost]

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

Handy links:

• Release Notes
• Store Download