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

fix bug:The datas of Complete exists repeatedly #6130

Closed
wants to merge 1 commit into from
Closed

fix bug:The datas of Complete exists repeatedly #6130

wants to merge 1 commit into from

Conversation

xieyuanhuata
Copy link
Contributor

@xieyuanhuata xieyuanhuata commented Sep 6, 2019

Signed-off-by: xieyuanhu xieyuanhuata@163.com

What it does

fix #6128 If the selectors and triggerCharacters of the two plugins are the same, then the datas of Complete will be duplicated.So,it is no need to generate two adapters for plugins with the same selector and triggerCharacters.

How to test

1、Install vscode plugins and the hello-plugin into theia,and then start theia;
redhat.java-0.46.0.vsix --- https://marketplace.visualstudio.com/items?itemName=redhat.java
vscjava.vscode-java-debug-0.20.0.vsix --- https://marketplace.visualstudio.com/items?itemName=vscjava.vscode-java-debug
vscjava.vscode-java-dependency-0.4.0.vsix -- https://marketplace.visualstudio.com/items?itemName=vscjava.vscode-java-dependency
vscjava.vscode-maven-0.17.0.vsix --- https://marketplace.visualstudio.com/items?itemName=vscjava.vscode-maven
vscode-java-test-0.18.2.vsix --- https://marketplace.visualstudio.com/items?itemName=vscjava.vscode-java-test
2、Open a java project,and insert a chart;

Review checklist

Reminder for reviewers

@xieyuanhuata xieyuanhuata requested a review from a team as a code owner September 6, 2019 13:31
@akosyakov
Copy link
Member

Please elaborate what is the root cause and how this PR supposed to fix it?

@akosyakov akosyakov added the plug-in system issues related to the plug-in system label Sep 6, 2019
@xieyuanhuata
Copy link
Contributor Author

Please elaborate what is the root cause and how this PR supposed to fix it?

OK

Signed-off-by: xieyuanhu <xieyuanhuata@163.com>
@akosyakov akosyakov added the 🤔 needs more info issues that require more info from the author label Sep 9, 2019
}

private addNewAdapter(adapter: Adapter, selector?: theia.DocumentSelector, triggerCharacters?: string[]): number {
const callId = selector ? this.nextNewCallId(selector, triggerCharacters) : this.nextCallId();
Copy link
Member

Choose a reason for hiding this comment

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

Please explain what is supposed to fix and how?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is a need to create multiple duplicate adapters for scenes with the same selector and triggerCharacters.

Copy link
Member

Choose a reason for hiding this comment

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

2 different vscode extensions contribute different providers for the same selector and trigger characters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This situation is extreme but still exists, such as: vscode intellicode and redhat-java

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand an issue with that. Each provider should be handled independent and has own id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need to do something specific for this particular scenes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If deduplication is done at the data level, there is bound to be loss in overall performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akosyakov
My solution is:
If the selector and the triggerCharacters are the same, then only one provider is provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, this problem also exists in vscode, I think this is a flaw in the design of vscode.

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 you should file a bug against vscode intellicode and redhat-java, and ask whether they supposed to be used together. It does not seem like that to me.

Code wise changes don't look good, it will lead to issues in other cases.

@tsmaeder
Copy link
Contributor

Having multiple sources for language-related features like code completion is not a mistake, it's explicitly a feature of the VS Code API and LSP. For example, there is a language server providing completion for Apache Camel stuff inside Java files.
The VS Code documentation even defines rules on how to handle (merge, for example) results when multiple extensions could provide results.
Sorry, but what the intent of this PR is not something we want. I don't think we should accept this PR.

@akosyakov
Copy link
Member

akosyakov commented Sep 10, 2019

The VS Code documentation even defines rules on how to handle (merge, for example) results when multiple extensions could provide results.

@tsmaeder Could you provide a link to it? Could it be that our Monaco version does not support it well?

@xieyuanhuata Could you try against #5901?

@xieyuanhuata
Copy link
Contributor Author

xieyuanhuata commented Sep 10, 2019

@tsmaeder
I have looked at the vscode document and there is no configuration item for the datas of Complete to be merged.Could you describ briefly the merge rules ?

@tsmaeder
Copy link
Contributor

tsmaeder commented Sep 10, 2019

@tsmaeder Could you provide a link to it? Could it be that our Monaco version does not support it well?

If you look, for example at "registerCompletionProvider" here, it mentions what to do when there are multiple providers registered for the same document selector. There are similar rules for other providers described in the document.

@vince-fugnitto
Copy link
Member

Thank you for your contribution!

Unfortunately, the pull-request is outdated with conflicts (>1 year old) and there has not been any recent activity.
If you still plan on updating it, please provide your changes and it can be re-opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system 🤔 needs more info issues that require more info from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The datas of Complete exists repeatedly
4 participants