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

VS Code: use enablement for Cody commands #4155

Merged
merged 2 commits into from
May 13, 2024

Conversation

valerybugakov
Copy link
Member

@valerybugakov valerybugakov commented May 13, 2024

Test plan

  1. Start the extension locally.
  2. Disable autocomplete.
  3. Observe that the manual trigger command IS NOT present in the commands list (shift+cmd+P).
  4. Enable autocomplete.
  5. Observe that the manual trigger command IS present in the commands list (shift+cmd+P).

Can be done for all the commands based on the conditions specified in the enablement value.

Comment on lines 285 to 288
"command": "cody.command.edit-code",
"category": "Cody Command",
"title": "Edit Code",
"when": "cody.activated && editorTextFocus",
"enablement": "cody.activated && editorTextFocus",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cc @toolmantim @umpox, check out the "design input required" point in the PR description.
It affects most of the commands.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valerybugakov These current when conditions are unused and probably just copy-paste errors. This particular command is also hidden in the commandPallete setting like so:

    "menus": {
      "commandPalette": [
        {
          "command": "cody.command.edit-code",
          "when": "cody.activated && editorIsOpen"
        },

So I believe this works fine right now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philipp-spiess all autocomplete commands are still enabled when autocomplete is disabled. I think we should fix that.

Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the correct approach. most of the commands have entries in commandPalette already and are already gated out (these are in prod, after all) so we shouldn't change this.

For those that aren't I'd say we err on the side of removing the check to avoid breaking anything. See this: #4157

@valerybugakov
Copy link
Member Author

@philipp-spiess, do we want to turn off some commands entirely in some cases? e.g., multiModelCompletions when the setting is empty or autocomplete manual trigger when autocomplete is disabled?

@philipp-spiess
Copy link
Contributor

@valerybugakov What's the functional difference between turning-off via enabled and turning off via entries in commandsPallete and shortcuts with a when? 🤔

@valerybugakov
Copy link
Member Author

valerybugakov commented May 13, 2024

@philipp-spiess, probably one source of truth? If we know that the autocomplete manual trigger command should be disabled for both the command palette and the keybindings when autocomplete is disabled, should we do it in one place?

In the current vesrion, it does not work as expected, probably because we assumed when in commands works.

@valerybugakov
Copy link
Member Author

I think we should remove redundant when statements. For example, it's not needed for the edit command. But for a bunch of other commands, it's the right way to control on/off state. WDYT?

@philipp-spiess
Copy link
Contributor

@valerybugakov If it simplifies things than yeah, but I worry that we mess something up and break something in prod for this. Maybe we can check if commands have entries in both commandPalette and the keyboard shortcuts, we can move it to enablement instead? But that means we also can't trigger the command via code so we'd also need to grep for it inside the code I think, to ensure we don't have regressions (or maybe it's currently working unexpectedly too). Do you think it's worth it to spent this time now?

@valerybugakov
Copy link
Member Author

valerybugakov commented May 13, 2024

@philipp-spiess, from sources

Does not prevent executing the command by other means, like the executeCommand-api.')

So it should not break anything if we remove redundant when statements and convert the rest to enablement. I think this simplifies the setup and we won't need to invest additional time into it.

@valerybugakov valerybugakov merged commit 69aec78 into main May 13, 2024
18 checks passed
@valerybugakov valerybugakov deleted the vb/commands-configuration branch May 13, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants