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

Developer command palette entries should always be at the bottom #25915

Closed
Tyriar opened this issue May 4, 2017 · 21 comments
Closed

Developer command palette entries should always be at the bottom #25915

Tyriar opened this issue May 4, 2017 · 21 comments
Labels
feature-request Request for new features or functionality quick-pick Quick-pick widget issues

Comments

@Tyriar
Copy link
Member

Tyriar commented May 4, 2017

I've built muscle memory for switching themes via the command palette which I'm having difficulty getting rid of. Regardless, I don't think the first result for "theme" should be generating theme json.

image

@Tyriar Tyriar added the under-discussion Issue is under discussion for relevance, priority, approach label May 4, 2017
@jrieken jrieken removed their assignment May 5, 2017
@bpasero
Copy link
Member

bpasero commented May 5, 2017

I see the issue but our command picker has a strict sorting by alphabet and nothing else. I tried to just sort by shortest match, but that caused other frustration (see #25682)

Duplicates: #14727, #20553, #1964

@bpasero bpasero closed this as completed May 5, 2017
@bpasero bpasero added *duplicate Issue identified as a duplicate of another issue(s) and removed under-discussion Issue is under discussion for relevance, priority, approach labels May 5, 2017
@Tyriar
Copy link
Member Author

Tyriar commented May 5, 2017

@bpasero could it not be strict alphabetical sorting with the exception of "Developer" prefixed commands which are always at the bottom of the list? That way the dev/debugging stuff doesn't get in the way for regular users.

@bpasero
Copy link
Member

bpasero commented May 5, 2017

@Tyriar possible, but how do you know it is of that category? For us it is currently just a label, so we need to introduce the concept of "developer" commands.

@Tyriar
Copy link
Member Author

Tyriar commented May 5, 2017

@bpasero I guess it would need that yeah.

@bpasero bpasero reopened this May 5, 2017
@bpasero bpasero added feature-request Request for new features or functionality workbench and removed *duplicate Issue identified as a duplicate of another issue(s) labels May 5, 2017
@bpasero bpasero added this to the Backlog milestone May 5, 2017
@bpasero bpasero removed their assignment May 5, 2017
@bpasero
Copy link
Member

bpasero commented May 5, 2017

PR welcome.

@bpasero bpasero added the help wanted Issues identified as good community contribution opportunities label May 5, 2017
@bpasero bpasero removed this from the Backlog milestone May 5, 2017
@Tyriar
Copy link
Member Author

Tyriar commented May 5, 2017

@bpasero where abouts is the code for the command palette sorting?

@ihalip
Copy link
Contributor

ihalip commented May 5, 2017

I think the sorting is being done in src/vs/base/parts/quickopen/browser/quickOpenModel.ts, see QuickOpenEntry.compare().

@kdmu
Copy link
Contributor

kdmu commented May 9, 2017

Hi @Tyriar are you working on this issue? If not I would like to pick it.

@Tyriar
Copy link
Member Author

Tyriar commented May 9, 2017

@kdmu I was going to but it would be great if you could help out 😃

@kdmu
Copy link
Contributor

kdmu commented May 9, 2017

@Tyriar Let's give it a try 😃

Which kind of filtering are we expecting to have after this issue? Is enough if we just filter out developer commands based on entry label prefix? Or we want to introduce the notion of command type, let's say developer and nonDeveloper?

@Tyriar
Copy link
Member Author

Tyriar commented May 9, 2017

@kdmu I'm expecting exactly the same behavior as now, just "Developer" prefixes entries are in a sorted list below the main sorted list. You would likely need to introduce a new "isDeveloperCommand` flag to commands so that we don't need to do a string search on start up.

@kdmu
Copy link
Contributor

kdmu commented May 9, 2017

@Tyriar not very clear about what do you mean by string search on start up. Adding isDeveloperCommand to BaseCommandEntry will require us to do a string matching with label. The only way I can imagine that we can avoid this is add the flag when we instantiate the command itself like here, however it seems it may lead to inconsistency.

My initial approach was filter out developer commands using a regex and sort both entries separately, the behavior will be the same but it will require us do string matching whenever commandHandler is invoked.

@Tyriar
Copy link
Member Author

Tyriar commented May 10, 2017

@bpasero please advise the ideal solution 😃

@bpasero
Copy link
Member

bpasero commented May 10, 2017

I think having a flag when creating these commands that identifies normal commands from developer commands is the only way that works across all languages, where "Developer" might be translated to something else.

@kdmu
Copy link
Contributor

kdmu commented May 10, 2017

@bpasero not familiar with vscode code base, I'll need some guidance about command's hierarchy.

I see those "Developer" commands are different type of commands? Taking as example: "Developer: Inspect Key Mappings" is instantiate as Command, while most of developer commands are like "Developer: Generate Color Theme From Current Settings" instantiated as ICommand though workbenchActionsRegistry

I'm guessing there's "parent" that all "Developer" share in common, is that true? If not I guess we will need to modify both type of commands.

Developer actions

@bpasero
Copy link
Member

bpasero commented May 10, 2017

I suggest to look at the registerWorkbenchAction method and add an overload that allows to declare a command as being a developer command (or any kind of category). E.g. instead of having category?: string we could have an overload that accepts { label: string, kind: enum }

@kdmu
Copy link
Contributor

kdmu commented May 11, 2017

While most of "Developer" commands are WorkbenchAction and registered through registerWorkbenchAction, I found some of them do not, there's EditorAction like "Developer: Inspect <Key Mappings|TM Scopes>" registered through other mechanism or "Developer: Webview Tools" being registered through CommandsRegistry and MenuRegistry.

@bpasero are my thoughts on the right track? If they are I guess we may need to take care of those non-WorkbenchAction apart from overloading registerWorkbenchAction (?)

@bpasero
Copy link
Member

bpasero commented May 11, 2017

Yeah tricky. Sorry I am not having enough time to look into this further at the moment.

@kdmu
Copy link
Contributor

kdmu commented May 11, 2017

If we can afford the cost of string matching, a possible solution could be filtering entries based on label and alias. If I am not wrong, although label is translated in the destination language, alias is kept in english.

@bpasero bpasero removed the help wanted Issues identified as good community contribution opportunities label May 19, 2017
@bpasero bpasero added quick-pick Quick-pick widget issues and removed workbench labels Nov 14, 2017
@bpasero
Copy link
Member

bpasero commented Nov 17, 2017

No plans to change this.

@bpasero bpasero closed this as completed Nov 17, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality quick-pick Quick-pick widget issues
Projects
None yet
Development

No branches or pull requests

5 participants