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

improve getCurrentAttribute method for AbstractPositionReqest #584

Merged

Conversation

Seiphon
Copy link
Contributor

@Seiphon Seiphon commented Oct 24, 2019

No description provided.

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

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.

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 , 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.

Copy link
Contributor

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).

Copy link
Contributor Author

@Seiphon Seiphon Oct 29, 2019

Choose a reason for hiding this comment

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

Copy link
Contributor

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

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

@angelozerr angelozerr Oct 25, 2019

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;
			}
		}

@Seiphon Seiphon force-pushed the improve_getCurrentAttribute branch from 70ca73b to e03dc15 Compare October 28, 2019 05:03
@Seiphon Seiphon force-pushed the improve_getCurrentAttribute branch from e03dc15 to 1149554 Compare October 29, 2019 15:31
@angelozerr angelozerr merged commit 6e8d4f1 into eclipse-lemminx:master Oct 29, 2019
@angelozerr
Copy link
Contributor

Thanks @Seiphon !

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