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

CodeAction and CodeActionProviderMetaData fields added #10073

Closed
wants to merge 1 commit into from

Conversation

Archie27376
Copy link
Contributor

@Archie27376 Archie27376 commented Sep 9, 2021

What it does

Fixes #9989

  • added support for the disabled and isPreferred fields to the CodeAction interface
  • added support for the documentation field to CodeActionProviderMetaData interface

How to test

  1. include the vscode-code-action-test test plugin
  2. start the application and open a markdown file
  3. type :) in the file
  4. the quick-fix should display entries - the cat emoji entry is explicitly hidden (will be present on master)

Review checklist

Reminder for reviewers

@Archie27376 Archie27376 force-pushed the gh9989 branch 2 times, most recently from bfae798 to f06fff2 Compare September 9, 2021 20:39
@vince-fugnitto vince-fugnitto added the vscode issues related to VSCode compatibility label Sep 10, 2021
@msujew
Copy link
Member

msujew commented Sep 10, 2021

@tsmaeder Do you have an example extension at hand with which one can test these changes?

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@Archie27376 using the test plugin vscode-code-action I was able to confirm at least for the CodeAction changes that there is still work to do.

bug-code-action.mp4

The cat result in the quick-fix should not be displayed (similarly to vscode) as it is explicitly disabled.

The fix is quite simple however.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I confirmed that the behavior works correctly with the test plugin 👍

The cat result is explicitly disabled and not visible similarly to vscode:

code-action-test.mp4

*
* At most one documentation entry will be shown per provider.
*/
documentation?: ReadonlyArray<{ command: Command, kind: CodeActionKind }>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how the documentation field is transferred to the main side and ultimately the monaco editor.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can omit support for documentation for now, it is used when registering registerCodeActionsProvider but our implementation differs from vscode in many ways:

registerCodeActionsProvider(
selector: theia.DocumentSelector,
provider: theia.CodeActionProvider,
pluginModel: PluginModel,
pluginInfo: PluginInfo,
metadata?: theia.CodeActionProviderMetadata
): theia.Disposable {
const callId = this.addNewAdapter(new CodeActionAdapter(provider, this.documents, this.diagnostics, pluginModel ? pluginModel.id : '', this.commands));
this.proxy.$registerQuickFixProvider(
callId,
pluginInfo,
this.transformDocumentSelector(selector),
metadata && metadata.providedCodeActionKinds ? metadata.providedCodeActionKinds.map(kind => kind.value!) : undefined
);
return this.createDisposable(callId);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

But ultimately, we're registering the code action provider to the editor, which does have a documentation field. We should pass this info to the editor in Theia, as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tsmaeder Would you know how to properly pass the documentation field? I myself am not sure how to go about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Our definition of modes.CodeActionProvider is way out of date. We need to update our definition in monaco.d.ts and pass the necessary information to the front end through "registerQuickFixProvider". The current definition in monaco actually looks like this: https://github.com/microsoft/vscode/blob/2586299c42490a8ed6dff96ed731d44fcbe5b20e/src/vs/editor/common/languages.ts#L777

Copy link
Contributor

Choose a reason for hiding this comment

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

I looks like in the branch we currently use (0.23) of vs code, the definition lives here: (https://github.com/microsoft/vscode/blob/b882940dc6d5b2acb096969b9e43385248719df7/src/vs/editor/common/modes.ts#L740).

I believe it would work to extend the definition of CodeActionProvider in monaco.d.ts with the needed bits.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Archie27376 : Please have a look at the hints that Thomas provided and let us know if you need further information!

Copy link
Member

Choose a reason for hiding this comment

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

@JonasHelming Dante's (@Archie27376) internship has terminated, someone else would need to pick up the pull-request.

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

Successfully merging this pull request may close these issues.

Implement missing CodeAction fields
5 participants