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 #592

Merged
merged 1 commit into from
Nov 8, 2019

Conversation

Seiphon
Copy link
Contributor

@Seiphon Seiphon commented Nov 6, 2019

No description provided.

if (documentation != null) {
return documentation;
}
Map<String, String> commentsMap = document.getCommentsMap();
Copy link
Contributor

@angelozerr angelozerr Nov 6, 2019

Choose a reason for hiding this comment

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

I think CMDTDElementDeclaration should provide a new method CMDTDElementDeclaration#getDocumentation(String attrName) which uses the DTDElementInfo (see next comment)

Replace with

this.documentation = elementDecl.getDocumentation(getName())

@@ -124,6 +127,12 @@ public void startContentModel(String elementName, Augmentations augs) throws XNI
if (hierarchiesMap == null) {
hierarchiesMap = new HashMap<>();
}
if (commentsMap == null) {
Copy link
Contributor

@angelozerr angelozerr Nov 6, 2019

Choose a reason for hiding this comment

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

I think we should have just one Map hierarchiesMap which stores comment and hierarchies both (as value). To do that, I suggest that you create a private class which holds those information like:

static class DTDElementInfo {

	private final Set<String> hierarchies;
	private String comment;

	public DTDElementInfo() {
		this.hierarchies = new LinkedHashSet<String>();
		this.comment = null;
	}

	public Set<String> getHierarchies() {
		return hierarchies;
	}
	
	public String getComment() {
		return comment;
	}
		
	public void setComment(String comment) {
		this.comment = comment;
	}
}

hierarchiesMap should store String as key (like today) and DTDElementInfo as value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @angelozerr , by this way, we can collect the elements' comments and their child elements' comments. But how to collect attributes' commets? We can not just put attributes and its commet to hierarchiesMap, because there are some attributes have same name.

Copy link
Contributor

Choose a reason for hiding this comment

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

I forget the attribute case that you could manage like this:

static class DTDNodeInfo {

	private String comment;

	public DTDNodeInfo() {
		this.comment = null;
	}

	public String getComment() {
		return comment;
	}

	public void setComment(String comment) {
		this.comment = comment;
	}
}

static class DTDElementInfo extends DTDNodeInfo {

	private final Set<String> hierarchies;
	private final Map<String, DTDNodeInfo> attributes;
	
	public DTDElementInfo() {
		this.hierarchies = new LinkedHashSet<String>();
		this.attributes = new HashMap<>();
	}

	public Set<String> getHierarchies() {
		return hierarchies;
	}
	
	public String getComment(String attrName) {
		DTDNodeInfo attr = attributes.get(attrName);
		return attr != null ? attr.getComment() : null;
	}
}

commentsMap = new HashMap<>();
}
if (comment != null) {
commentsMap.put(elementName, comment);
Copy link
Contributor

Choose a reason for hiding this comment

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

use DTDElementInfo #setComment

@angelozerr
Copy link
Contributor

@Seiphon your PR looks promising but I think we should avoir creating a new Map for comments. I think we should have a map which contains information of DTDElementInfo (hierachies + comments) for each element name.

See my comment with DTDElementInfo.

The second thing is that you support comments for attribute declaration but nor for element delclaration. Please support it too.

@Seiphon Seiphon force-pushed the ElementDeclaration branch from 503bf4e to 1e75d53 Compare November 6, 2019 13:27
@@ -200,4 +250,46 @@ public LocationLink findTypeLocation(DOMNode node) {
public boolean isDirty() {
return tracker != null ? tracker.isDirty() : null;
}

static class DTDNodeInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move those 2 classes on the top of the CMDTDDocument class.

@Seiphon Seiphon force-pushed the ElementDeclaration branch from 1e75d53 to d6a6269 Compare November 6, 2019 13:38
@Seiphon
Copy link
Contributor Author

Seiphon commented Nov 6, 2019

Screenshot for element haver:
Screenshot (36)

Screenshot for attribute haver:
Screenshot (37)

The documentation style looks like these two pictures, it that ok?


private static void assertHover(String value, String expectedHoverLabel, Integer expectedHoverOffset)
throws BadLocationException {
XMLAssert.assertHover(new XMLLanguageService(), value, "src/test/resources/catalogs/service.xml", null,
Copy link
Contributor

@angelozerr angelozerr Nov 6, 2019

Choose a reason for hiding this comment

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

service.xml doesn't exist as catalog. Please rename service.xml to catalog-liferay.xml and create this file catalog-liferay.xml to references your liferay-service-builder_7_2_0.dtd

@angelozerr
Copy link
Contributor

The documentation style looks like these two pictures, it that ok?

For me I think it's a good start, perhaps we will improve it in an another PR (add title, format as markdown etc).

Please

  • catalog for test (see my below comments)
  • please update description of your commit like XSD 1.1 working #515

@Seiphon Seiphon force-pushed the ElementDeclaration branch from d6a6269 to c4e2b1d Compare November 6, 2019 14:20
@@ -0,0 +1,51 @@
<?xml version="1.0"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a XML catalog. Please update this file with

<?xml version="1.0"?>
<catalog xmlns="urn:oasis:names:tc:entity:xmlns:xml:catalog">
	<system
		systemId="http://www.liferay.com/dtd/liferay-service-builder_7_2_0.dtd"
		uri="../dtd/liferay-service-builder_7_2_0.dtd" />
</catalog>

To check that the XML catalog is working and uses the local liferay-service-builder_7_2_0.dtd, disconnect your Internet and try to execute your test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, just got it. the pr has been updated.

@Seiphon Seiphon force-pushed the ElementDeclaration branch from c4e2b1d to 1bddb88 Compare November 6, 2019 14:57
@angelozerr
Copy link
Contributor

angelozerr commented Nov 6, 2019

It's perfect @Seiphon please update description of your commit like #515

Fixes #585

Signed-off-by:  ...

@Seiphon Seiphon force-pushed the ElementDeclaration branch 2 times, most recently from 2dae487 to 296f9d1 Compare November 6, 2019 15:39
Fixes eclipse-lemminx#585

Signed-off-by: Seiphon Wang <seiphon.wang@liferay.com>
@Seiphon Seiphon force-pushed the ElementDeclaration branch from 296f9d1 to 9286aa0 Compare November 7, 2019 00:29
@Seiphon
Copy link
Contributor Author

Seiphon commented Nov 8, 2019

Hi @angelozerr, is that ok if I use commit description as follows:
DTD hover/completion support for documentation

Fixes #585

Signed-off-by: Seiphon Wang <seiphon.wang@liferay.com>

Is the title of the commit not corrent? or should I use DTD 1.1 working instead?

@angelozerr
Copy link
Contributor

yes it's perfect!

@Seiphon
Copy link
Contributor Author

Seiphon commented Nov 8, 2019

I have already updated the description of this commit as mentioned in last comment.
Screenshot (40)

@angelozerr angelozerr merged commit b0dbc6b into eclipse-lemminx:master Nov 8, 2019
@angelozerr
Copy link
Contributor

Thanks a lot @Seiphon for your great PR!

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.

2 participants