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

DTD hover/completion support for documentation #585

Closed
Seiphon opened this issue Oct 25, 2019 · 9 comments · Fixed by enxio/lsp4xml#10
Closed

DTD hover/completion support for documentation #585

Seiphon opened this issue Oct 25, 2019 · 9 comments · Fixed by enxio/lsp4xml#10
Assignees
Labels
completion This issue or enhancement is related to completion support DTD enhancement New feature or request hover
Milestone

Comments

@Seiphon
Copy link
Contributor

Seiphon commented Oct 25, 2019

Hi @angelozerr and @fbricon ,

I found when do hover and code completion, the prompt information is not expected.
How do you think about this issue?
Please see the picture:
hover:
Screenshot (33)
code completion:
Screenshot (28)

The expected result should like follows (from Eclipse original XML editor):
hover:
Screenshot (32)
code completion:
Screenshot (29)

cc @jtydhr88

@fbricon
Copy link
Contributor

fbricon commented Oct 25, 2019

is this a follow up on #577? Could you provide a sample document reproducing this issue?

@Seiphon
Copy link
Contributor Author

Seiphon commented Oct 25, 2019

No, this is a independent issue. I will upload a service.xml for you to test.

service.zip

@fbricon
Copy link
Contributor

fbricon commented Oct 25, 2019

I see. The comments before the element declaration in the DTD is expected to serve as documentation:

<!--
An entity usually represents a business facade and a table in the database. If
an entity does not have any columns, then it only represents a business facade.
The Service Builder will always generate an empty business facade POJO if it
does not exist. Upon subsequent generations, the Service Builder will check to
see if the business facade already exists. If it exists and has additional
methods, then the Service Builder will also update the SOAP wrappers.

If an entity does have columns, then the value object, the POJO class that
is mapped to the database, and other persistence utilities are also generated
based on the order and finder elements.
-->
<!ELEMENT entity (column*, localized-entity?, order?, finder*, reference*, tx-required*)>

@angelozerr should know better how it should be to implemented

@fbricon fbricon added completion This issue or enhancement is related to completion support enhancement New feature or request hover DTD labels Oct 25, 2019
@angelozerr
Copy link
Contributor

At first I noticed that hover has bad renderer with LSP4E. I have never found time to study this case.

You can see the trouble in

Screenshot (33)

Why we cannot see the whole source link? LSP4E should support markdown, but I'm not sure LSP4XML receive this capability. Is a problem with markdown converter (mylin markdown is used for that)?

The comments before the element declaration in the DTD is expected to serve as documentation:

Ok the main problem that I see is that we don't collect documentation for DTD.

Today we don't support documentation for DTD https://github.com/angelozerr/lsp4xml/blob/master/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/extensions/dtd/contentmodel/CMDTDElementDeclaration.java#L94

Today we collect hierarchy element https://github.com/angelozerr/lsp4xml/blob/master/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/extensions/dtd/contentmodel/CMDTDDocument.java#L133

I think to manage comment, we should collect it

public class CMDTDDocument {

       @Override
	public void comment(XMLString text, Augmentations augs) throws XNIException {
                // TODO: Here we must attach the text comment with the next element declaration          
		super.comment(text, augs);
	}

}

@Seiphon is there something that you could try it?

@angelozerr
Copy link
Contributor

@Seiphon for the problem with LSP4E, https://bugs.eclipse.org/bugs/show_bug.cgi?id=552429 was merged and now you should see an hyperlink to open the XML Schema:

LSP4XMLMarkdown

@Seiphon
Copy link
Contributor Author

Seiphon commented Oct 28, 2019

Thanks @angelozerr , so the next step is to impletement the collection of DTD documentation, right?
I will try to work on it this week.

@angelozerr
Copy link
Contributor

angelozerr commented Oct 28, 2019

so the next step is to impletement the collection of DTD documentation, right?
I will try to work on it this week.

Exactly! It's just an idea, if you find a better mean please do that. Please write test too with this feature. To do that, I suggest you write your test in org.eclipse.lsp4xml.extensions.contentmodel.DTDHoverExtensionsTest like we have for XSD https://github.com/angelozerr/lsp4xml/blob/master/org.eclipse.lsp4xml/src/test/java/org/eclipse/lsp4xml/extensions/contentmodel/XMLSchemaHoverExtensionsTest.java

@angelozerr angelozerr changed the title The description of xml element is not expected on the prompt imformation of hover and code-completion DTD hover/completion support for documentation Nov 2, 2019
@Seiphon
Copy link
Contributor Author

Seiphon commented Nov 6, 2019

Hi @angelozerr , sorry for delay. I implement it by my understanding on this pr #592 . If there is something not correct, please let me know, I will update it.

Seiphon added a commit to Seiphon/lsp4xml that referenced this issue Nov 6, 2019
Fixes eclipse-lemminx#585

Signed-off-by: Seiphon Wang [email protected]
Seiphon added a commit to Seiphon/lsp4xml that referenced this issue Nov 6, 2019
Fixes eclipse-lemminx#585

Signed-off-by: Seiphon Wang <[email protected]>
Seiphon added a commit to Seiphon/lsp4xml that referenced this issue Nov 7, 2019
@angelozerr
Copy link
Contributor

@Seiphon your PR is ready to merge. Once you will fix commit description #592 (comment) I would like to merge your great PR

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completion This issue or enhancement is related to completion support DTD enhancement New feature or request hover
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants