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

Added folding setting to keep displaying the closing tag after folding #1209

Merged
merged 3 commits into from May 31, 2022

Conversation

JessicaJHee
Copy link
Contributor

Requires redhat-developer/vscode-xml/pull/714

Signed-off-by: Jessica He jhe@redhat.com

Copy link
Contributor

@AlexXuChen AlexXuChen left a comment

Choose a reason for hiding this comment

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

Features working as intended:
Peek 2022-05-27 16-57
A couple of changes, and add/change tests in https://github.com/eclipse/lemminx/blob/master/org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/services/XMLFoldingsTest.java with the setting enabled/disabled.

private boolean includeClosingTagInFold;

public XMLFoldingSettings() {
this.includeClosingTagInFold = DEFAULT_INCLUDE_CLOSING_TAG_IN_FOLD;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you have 2 constructors, you can use the overloaded one in the other, i.e. call XMLFoldingSettings(DEFAULT_INCLUDE_CLOSING_TAG_IN_FOLD); instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed it is a good practive to have just one constructor which initialize all fields.

return includeClosingTagInFold;
}

public void merge(XMLFoldingSettings newSettings) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add JavaDoc here similar to other merge definitions:

/**
* Merge only the given completion settings (and not the capability) in the
* settings.
*
* @param newCompletion the new settings to merge.
*/

@angelozerr
Copy link
Contributor

angelozerr commented May 28, 2022

It shoud be nice to have test with this new settings.

I suggest that you: define a new static method in

private static void assertRanges(String[] lines, ExpectedIndentRange[] expected, String message, Integer nRanges) {

private static void assertRanges(String[] lines, ExpectedIndentRange[] expected) {
	assertRanges(lines, expected, "", null, new XMLFoldingSettings());
}

private static void assertRanges(String[] lines, ExpectedIndentRange[] expected, XMLFoldingSettings settings) {
	assertRanges(lines, expected, "", null, settings);
}

private static void assertRanges(String[] lines, ExpectedIndentRange[] expected, String message, Integer nRanges) {
    assertRanges(lines, expected, message, nRanges, new XMLFoldingSettings());
}

private static void assertRanges(String[] lines, ExpectedIndentRange[] expected, String message, Integer nRanges, XMLFoldingSettings settings)
TextDocument document = new TextDocument(String.join("\n", lines), "test://foo/bar.json");
		DOMDocument xmlDocument = DOMParser.getInstance().parse(document, null);
		XMLLanguageService languageService = new XMLLanguageService();

		FoldingRangeCapabilities context = new FoldingRangeCapabilities();
		context.setRangeLimit(nRanges);
		settings.setCapabilities(context);
		List<FoldingRange> actual = languageService.getFoldingRanges(xmlDocument, settings);

		List<ExpectedIndentRange> actualRanges = new ArrayList<>();
		for (FoldingRange f : actual) {
			actualRanges.add(r(f.getStartLine(), f.getEndLine(), f.getKind()));
		}
		Collections.sort(actualRanges, (r1, r2) -> r1.startLine - r2.startLine);
		assertArrayEquals(expected, actualRanges.toArray(), message);
}

after that you can duplicate test and set your new settings includeClosingTagInFold:

@Test
	public void testFoldOneLevelWithincludeClosingTagInFoldAsFalse () {
		String[] input = new String[] {
			/*0*/"<html>",
			/*1*/"Hello",
			/*2*/"</html>"
		};
                XMLFoldingSettings settings = new XMLFoldingSettings ();
                settings.setIncludeClosingTagInFold(false)
		assertRanges(input,  new ExpectedIndentRange[] {r(0, 2)}, );
	}

Please not the test will fail, please adjust the proper ranges.

And you can do the same thing for existing tests.

Signed-off-by: Jessica He <jhe@redhat.com>
@@ -49,6 +49,10 @@ public XMLFoldings(XMLExtensionsRegistry extensionsRegistry) {
this.extensionsRegistry = extensionsRegistry;
}

public boolean isIncludeClosingTagInFold(XMLFoldingSettings context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As it is not an important method, I will move it after getFoldingRanges which is main entry method. More this method is used only in this class, so you can set to private. At the end, this method don't need some fields of the XMLFoldings, so I will add static, in other words :

private static isIncludeClosingTagInFold(XMLFoldingSettings foldingSettings)

@@ -32,4 +32,35 @@ public FoldingRangeCapabilities getCapabilities() {
public Integer getRangeLimit() {
return capabilities != null ? capabilities.getRangeLimit() : null;
}

public static final boolean DEFAULT_INCLUDE_CLOSING_TAG_IN_FOLD = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

this static filed is used only in this class, set it to private.

}

@Test
public void testFoldTwoLevelWithincludeClosingTagInFoldAsTrue () {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove tabs with space (public void test...). Do the samething for other test methods

Signed-off-by: Jessica He <jhe@redhat.com>
@angelozerr angelozerr merged commit 3ed3054 into eclipse:master May 31, 2022
@angelozerr
Copy link
Contributor

Great job @JessicaJHee !

@angelozerr angelozerr added this to the 0.21.0 milestone Jun 1, 2022
@angelozerr angelozerr added the enhancement New feature or request label Jun 29, 2022
@angelozerr angelozerr changed the title added folding setting to keep displaying the closing tag after folding Added folding setting to keep displaying the closing tag after folding Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants