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

Extensions: Command callback arguments are getting harder to deal with #25716

Closed
eamodio opened this issue May 1, 2017 · 6 comments
Closed
Assignees
Labels
api *out-of-scope Posted issue is not in scope of VS Code under-discussion Issue is under discussion for relevance, priority, approach

Comments

@eamodio
Copy link
Contributor

eamodio commented May 1, 2017

  • VSCode Version: Code - Insiders 1.12.0-insider (0bec115, 2017-04-28T18:57:52.518Z)
  • OS Version: Windows_NT ia32 10.0.16184

In the docs registerCommand has callback args of args: any[], while registerTextEditorCommand as args of (textEditor: TextEditor, edit: TextEditorEdit, args: any[]

These, of course, are accurate, but don't mention that when a command is called from different contexts some extra arguments may be included at the start of the args array. OK, that isn't entirely true, it is mentioned in the note here but it also isn't 100% accurate -- as the inference can also happen from the command palette as well as possibly other cases.

Anyway, so say a command is triggered from file explorer item context menu, or an editor context menu -- that the first argument in the args: any[] is going to be a Uri, and now with the new SCM menus the command callback args can contain an arbitrary number of items (the SCM resources selected). And compound that with command execution via executeCommand as well as keybindings supporting argument passing -- its getting hard to deal with the differing args.

Sure, different commands can be registered for different contexts, but that doesn't feel right, especially with shortcut keys, command palette entries, etc (yes, these can be dealt with in most cases with when clauses, but that feels like extra and unneeded complexity -- at least imo).

Honestly I'm not really sure how to solve this, and maybe the recommended solution is to use different commands for different contexts, but maybe there is another way?

One thought is to somehow separate "system-supplied" arguments, vs "user-supplied", or have some sort of "context" parameter (at a known and stable position) so that the type of arguments could be inferred. Or maybe a combination of both, something like:

declare type CommandSource = 'executeCommand' | 'commandPalette' | 'explorer/context' | 'editor/context' | 'editor/title'; //  ... you get the picture

callback: (source: CommandSource, sourceArgs: any[], args: any[]) => any

// OR -- better yet imo

interface EditorCommandContext {
    textEditor: TextEditor;
    edit: TextEditorEdit;
    uri?: Uri;
}

interface ExplorerCommandContext {
    uri?: Uri;
}

interface ScmCommandContext {
    // ??? - Haven't dug into the specifics of this one yet
}

callback: (source: CommandSource, context: {} | EditorCommandContext | ExplorerCommandContext | ScmCommandContext, args: any[]) => any)

With something like the last option, it is very extensible (its easy to add another property to the context without breaking any existing code) and its also easier to document and consume. It would also have the added benefit of providing an extension with insight to in what context a command was executed.

Of course changing the existing callbacks would be a major breaking change, but this could be a new command registration api (with the maybe eventual deprecation of the existing command registration?).

To give a concrete example of the issues outlined above -- I'm currently working on refactoring the commands in GitLens, so that I can add context menu support to SCM items. But this has been proving more complex than I had first anticipated because of the argument structure of the SCM menus, as well as not being able to tell one context from another. So I've been toying with a few different solutions -- from attempting argument inference in a command base class, that would reorganize the arguments into a more structured layout -- to multiple commands registered for different contexts, and then boiling them together in a sort of "meta" command that would provide something like the callback outlined above.

In summary, I feel like the current command callback structure is only going to get harder to deal with as the product evolves and more contexts are added -- so I'm very curious for feedback, alternative solutions, etc.

Thanks!

//cc @jrieken

@kieferrm kieferrm added the api label May 1, 2017
@jrieken jrieken added the under-discussion Issue is under discussion for relevance, priority, approach label May 2, 2017
@jrieken
Copy link
Member

jrieken commented Jun 28, 2017

Not really sure if knowing from where you have been invoked help. Being command you must be prepared for being invoked with no arguments and for being invoked with bad arguments. Also, when-contexts don't protect you from being called when you don't want to be called. As of today, they are just user-recommendation. Tho, we do have ideas to add something like command-precondition-when-clauses that ensure your command is never invoked unless a certain (when)-condition holds.

Wrt extending SCM it seems like you want API from the SCM extension, e.g to deal with the SCM object model. We do have support for this in the API but little experience.

@eamodio
Copy link
Contributor Author

eamodio commented Jun 28, 2017

@jrieken funny you replied to this today -- I had written up the below and apparently forgot to hit send last night ;)

FYI, I have employed something close to this in GitLens -- see here. While it is not ideal with of all the inferring that must be done, it is much easier to consume. I'm are also starting using it on the custom view work going on in GitLens as well.

@eamodio
Copy link
Contributor Author

eamodio commented Apr 30, 2018

Any further discussion on this? As I work on new extensions and deal with command that can be executed from difference contexts I feel this pain. It would be great if there was a way this could be addressed.

@jrieken
Copy link
Member

jrieken commented Apr 30, 2018

No, we have no plans to change how commands are invoked

@eamodio
Copy link
Contributor Author

eamodio commented Apr 30, 2018

😢

@jrieken jrieken added the *out-of-scope Posted issue is not in scope of VS Code label May 31, 2018
@vscodebot
Copy link

vscodebot bot commented May 31, 2018

This issue is being closed to keep the number of issues in our inbox on a manageable level, we are closing issues that have been on the backlog for a long time but have not gained traction: We look at the number of votes the issue has received and the number of duplicate issues filed. If you disagree and feel that this issue is crucial: We are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding and happy coding!

@vscodebot vscodebot bot closed this as completed May 31, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Jul 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api *out-of-scope Posted issue is not in scope of VS Code under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

3 participants