-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
[1.21 Canary] User actions have IDs added to them in the JSON #17109
Comments
This is by design and mentioned in Pankaj's action IDs spec. This is the way that we will give durable IDs to user-originated actions so that we can pull apart key/button/menu bindings and actual commands. If you're expecting something different, we should have a team sync about it! |
Ah, so, this might be under spec'd? I'd assume that we generate IDs for those commands, but not reserialize those "generated" IDs. So that people don't need to know anything about the existence of IDs, unless they want to add the action to {{menu}} |
|
I think it's important for the IDs to become stable at the first opportunity, otherwise somebody could add an action to the menu with its internal ID, and then change something about it (which generates a new ID) and break the link with the one in the menu. |
How? If we don't give the action an ID in the json, then how would they be using that action's internally generated ID somewhere else in the settings? (unless you're assuming that there's some UI-based way of doing that sometime in the next 10 years) |
The reason we're tackling action things now is to enable UI-based stuff in 1.21. Is there a problem with writing IDs to the user's settings? |
It's extremely noisy, if users aren't using that feature (which none will be in 1.21) (unless actions in the new tab menu et. al will actually be landing in 1.21, which seems like a lot of a lift in the next 7 days) |
I've got a couple thoughts...
Footnotes
|
We don't allow for actions anywhere other than in It feels like the point in the Command Palette spec where we were thinking "Okay, all actions need to be given a name if they want to show up in the command palette", and we decided immediately after: "what if we could just generate names for everything"? Users don't need to known about the action IDs, certainly not in 1.21. And if we do add a "new tab menu" SUI page in 1.22, and a user wants to add one of their custom actions then - we can always decide to commit that ID to the file then. |
That's not required for my scenario to exist! What if I want to make alt-shift-d/e/l If you want to change the spec, propose it and talk to @PankajBhojwani about it. We're going to have action IDs eventually, so why pull them now? |
The thing is, the immediate next PR is going to degranulate the actions from the key bindings in the settings file, so there's a map of keys to actions and a map of actions to their actual commands. That practically requires action IDs as the linkage token between them. We should meet to go over the final goal of the spec if we're misaligned on that! |
We will no longer serialize IDs that we generated for the user. This change is being made so that we can release user action IDs at the same time as the features that require them! Validation: Generated IDs do not get written to the json, user-made IDs still do Closes #17109
Unclear which version exactly. Was working in between
main
and1.21.1094.0
, I think. mostly trying to debug #17089. And look at that, all my actions haveid
's added to them (I did not add these):The text was updated successfully, but these errors were encountered: