Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically generate ShortcutAction code from ~JSON Schema~ X-Macros #3475

Closed
15 tasks
zadjii-msft opened this issue Nov 7, 2019 · 2 comments · Fixed by #11859
Closed
15 tasks

Automatically generate ShortcutAction code from ~JSON Schema~ X-Macros #3475

zadjii-msft opened this issue Nov 7, 2019 · 2 comments · Fixed by #11859
Assignees
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@zadjii-msft
Copy link
Member

While working on #3391, I felt like there was a ton of boilerplate that was needed for each and every keybindings. The thought crossed my mind that we could do better, but I wasn't sure how. In my mind, I imagined a single file that described each of the ShortcutActions and their possible args, and a small script that just automatically generated all the necessary code to parse them, their args, and hook them up to the TerminalApp.

Now that I've thought on it more, here are some of the steps that'll be needed:

  • Take code from my command palette branch for ShortcutActionDispatch. This is a class that just handles the dispatching of actions. In that branch, I needed it because both keybindings and commands needed to be able to dispatch ShortcutActions, but only AppKeyBindings needed to do a KeyChord lookup. This code being encapsulated will be necessary for a future step.
  • Add more _templated_ JsonUtils #2550 Add more templated json utils. We'll need this to be able to automagically generate the code to parse a given type from a Json::Value
  • Move the code around so that anything having to do with the entirety of the list of ShortcutActions is in single files by themselves:
    • AppKeybindingSerialization should only deal with parsing keychords. Lets put the ShortcutActions and their keys in another file
    • Add a IShortcutActionHandler interface, with _Handle{{SomeShortcutAction}}(auto&& sender, ActionArgs args) methods for each ShortcutAction. It should declare all the event handlers, but not the definitions.
    • Make TerminalPage extend IShortcutActionHandler. This will be necessary to make sure TerminalPage implements each of them.
    • That one method that hooks up the TerminalPage to the ShortcutActionDispatch. Pull that into it's own file as well.
  • Write a script that parses profiles.schema.json for each of the "*ShortcutAction" definitions, and turns them into code in all the necessary files
    • ShortcutActionKeys.h
    • ActionAndArgs.idl/.h/.cpp
    • ShortcutActionDispatch.idl/.h/.cpp
    • IShortcutActionHandler.h
    • For actions that are listed in ShortcutActions but don't have a proper Args type, then make sure that they're still dispatched correctly
    • When the number of args changes, write the above files, and display a message that the developer needs to update AppActionHandlers.cpp to add implementations for the new types.
    • Validate that all "*ShortcutAction"s listed in the schema actually show up in the onOf schema for Keybinding

Each top-level box could be it's own task here.

In the end, the developer should be able to modify the schema for add their new argument or ShortcutAction, and run a script, and the script should automatically write all the modifications necessary.

I'll probably need to do this in powershell. cmd would be impossible, and as much as I love python, I don't think that's really a part of our toolchain at all currently.

@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Nov 7, 2019
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Nov 7, 2019
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 7, 2019
@zadjii-msft
Copy link
Member Author

(discussed with @DHowett-MSFT, so yanking triage)

@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 7, 2019
zadjii-msft added a commit that referenced this issue Nov 21, 2019
zadjii-msft added a commit that referenced this issue Nov 27, 2019
## Summary of the Pull Request

Moves all the code responsible for dispatching an `ActionAndArgs` to it's own class, `ShortcutActionDispatch`. Now, the `AppKeyBindings` just uses the single instance of a `ShortcutActionDispatch` that the `TerminalPage` owns to dispatch events, without the need to re-attach the event handlers every time we reload the settings.

## References

This is something I originally did as a part of #2046.

I need this now for #607.

It's also a part of work for #3475

## PR Checklist
* [x] This is a bullet point within #3475
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

With this change, we'll be able to have other things dispatch `ShortcutAction`s easily, by constructing an `ActionAndArgs` and just passing it straight to the `ShortcutActionDispatch`.

## Validation Steps Performed

Ran the Terminal, tried out some keybindings, namely <kbd>Ctrl+c</kbd> for copy when there is a selection, or send `^C` when there isn't. That still works. 

Reloading settings also still works. 

-----------------------------------------------
* Move action handling to it's own class separate from AKB. This is the first checkbox in #3475

(cherry picked from commit 696726b)

* clean up doc comments
@carlos-zamora
Copy link
Member

There's a lot in here. @zadjii-msft since you created this issue, think it's worth throwing in the v2 milestone?

@zadjii-msft zadjii-msft changed the title Automatically generate ShortcutAction code from JSON Schema Automatically generate ShortcutAction code from ~JSON Schema~ X-Macros Sep 21, 2021
@zadjii-msft zadjii-msft self-assigned this Nov 8, 2021
@ghost ghost added the In-PR This issue has a related PR label Dec 1, 2021
@ghost ghost closed this as completed in #11859 Dec 6, 2021
ghost pushed a commit that referenced this issue Dec 6, 2021
This adds x-macros for each of the actions, greatly reducing the amount of boilerplate needed for each action args. 

Originally, I wanted to do more with this, but I think the x-macros we've discovered properly treads the line of ease-of-use and native c++ support, with how much it'll do for us

* [x] Closes #3475
* [x] Sure enough, the tests still pass.
* [ ] mmmmmmm  ![image](https://user-images.githubusercontent.com/18356694/144319133-329ee7ef-4aa7-4769-b11b-6e4994075dd0.png)
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Dec 6, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants