Skip to content

Support advanced characters for entity name.#718

Merged
angelozerr merged 1 commit into
eclipse-lemminx:masterfrom
angelozerr:entities-improve
May 26, 2020
Merged

Support advanced characters for entity name.#718
angelozerr merged 1 commit into
eclipse-lemminx:masterfrom
angelozerr:entities-improve

Conversation

@angelozerr

Copy link
Copy Markdown
Contributor

Support advanced characters for entity name.

See redhat-developer/vscode-xml#262

Signed-off-by: azerr azerr@redhat.com

@angelozerr angelozerr marked this pull request as ready for review May 25, 2020 09:11
@angelozerr

Copy link
Copy Markdown
Contributor Author

This PR fixes the issue Entity names with underscore and backspace (the 4 issue from redhat-developer/vscode-xml#262).

I did some refactoring to use the same code for entity range diagnostics, completion, and definition

@fbricon

fbricon commented May 25, 2020

Copy link
Copy Markdown
Contributor

I don't know if this is caused by this PR, but given the following xml:

<!DOCTYPE web-app PUBLIC "-//Sun Microsystems, Inc.//DTD Web Application 2.3//EN" "http://java.sun.com/dtd/web-app_2_3.dtd"[
<!ENTITY mdash "&#x2014;">
]>
m| 
<web-app>
  <display-name>Servlet 2.3 Web Application</display-name>
</web-app>

when typing anything outside the nodes, I get an NPE:

[Error - 2:10:18 PM] May 25, 2020 02:10:18 org.eclipse.lemminx.services.XMLCompletions collectOpenTagSuggestions()
Message: While performing ICompletionParticipant#onTagOpen
java.lang.IllegalArgumentException: Property must not be null: label
	at org.eclipse.lsp4j.util.Preconditions.checkNotNull(Preconditions.java:29)
	at org.eclipse.lsp4j.CompletionItem.<init>(CompletionItem.java:144)
	at org.eclipse.lemminx.extensions.contentmodel.participants.ContentModelCompletionParticipant.addCompletionItem(ContentModelCompletionParticipant.java:244)
	at org.eclipse.lemminx.extensions.contentmodel.participants.ContentModelCompletionParticipant.fillWithChildrenElementDeclaration(ContentModelCompletionParticipant.java:183)
	at org.eclipse.lemminx.extensions.contentmodel.participants.ContentModelCompletionParticipant.onTagOpen(ContentModelCompletionParticipant.java:65)
	at org.eclipse.lemminx.services.XMLCompletions.collectOpenTagSuggestions(XMLCompletions.java:608)
	at org.eclipse.lemminx.services.XMLCompletions.collectInsideContent(XMLCompletions.java:733)
	at org.eclipse.lemminx.services.XMLCompletions.doComplete(XMLCompletions.java:224)
	at org.eclipse.lemminx.services.XMLLanguageService.doComplete(XMLLanguageService.java:139)
	at org.eclipse.lemminx.XMLTextDocumentService.lambda$completion$1(XMLTextDocumentService.java:189)
	at java.base/java.util.concurrent.CompletableFuture.biApply(CompletableFuture.java:1307)
	at java.base/java.util.concurrent.CompletableFuture$BiApply.tryFire(CompletableFuture.java:1276)
	at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:479)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1016)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1665)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1598)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:177)

There's no issue with vscode-xml 0.11.0

@fbricon

fbricon commented May 25, 2020

Copy link
Copy Markdown
Contributor

Opened #720 about that last bug

@angelozerr

Copy link
Copy Markdown
Contributor Author

I don't know if this is caused by this PR, but given the following xml:

Yes it's a a new bug that you have opened #720

@angelozerr angelozerr self-assigned this May 25, 2020
@angelozerr angelozerr added this to the 0.12.0 milestone May 25, 2020
@angelozerr angelozerr added the DTD label May 25, 2020
@xorye

xorye commented May 25, 2020

Copy link
Copy Markdown

When hovering over an entity that is not defined, I get documentation of the first defined entity:
image

xml:

<?xml version='1.0' encoding='UTF-8'?>
<!DOCTYPE article [
    <!ENTITY foo "foo">
]>

<article>
   &foo
</article>

@angelozerr

Copy link
Copy Markdown
Contributor Author

When hovering over an entity that is not defined, I get documentation of the first defined entity:

Sorry I push my current work for entities hover in this branch. I removed it, now you should not see hover.

@angelozerr angelozerr force-pushed the entities-improve branch 3 times, most recently from d947252 to f36d32a Compare May 26, 2020 12:42
@xorye

xorye commented May 26, 2020

Copy link
Copy Markdown

I found an edge case, which I think is caused by this code:
https://github.com/eclipse/lemminx/blob/f36d32a005400145905a8888d7a81d410b115db5/org.eclipse.lemminx/src/main/java/org/eclipse/lemminx/utils/XMLPositionUtility.java#L551-L553

<?xml version='1.0' encoding='UTF-8'?>
<!DOCTYPE article [
    <!ENTITY hello "foo">
    <!ENTITY helloworld "foo">
]>

<article>
   &helloworld

When trying Go to definition in this case, with the cursor on the o, we go to the definition for hello

gif

I think this issue would be very rare, however

@angelozerr

Copy link
Copy Markdown
Contributor Author

@xorye I change the strategy to discover entity. The referenced entity must be ends with ';'. So in your case definition will not work. You must write ';' at the end of helloword.

@fbricon

fbricon commented May 26, 2020

Copy link
Copy Markdown
Contributor

@xorye I change the strategy to discover entity. The referenced entity must be ends with ';'. So in your case definition will not work. You must write ';' at the end of helloword.

you could also detect if there's a whitespace

@angelozerr

Copy link
Copy Markdown
Contributor Author

you could also detect if there's a whitespace

I think it's a bad idea, because entity reference must start with '&' and ends with ';'

@angelozerr angelozerr merged commit b0dd7ce into eclipse-lemminx:master May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants