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

Support advanced characters for entity name. #718

Merged
merged 1 commit into from
May 26, 2020

Conversation

angelozerr
Copy link
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
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
Copy link
Contributor

fbricon commented May 25, 2020

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
Copy link
Contributor

fbricon commented May 25, 2020

Opened #720 about that last bug

@angelozerr
Copy link
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
Copy link

xorye commented May 25, 2020

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
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
Copy link

xorye commented May 26, 2020

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
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
Copy link
Contributor

fbricon commented May 26, 2020

@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
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