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

[1.21 Canary] User actions have IDs added to them in the JSON #17109

Closed
zadjii-msft opened this issue Apr 23, 2024 · 11 comments · Fixed by #17145
Closed

[1.21 Canary] User actions have IDs added to them in the JSON #17109

zadjii-msft opened this issue Apr 23, 2024 · 11 comments · Fixed by #17145
Assignees
Labels
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. Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.

Comments

@zadjii-msft
Copy link
Member

Unclear which version exactly. Was working in between main and 1.21.1094.0, I think. mostly trying to debug #17089. And look at that, all my actions have id's added to them (I did not add these):

        {
            "command":
            {
                "action": "sendInput",
                "input": "git status"
            },
            "id": "User.sendInput.B1352E5E"
        },
        {
            "command":
            {
                "action": "selectOutput",
                "direction": "prev"
            },
            "id": "User.selectOutput.D3F0B923",
            "keys": "ctrl+shift+up"
        },
        {
            "command":
            {
                "action": "splitPane"
            },
            "id": "User.splitPane.9AF9A264"
        },
        {
            "command":
            {
                "action": "selectOutput",
                "direction": "next"
            },
            "id": "User.selectOutput.2A0DA8E0",
            "keys": "ctrl+shift+down"
        },
        {
            "command":
            {
                "action": "selectCommand",
                "direction": "next"
            },
            "id": "User.selectCommand.2A0DA8E0",
            "keys": "ctrl+alt+shift+down"
        },
        {
            "command":
            {
                "action": "selectCommand",
                "direction": "next"
            },
            "keys": "ctrl+g"
        },
        {
            "command":
            {
                "action": "selectCommand",
                "direction": "prev"
            },
            "id": "User.selectCommand.D3F0B923",
            "keys": "ctrl+alt+shift+up"
        },
@zadjii-msft zadjii-msft added Area-Settings Issues related to settings and customizability, for console or terminal Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. labels Apr 23, 2024
@zadjii-msft zadjii-msft added this to the Terminal v1.21 milestone Apr 23, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Apr 23, 2024
@zadjii-msft zadjii-msft changed the title [1.21 Canary] User actions [1.21 Canary] User actions have IDs added to them in the JSON Apr 23, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Apr 23, 2024
@DHowett
Copy link
Member

DHowett commented Apr 23, 2024

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!

PR #16904, spec #16816

@DHowett DHowett added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 23, 2024
@zadjii-msft
Copy link
Member Author

As for user-specified commands, if no id is set, we will auto-generate one for that command based on the action and any additional arguments. For example, the split pane right command above might result in an autogenerated id User.SplitPaneRight.

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}}

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Apr 23, 2024
@DHowett
Copy link
Member

DHowett commented Apr 23, 2024

We will then restructure ... users' settings files ...:

  • ...
    • There will now be one json block ... which will also contain the id.

@DHowett
Copy link
Member

DHowett commented Apr 23, 2024

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.

@zadjii-msft
Copy link
Member Author

otherwise somebody could add an action to the menu with its internal ID

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)

@DHowett
Copy link
Member

DHowett commented Apr 23, 2024

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?

@zadjii-msft
Copy link
Member Author

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)

@DHowett
Copy link
Member

DHowett commented Apr 23, 2024

I've got a couple thoughts...

  • The JSON file is much less of a "UI" than it used to be, which frees us up to do things like this. That's not to say it's not a UI.
  • Getting to the ideal form of the settings file early is fine, if we're going to get there eventually.
  • It's like profiles that only have names: we always serialize GUIDs for them, because that's the only durable way to refer to them.
  • We made a mistake making action names or values into the hash keys, because it is complicated when you're using a resource or when you want to bind an entire action to multiple keys1 and stuff like that. Putting the durable ID up front immediately eliminates all of those concerns.

Footnotes

  1. Right now, I think you need N copies of the action (one per key) and if you want to change what it does you need to update all N copies (right?) That's a scenario today that would benefit from durable IDs.

@zadjii-msft
Copy link
Member Author

Right now, I think you need N copies of the action (one per key) and if you want to change what it does you need to update all N copies (right?)

We don't allow for actions anywhere other than in actions right now, actually. We've been holding off adding support for actions literally anywhere else until we got action IDs. New Tab Menu? We explicitly skipped actions in the v1 PR for that, because this wasn't done yet.

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.

@DHowett
Copy link
Member

DHowett commented Apr 23, 2024

We don't allow for actions anywhere other than in actions right now, actually.

That's not required for my scenario to exist!

image

What if I want to make alt-shift-d/e/l split: left instead? Gotta edit all three.

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?

@DHowett
Copy link
Member

DHowett commented Apr 23, 2024

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!

@carlos-zamora carlos-zamora added Needs-Attention The core contributors need to come back around and look at this ASAP. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements Needs-Attention The core contributors need to come back around and look at this ASAP. labels Apr 25, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed Needs-Tag-Fix Doesn't match tag requirements labels Apr 25, 2024
github-merge-queue bot pushed a commit that referenced this issue Apr 29, 2024
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
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants