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

Handle case of empty command id with arguments #9223

Merged
merged 2 commits into from
Mar 26, 2021

Conversation

tsmaeder
Copy link
Contributor

Signed-off-by: Thomas Mäder tmader@redhat.com

What it does

This issue fixes the case where a command is transferred that has a non-empty argument list, but an empty command id. This is supposed to fix eclipse-che/che#18921 in Eclipse Che. The case of the git plugin is not reproducible for me in plain Theia (looks like it's timing-dependent), so I made an example with code actions to reproduce the problem.

How to test

  1. Add the attached vsix to a theia instance and start Theia
    prefs-explorer-0.0.1.vsix.zip
  2. Open a any text file and select some text
  3. You should get a code action labeled "bogus title".

Review checklist

Reminder for reviewers

Signed-off-by: Thomas Mäder <tmader@redhat.com>
@tsmaeder
Copy link
Contributor Author

Here's the relevant code that contributes the code action:

	function provideCodeActions(document: vscode.TextDocument, range: vscode.Range | vscode.Selection, context: vscode.CodeActionContext, token: vscode.CancellationToken): vscode.ProviderResult<(vscode.Command | vscode.CodeAction)[]> {
		const circularStruct: {
			innerValue: any;
		} = {
			innerValue: undefined
		}

		circularStruct.innerValue= circularStruct;
		
		const cmd: vscode.Command= {
			command: '',
			arguments: [circularStruct],
			title: 'bogus title',
			tooltip: 'bogus tooltip'
		};
		return [cmd];
	}

@tsmaeder tsmaeder requested review from vinokurig and akosyakov March 19, 2021 12:37
@vince-fugnitto vince-fugnitto added commands issues related to application commands vscode issues related to VSCode compatibility labels Mar 19, 2021
Signed-off-by: Thomas Mäder <tmader@redhat.com>
@tsmaeder
Copy link
Contributor Author

@kittaakos @vinokurig can I get a "hallelujah" on this one?

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

It looks good to me. Thank you for the patch 👍

@tsmaeder tsmaeder merged commit 76bc164 into eclipse-theia:master Mar 26, 2021
@azatsarynnyy
Copy link
Member

FYI it breaks the downstream eclipse-che/che#19441

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commands issues related to application commands vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Circular struture" errors when accepting "current change"
5 participants