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.

Sign up for GitHub

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

Jump to bottom

Improved namespace renaming for XML/XSD #403

Merged
merged 1 commit into from
May 31, 2019
Merged

Improved namespace renaming for XML/XSD #403

merged 1 commit into from
May 31, 2019

Conversation

NikolasKomonen
Copy link
Contributor

@NikolasKomonen NikolasKomonen commented May 29, 2019 •
edited
Loading

Fixes #366

Can rename namespace declaration and all will change, renaming element with namespace does not wipe
namespace, attribute values referencing a namespace will update if necessary

@NikolasKomonen NikolasKomonen changed the title Fix normalizePath test on Windows OS Improved namespace renaming for XML/XSD May 29, 2019
@fbricon fbricon requested a review from angelozerr May 29, 2019 18:28
angelozerr
angelozerr reviewed May 29, 2019
View reviewed changes
...ipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/extensions/contentmodel/model/CMDocument.java Outdated Show resolved Hide resolved
.../eclipse/lsp4xml/extensions/contentmodel/participants/ContentModelCompletionParticipant.java Outdated
String documentation = child.getDocumentation();
if (documentation != null) {
item.setDetail(documentation);
StringBuilder sb = new StringBuilder();
Copy link
Contributor

@angelozerr angelozerr May 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think this code should ne set here but on the cm document implementation. I suggest you that you create an abstractcmdocument class which provides the getfulldocumentation method. This abstract class could store the uri too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@angelozerr

CMDTDDocument is already extending a class, while CMXSDDocument is not.

CMDTDElementDeclaration is also extending a class, while CMXSDElementDeclaration is not.

Is there something I could do to avoid this problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

Your right @NikolasKomonen please forget my comment

Copy link
Contributor

Choose a reason for hiding this comment

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

A another suggestion is to set the documentation code with source in a static method in XMLGenerator. Set the build of documentation with source will help more how this doc is built.

@NikolasKomonen NikolasKomonen force-pushed the renameNamespace branch 2 times, most recently from 094e9a2 to f0d9a81 Compare May 29, 2019 19:15
angelozerr
angelozerr reviewed May 29, 2019
View reviewed changes
org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/XMLRename.java Outdated
import org.eclipse.lsp4xml.dom.parser.TokenType;

/**
* XMLRename
Copy link
Contributor

Choose a reason for hiding this comment

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

Explain more what is the goal of this class like rename namespaces and add @author

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before for the Rename feature, it was using XMLHighlighting to get Highlights and then it turned them into TextEdits. Also Highlighting would only work for 1 Element (Start Tag and End Tag) but with XMLRename it will handle many elements with the same name, namespaces and maybe other renaming in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok thanks for the clarification

angelozerr
angelozerr reviewed May 29, 2019
View reviewed changes
org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/services/XMLRename.java
/**
* XMLRename
*/
public class XMLRename {
Copy link
Contributor

Choose a reason for hiding this comment

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

XMLRename is too generic I think. Perhaps XMLRenameNamespace should be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You think so? We have XMLFormatter, XMLHighlighting, XMLCompletions ...

This class will do all renaming, not only namespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank's for the clarification, I understand more now. Please add the 2 rename features (rename tag element and rename namespaces by using ui, li in the javadoc)

@NikolasKomonen NikolasKomonen force-pushed the renameNamespace branch 2 times, most recently from bc45fa5 to 8fc1179 Compare May 29, 2019 21:10
@NikolasKomonen
Copy link
Contributor Author

@angelozerr Updated, moved the methods to DOMAttr. Implemented doRename() to follow the pattern for XMLLanguageService

angelozerr
angelozerr reviewed May 29, 2019
View reviewed changes
org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/XMLPositionUtility.java
@@ -528,4 +535,39 @@ public static Range getElementDeclMissingContentOrCategory(int offset, DOMDocume
return null;
}

public static boolean doesTagCoverPosition(Range startTagRange, Range endTagRange, Position position) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add javadoc

angelozerr
angelozerr reviewed May 29, 2019
View reviewed changes
org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/XMLPositionUtility.java
|| endTagRange != null && covers(endTagRange, position);
}

public static boolean covers(Range range, Position position) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add javadoc

angelozerr
angelozerr reviewed May 29, 2019
View reviewed changes
org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/XMLPositionUtility.java
return isBeforeOrEqual(range.getStart(), position) && isBeforeOrEqual(position, range.getEnd());
}

public static boolean isBeforeOrEqual(Position pos1, Position pos2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add javadoc

angelozerr
angelozerr reviewed May 29, 2019
View reviewed changes
org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/utils/XMLPositionUtility.java
|| (pos1.getLine() == pos2.getLine() && pos1.getCharacter() <= pos2.getCharacter());
}

public static Range getTagNameRange(TokenType tokenType, int startOffset, DOMDocument xmlDocument) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add javadoc

angelozerr
angelozerr reviewed May 29, 2019
View reviewed changes
org.eclipse.lsp4xml/src/test/java/org/eclipse/lsp4xml/utils/FilesUtilsTest.java
@@ -38,9 +38,9 @@ public void testFilesCachePathPreference() throws Exception {
@Test
public void normalizePathTest() {
assertEquals(Paths.get(System.getProperty("user.home"), "Test", "Folder").toString(), FilesUtils.normalizePath("~/Test/Folder"));
assertEquals(Paths.get(separator, "Test", "~", "Folder").toString(), FilesUtils.normalizePath("/Test/~/Folder"));
assertEquals(Paths.get(separator + "Test", "~", "Folder").toString(), FilesUtils.normalizePath("/Test/~/Folder"));
Copy link
Contributor

Choose a reason for hiding this comment

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

It should belong to another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be, but there was a problem with git history in master. This will fix the problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok no pb

angelozerr
angelozerr reviewed May 29, 2019
View reviewed changes
org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/dom/DOMElement.java
@@ -10,6 +10,9 @@
*/
package org.eclipse.lsp4xml.dom;

import static org.eclipse.lsp4xml.dom.DOMAttr.XMLNS_ATTR;
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep the XMLNS_ATTR constants as private and provide a static method DOMAttr#isXmlns(String attributeName) which can be used in DOMAttr#isXmlNs and in DOMElement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is hard to make it private, DOMElement needs this value: https://github.com/angelozerr/lsp4xml/blob/master/org.eclipse.lsp4xml/src/main/java/org/eclipse/lsp4xml/dom/DOMElement.java#L123

But I did changes and made static isXmlns(String) and other methods too.

@angelozerr
Copy link
Contributor

@NikolasKomonen I played with your PR and it seems it works great I will continue to play with it again).

The rename of namespace prefix is working when you do a rename to the xmlns: attribute (I mean xmlns of xmlns:qq):

<aa:a xmlns:aa="aa.com" xmlns:qq="qq.com">
	<aa:b></aa:b>
	<aa:b></aa:b>
	<aa:c />
	<t type="aa:b" />
	<qq:b></qq:b>
</aa:a>

It should be very cool if we could rename the aa of an element (I mean <aa by an another name)

@angelozerr @NikolasKomonen
Improved namespace renaming for XML/XSD documents
c4459ba 
Fixes eclipse-lemminx#366

Can rename namespace declaration and all will change, renaming element with namespace does not wipe
namespace, attribute values referencing a namespace will update if necessary

Signed-off-by: Nikolas <nikolaskomonen@gmail.com>
@angelozerr angelozerr merged commit d4cf351 into eclipse-lemminx:master May 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angelozerr angelozerr angelozerr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to rename a namespace.
2 participants
@NikolasKomonen @angelozerr