Add IDs to Commands #16904
Merged
Add IDs to Commands #16904
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DHowett
reviewed
Mar 20, 2024
PankajBhojwani
commented
Mar 28, 2024
| // - Example: The "SendInput 'abc'" action will have the generated ID "User.sendInput.<hash of 'abc'>" | ||
| // Return Value: | ||
| // - The ID, based on the ShortcutAction and the Args | ||
| winrt::hstring ActionAndArgs::GenerateID() const |
There was a problem hiding this comment.
These IDs are very similar to our InternalActionID except instead of also hashing the ShortcutAction, we only hash the Args and we leave the ShortcutAction as a string. Since these will end up in the user file, I prefer this as it will be more readable
Comment on lines
+507
to
+509
| // we need to generate an ID for a command in the user settings if it doesn't already have one | ||
| auto actionMap{ winrt::get_self<ActionMap>(userSettings.globals->ActionMap()) }; | ||
| fixedUp = actionMap->GenerateIDsForActions() || fixedUp; |
There was a problem hiding this comment.
Since we aren't actually using these IDs yet, this could definitely be moved into the follow up PR that is actually going to refactor our maps to use the new IDs. However then that leaves this PR as just "adding an ID field to Command and barely populating them anywhere". I'm okay with either if anyone feels strongly about it
|
Notes from chat: test everything! test the round-trip serialization of a command with and without an ID, etc! |
microsoft-github-policy-service
bot
added
Needs-Author-Feedback
|
omg why is this so little code |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
DHowett
requested changes
Apr 5, 2024
src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
| // - Example: The "SendInput 'abc'" action will have the generated ID "User.sendInput.<hash of 'abc'>" | ||
| // Return Value: | ||
| // - The ID, based on the ShortcutAction and the Args | ||
| winrt::hstring ActionAndArgs::GenerateID() const |
Comment on lines
+507
to
+509
| // we need to generate an ID for a command in the user settings if it doesn't already have one | ||
| auto actionMap{ winrt::get_self<ActionMap>(userSettings.globals->ActionMap()) }; | ||
| fixedUp = actionMap->GenerateIDsForActions() || fixedUp; |
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
microsoft-github-policy-service
bot
removed
the
Needs-Author-Feedback
DHowett
requested changes
Apr 12, 2024
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
microsoft-github-policy-service
bot
removed
the
Needs-Author-Feedback
lhecker
approved these changes
Apr 17, 2024
DHowett
approved these changes
Apr 17, 2024
|
FWIW, your and perhaps a better name like |
DHowett
approved these changes
Apr 17, 2024
5 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 4, 2024
As outlined in #16816, refactor `ActionMap` to use the new action IDs added in #16904 ## Validation steps performed - [x] Legacy style commands are parsed correctly (and rewritten to the new format) - [x] Actions are still layered correctly and their IDs can be used to 'overwrite' actions in earlier layers - [x] Keybindings that refer to an ID defined in another layer work correctly - [x] User-defined actions without an ID have one generated for them (and their settings file is edited with it) - [x] Schema updated Refs #16816 Closes #17133
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As laid out in #16816, adds an
IDfield toCommand.This first PR only adds IDs for built-in commands in defaults, and generates IDs for user-created commands that don't define an ID. Also note that for now we will not be allowing IDs for iterable/nested commands.
The follow-up PR is where we will actually use the IDs by referring to commands with them.
Refs #16816
Validation Steps Performed
User-created commands in the settings file get rewritten with generated IDs