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

Added request for 'canBindGrammar' for vscode-xml command #1084

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

AlexXuChen
Copy link
Contributor

Added request for 'canBindGrammar' for vscode-xml 'Open Binding Wizard' command.

References: redhat-developer/vscode-xml/pull/544

Signed-off-by: Alexander Chen alchen@redhat.com

@datho7561 datho7561 added this to the 0.17.2 milestone Jul 13, 2021
@angelozerr
Copy link
Contributor

To be consistent with binding grammar command please use custom command instead of adding new method to the xml language server and services

@AlexXuChen
Copy link
Contributor Author

To be consistent with binding grammar command please use custom command instead of adding new method to the xml language server and services

Atm this is a custom command on the client side, where it uses the commands.executeCommand('setContext', 'canBindGrammar', result) (See the pr on vscode-xml). Is this what you're referring to?

@AlexXuChen
Copy link
Contributor Author

This isn't a command itself though, it is the check to see if the command defined on the vscode-xml side should be exposed or not.

For example, if we have an xml document:

<tag
   attr1=""
   attr2=""/>

then the command I made in vscode-xml should be exposed on the command palette (XML: Bind to grammar/schema)

If instead we have:

<tag xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
 xsi:noNamespaceSchemaLocation="foo.xsd"
   attr1=""
   attr2=""/>

Then you shouldn't be able to find the command on the command palette.

The command itself is defined and registered in vscode-xml, the check is the communication between the client and server to see if the command should run at all.

So the command to associate grammar, https://github.com/eclipse/lemminx/blob/master/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/extensions/contentmodel/commands/AssociateGrammarCommand.java, is the command that will be run if the client exposes the "Bind to grammar/schema file", which I believe means I don't need a new command on the lemminx side.

Please correct me if I've misunderstood.

@AlexXuChen
Copy link
Contributor Author

Has grammar? -> yes -> Dont expose "Bind to ..."
I
-> no -> Expose "Bind to ..." -> Call binding wizard (xml.open.binding.wizard) -> Bind grammar (xml.command.bind.grammar)

@AlexXuChen
Copy link
Contributor Author

Just to confirm, I understand your ask, which is to keep this out of XMLLanguageServer.java, and do something similar to the command, however when looking through the command and plugin, I was unable to find the entry point to the server.

@angelozerr
Copy link
Contributor

Yes i understand that. In the vscode xml you create a new request type but please dont do that since now we have the capability to consume command line we did with xml.associate.grammar.insert. please consume the new xml.associat.grammar.canBinding command on vscode-xml side registered on lemminx side

@angelozerr
Copy link
Contributor

Yes i confirm that.

To understand more command
Please read https://github.com/eclipse/lemminx/blob/master/docs/LemMinX-Extensions.md#command-service

@AlexXuChen AlexXuChen marked this pull request as ready for review July 19, 2021 16:12
@angelozerr
Copy link
Contributor

angelozerr commented Jul 20, 2021

@AlexXuChen
Copy link
Contributor Author

Writing tests in progress

@AlexXuChen AlexXuChen requested a review from angelozerr July 22, 2021 13:57
@angelozerr angelozerr merged commit d633fbc into eclipse-lemminx:master Jul 22, 2021
@angelozerr
Copy link
Contributor

Great job @AlexXuChen !

@AlexXuChen AlexXuChen deleted the issue514_xml branch July 22, 2021 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants