-
Notifications
You must be signed in to change notification settings - Fork 93
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
improve getCurrentAttribute method for AbstractPositionReqest #584
improve getCurrentAttribute method for AbstractPositionReqest #584
Conversation
d292aab
to
70ca73b
Compare
@@ -101,6 +102,9 @@ public String getCurrentTag() { | |||
|
|||
@Override | |||
public String getCurrentAttributeName() { | |||
if (node != null && node.isAttribute() && ((DOMAttr) node).getName() != null) { | |||
return ((DOMAttr) node).getName(); | |||
} | |||
return currentAttributeName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the setCurrentAttributeName method and currentAttributeName field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @angelozerr , I am afraid we can not just remove them. If we remove them, it will break something here .
In this case, the DOMNode can not be recognized as a attribute, so it will always return a null which is not excepted. However if by the original scan way, we can get the attribute name. So maybe we should keep them, if there is no a better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I executed our tests by removing currentAttributeName and everything is working? Have you a usecase which causes the trouble (have you a test for that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After removed the setCurrentAttributeName method and currentAttributeName field,
https://github.com/angelozerr/lsp4xml/blob/4eaea24d41b23dd09d76737483b3557f7bbb71e4/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/XMLCompletions.java#L90
and
https://github.com/angelozerr/lsp4xml/blob/4eaea24d41b23dd09d76737483b3557f7bbb71e4/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/XMLCompletions.java#L123
should also be removed.
When I executed tests, completionOnAttributeName() of XMLSchemaCompletionExtensionsTest, and testHTMLAttributeValueCompletion() of HTMLCompletionExtensionsTest will fail.
https://github.com/angelozerr/lsp4xml/blob/4eaea24d41b23dd09d76737483b3557f7bbb71e4/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/extensions/contentmodel/participants/ContentModelCompletionParticipant.java#L313
attributeName will always get a null here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see the problem. When completion on attribute value is done, the scanner starts with attribute value token and doesn't get the currentAttributeName. We should remove this currentAttributeName and work only with DOM node because I find it's not very clean to use currentAttributeName when completion is done on attribute name and use node in case . I suggest you to implement getCurrentAttributeName
like this:
@Override
public String getCurrentAttributeName() {
DOMAttr attr = getCurrentAttribute();
return attr != null ? attr.getName() : null;
}
/**
* Returns the current attribute at the given offset and null otherwise.
*
* @return the current attribute at the given offset and null otherwise.
*/
private DOMAttr getCurrentAttribute() {
if (node == null) {
return null;
}
switch (node.getNodeType()) {
case Node.ELEMENT_NODE:
return node.findAttrAt(offset);
case Node.ATTRIBUTE_NODE:
return ((DOMAttr) node);
default:
return null;
}
}
@@ -578,6 +579,10 @@ public static void assertHover(XMLLanguageService xmlLanguageService, String val | |||
} | |||
} | |||
|
|||
public static void assertHoverAttributeName(IHoverRequest request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this method test
@@ -91,6 +91,7 @@ public String onTag(IHoverRequest request) throws Exception { | |||
|
|||
@Override | |||
public String onAttributeName(IHoverRequest request) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace with
@Override
public String onTag(IHoverRequest request) throws Exception {
if ("bean".equals(request.getCurrentTag())) {
return TEST_FOR_TAG_HOVER;
}
return null;
}
@Override
public String onAttributeName(IHoverRequest request) throws Exception {
if ("class".equals(request.getCurrentAttributeName())) {
return TEST_FOR_ATTRIBUTENAME_HOVER;
}
return null;
}
}
70ca73b
to
e03dc15
Compare
e03dc15
to
1149554
Compare
Thanks @Seiphon ! |
No description provided.