Bugfix: serialize iterable commands #10373
Conversation
carlos-zamora
added
the
zPreview-Service-Queued-1.13
ghost
added
Area-Settings
|
While working on this bugfix, I realized that another bug exists: {
"name": "${profile.name}",
"command": { "action": "newTab", "profile": "${profile.name}" },
"iterateOn": "profiles"
},
{
"name": "${profile.name}",
"commands": [ { "command": "newTab" } ]
}Here, the settings model exports these commands to the command palette as one entry in the name map (the iterable command). This is wrong though, because really the user is explicitly asking for a command named "${profile.name}". However, this bug already exists in v1.8, but in a different manifestation. Here's the encountered behaviors:
This bugfix brings us closer to a world where this new bug is fixed. We would need to mess with how we expand commands. But (a) that's too risky for a hotfix and (b) this is a really obscure bug. @DHowett Thoughts? |
I'd say just.. file it and we can move on. 😄 |
| // copy _IterableCommands | ||
| for (const auto& cmd : _IterableCommands) | ||
| { | ||
| actionMap->_IterableCommands.Append(*(get_self<Command>(cmd)->Copy())); |
There was a problem hiding this comment.
qq: why do we need to keep them separate from the nested commands?
There was a problem hiding this comment.
Keeping them separate does half of the work for this bug.
Combining them keeps the same buggy behavior from v1.8/main.
carlos-zamora
added
the
AutoMerge
|
Hello @carlos-zamora! Because this pull request has the 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 (
|
ghost
merged commit 
DHowett
removed
the
zPreview-Service-Queued-1.13
## Summary of the Pull Request Fixes a bug where top-level iterable commands were not serialized. ## PR Checklist * [X] Closes #10365 * [X] Tests added/passed ## Detailed Description of the Pull Request / Additional comments - `Command::ToJson`: - iterable commands deserve the same treatment as nested commands - `ActionMap`: - Similar to how we store nested commands, iterable commands need to be handled separately from standard commands. Then, when generating the name map, we make sure we export the iterable commands at the same time we export the nested commands. (cherry picked from commit 9294ecc)
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 Handy links: |
Summary of the Pull Request
Fixes a bug where top-level iterable commands were not serialized.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Command::ToJson:ActionMap: