Introduce serialization for actions by carlos-zamora · Pull Request #9926 · microsoft/terminal

carlos-zamora · 2021-04-22T23:37:06Z

Summary of the Pull Request

This PR builds on the ActionMap PR (#6900) by leveraging ActionMap to serialize actions. From the top down, the process is now as follows:

CascadiaSettings: remove the hack of copying whatever we had for actions before.
GlobalAppSettings: serialize the ActionMap to "actions": []
ActionMap: iterate over the internal _ActionMap (list of actions) and serialize each Command
Command: THIS IS WHERE THE MAGIC HAPPENS! For each key mapping, serialize an action. Only the first one needs to include the name and icon.
ActionAndArgs: Find the relevant IActionArgs parser and serialize the ActionAndArgs.
ActionArgs: ANNOYING CHANGE Serialize any args that are set. We need each setting to be saved as a std::optional. As with inheritance, this allows us to distinguish an explicit setting to the default value (sometimes null) vs an implicit "give me the default value". This allows us to serialize only the relevant details of each action, rather than writing all of the args.

References

Implement inheritance/layering behavior for settings lists/maps #8100: Inheritance/Layering for lists
- This tracks layering and better serialization for color schemes and actions. This PR resolves half of that issue. The next step is to apply the concepts used in this PR (and Introduce ActionMap to Terminal Settings Model #9621) to solve the similar problem for color schemes.
Epic: Introduce Actions page to the Settings UI #6900: Actions page

Validation Steps Performed

Tests added!

zadjii-msft

I only really have the one main question that I want answered. Otherwise, the rest seems pretty straightforward.

We should make sure to add a test for #9926 (comment) as well, because it's a good edge case

src/cascadia/TerminalSettingsModel/JsonUtils.h

src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp

src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp

zadjii-msft

🙌 on the ACTION_TO_SERIALIZERS_PAIR thing, and thanks for adding the test!

carlos-zamora · 2021-05-13T19:06:46Z

Move focus doesn't display direction

This bug was caused by the list of ALL_SHORTCUT_ACTIONS_WITH_ARGS missing MoveFocus. I went ahead and sorted it, then double checked that I'm not missing anything.

DHowett · 2021-05-17T02:29:18Z

src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp

+#undef ON_ALL_ACTIONS
+    };
+
+    static const std::map<ShortcutAction, std::string_view, std::less<>> ActionToStringMap{


We have got to stop it with the globally initialized maps. Sorry. This looks like a case for til::static_map or til::presorted_static_map. I'd rather do that than have yet another static initializer 😄

Should I do what we did with GeneratedActionNames in ActionAndArgs::GenerateName? Define these in the function they're used as static lambda functions. Or does that not make this any better?

@DHowett Can you explain the problem with static initializers for the uninitiated? 🙂

Sure!

Initializing a static non-constexpr value at file scope generates a block of code that must be executed when the library is loaded, every time it is loaded. For something like the settings model, that cost is paid once on startup, but it is still a cost that must be paid before any of the actual settings model code is run.

constexpr is a different story: those are evaluated at compile time and stored permanently in the binary.

For a really, really good example of the cost of a static initializer (or something very similar) and the payback you get from replacing it with a fully-constexpr data store, look at commit 16e1e29.

This seems fairly solid - it even offers perfect hash functions: https://github.com/serge-sans-paille/frozen

Leonard and I couldn't get this to work, unfortunately. We kept running into til::presorted_static_map saying that it couldn't find the static_map in the ctor or something like that. Sorry!

src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp

src/cascadia/TerminalSettingsModel/ActionArgs.cpp

src/cascadia/TerminalSettingsModel/ActionArgs.h

src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp

src/cascadia/TerminalSettingsModel/Command.cpp

DHowett · 2021-05-17T14:38:15Z

The problem is the static map, not the functions.

src/cascadia/TerminalSettingsModel/Command.cpp

## 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 ![Actions Page Demo](https://user-images.githubusercontent.com/11050425/116034988-131d1b80-a619-11eb-8df2-c7e57c6fad86.gif)

ghost · 2021-05-18T21:37:42Z

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.

ghost · 2021-05-25T19:11:00Z

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

Handy links:

carlos-zamora requested a review from zadjii-msft April 22, 2021 23:37

This comment has been minimized.

Sign in to view

carlos-zamora marked this pull request as ready for review April 22, 2021 23:58

This comment has been minimized.

Sign in to view

carlos-zamora force-pushed the dev/cazamor/tsm/am/serialization branch 2 times, most recently from 5d0a656 to 3b99fde Compare April 23, 2021 21:32

carlos-zamora mentioned this pull request Apr 26, 2021

Make Actions page editable #9949

Merged

carlos-zamora force-pushed the dev/cazamor/tsm/am/serialization branch 5 times, most recently from a6b5bde to 0068ae0 Compare May 5, 2021 04:21

Base automatically changed from dev/cazamor/tsm/action-map to main May 5, 2021 04:50

carlos-zamora and others added 4 commits May 4, 2021 21:52

Implement action serialization

c434ee6

handle and test nested and unbound commands

586b977

update spell check

0741c94

fix global summon

9cb377e

carlos-zamora force-pushed the dev/cazamor/tsm/am/serialization branch from 0068ae0 to 9cb377e Compare May 5, 2021 04:52

This comment has been minimized.

Sign in to view

Update Command::Copy()

b506817

This comment has been minimized.

Sign in to view

zadjii-msft requested changes May 6, 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 May 6, 2021

zadjii-msft added Area-Settings Issues related to settings and customizability, for 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 May 6, 2021

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

address Mike feedback

07cd343

zadjii-msft approved these changes May 10, 2021

View reviewed changes

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

ghost requested review from DHowett, PankajBhojwani, leonMSFT, lhecker and miniksa May 10, 2021 11:28

carlos-zamora mentioned this pull request May 12, 2021

Epic: Introduce Actions page to the Settings UI #6900

Open

12 tasks

bugfix: add 'movefocus' (de)serializer; sort list

815890f

Merge branch 'main' into dev/cazamor/tsm/am/serialization

3682f36

DHowett requested changes May 17, 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 May 17, 2021

address some Dustin feedback

4ac4e10

DHowett reviewed May 17, 2021

View reviewed changes

src/cascadia/TerminalSettingsModel/Command.cpp Outdated Show resolved Hide resolved

carlos-zamora added 3 commits May 17, 2021 18:38

revert Command.cpp iterator change

280073c

Merge branch 'main' into dev/cazamor/tsm/am/serialization

afe5dad

minor refactor of ToJson for cleaner code flow

68c9e8d

carlos-zamora assigned DHowett May 18, 2021

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 18, 2021

DHowett approved these changes May 20, 2021

View reviewed changes

ghost merged commit ff8fdbd into main May 20, 2021

ghost deleted the dev/cazamor/tsm/am/serialization branch May 20, 2021 18:44

This pull request was closed.

Conversation

carlos-zamora commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

Validation Steps Performed

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented May 13, 2021

Uh oh!

DHowett May 17, 2021

Choose a reason for hiding this comment

Uh oh!

carlos-zamora May 17, 2021

Choose a reason for hiding this comment

Uh oh!

lhecker May 17, 2021

Choose a reason for hiding this comment

Uh oh!

DHowett May 17, 2021

Choose a reason for hiding this comment

Uh oh!

lhecker May 17, 2021

Choose a reason for hiding this comment

Uh oh!

carlos-zamora May 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett commented May 17, 2021 via email

Uh oh!

Uh oh!

ghost commented May 18, 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.

Uh oh!

ghost commented May 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carlos-zamora commented Apr 22, 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.