-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[SCM] Fix SCM status bar commands #6208
Conversation
@@ -64,6 +64,7 @@ export interface ScmResourceDecorations { | |||
export interface ScmCommand { | |||
title: string; | |||
tooltip?: string; | |||
id?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it not it confusing that id is used as command as well? Can we instead fix it in the plugin system, that id is assigned to a command. I don't think we should pull deprected apis in the plugin system into other extensions.
Also command
and title
are not optional in vscode.d.ts: https://github.com/microsoft/vscode/blob/6de911e3298424cfb6dd6792bff5499f1cd6a230/src/vs/vscode.d.ts#L19-L29
I wonder why there are optional for us? Can we fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it not it confusing that id is used as command as well? Can we instead fix it in the plugin system, that id is assigned to a command.
done
Also
command
andtitle
are not optional in vscode.d.ts: https://github.com/microsoft/vscode/blob/6de911e3298424cfb6dd6792bff5499f1cd6a230/src/vs/vscode.d.ts#L19-L29I wonder why there are optional for us? Can we fix it?
The Git extension may set an SCM command item without an action:
theia/packages/git/src/browser/git-contribution.ts
Lines 647 to 652 in 2a97874
if (this.syncService.isSyncing()) { | |
return { | |
title: '$(refresh~spin)', | |
tooltip: 'Synchronizing Changes...' | |
}; | |
} |
576d926
to
eae45da
Compare
id: string; | ||
command?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after looking closer, actually it has to be id
here, because this Command
should be aligned with monaco command: https://github.com/microsoft/monaco-editor/blob/dc793af0020b9878b798b45fdcaf31e1b263c40b/monaco.d.ts#L5536-L5541, even though that vscode APIs are using command
not id
I think we should fix typings here:
theia/packages/plugin-ext/src/common/plugin-api-rpc.ts
Lines 610 to 612 in eae45da
acceptInputCommand?: ScmCommand; | |
statusBarCommands?: ScmCommand[]; | |
} |
i.e.:
acceptInputCommand?: Command;
statusBarCommands?: Command[];
since we actually send plugin commands not scm commands and then do adjustment in scm-main
eae45da
to
657bda8
Compare
What it does
Pass command arguments to SCM status bar commands. Pass
Command.id
to SCM status bar commands ifCommand.command
is undefined.How to test
/
SCM repository (this is a view of a dummy SCM repository):title
button in the status bar:See the notification
fixes eclipse-che/che#13974
Review checklist
Reminder for reviewers