Support xsiSchemaLocationSplit setting with experimental formatter

[JessicaJHee]

Fixes #1246

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

[JessicaJHee]

I believe some of the tests require this setting to get the correct result:

lemminx/org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/extensions/xsi/XSIFormatterExperimentalTest.java

Line 175 in 043e5e1

settings.getFormattingSettings().setPreserveEmptyContent(true);

This will prevent the empty content from getting deleted like here:

lemminx/org.eclipse.lemminx/src/test/java/org/eclipse/lemminx/extensions/xsi/XSIFormatterExperimentalTest.java

Line 165 in 043e5e1

"\r\n" + //

I can either work on the other setting first or change the test so that it's not dependent on the other setting?

[angelozerr]

@JessicaJHee for this kind of formatting you need to do that with an implementation of IformmatingParticipant. I suggest that you see how old formatter uses this participant. You woll need to add or update the signature of the participant to support edits

[angelozerr]

@JessicaJHee if you think it is better to work on another settings before doing this pr dont hesîtate to do that

[JessicaJHee]

@JessicaJHee for this kind of formatting you need to do that with an implementation of IformmatingParticipant. I suggest that you see how old formatter uses this participant. You woll need to add or update the signature of the participant to support edits

I didn't use this because I thought for the new formatter, the idea is to avoid using XMLBuilder by adding/removing spaces instead of rewriting the xml content? But if my understanding is wrong, I will try again and use XSIFormatterParticipant/IFormatterParticipant.

[rgrunber]

I noticed that formatting as onElement and then back to onPair doesn't work.

[rgrunber]

Whoops, just saw Angelo's other comments. My guess is you need to go into DOMAttributeFormatter#formatAttribute(..) and instead of plugging your own code, set up the IFormatterParticipants being contributed and get the proposed text edits of each one. You would be calling some new (default) method of IFormatterParticipant that is returning a text edit (and has no XMLBuilder).

With that in place the task just becomes implementing the new method in XSIFormatterParticipant based on the old logic.

@angelozerr feel free to correct me if any of this is wrong.

[angelozerr]

Indeed @rgrunber it was my idea. the formatter participant could have à New method with edits as parameter. Once experimental formatter will replace the old formatter we will able to remove the method with xmlbuilder.

[JessicaJHee]

Still fixing up a few things before this commit is ready for review!

[datho7561]

Great work, Jessica!

[datho7561]

Wait actually, some of the tests fail on my computer. Can you take a look again and see if anything odd is happening for you?

[datho7561]

Wait actually, some of the tests fail on my computer. Can you take a look again and see if anything odd is happening for you?

Well, it works well for me now. No idea what changed

[angelozerr]

I think given that the behaviour of the existing formatter is to line up the URLs even when splitAttributes is disabled, it would be a good idea to ensure that this PR is able to do that.

It seems that the PR requires that splitAttributes is set to true https://github.com/eclipse/lemminx/pull/1252/files#diff-c0186ecdc200d591a9caa3dadf037073d77443cee30423e92600647efbf3fddbR337

You should remove this seetings and it should work without splitAttributes. With the current PR if you remove the line code https://github.com/eclipse/lemminx/pull/1252/files#diff-c0186ecdc200d591a9caa3dadf037073d77443cee30423e92600647efbf3fddbR337 tests fail.

[JessicaJHee]

It seems that the PR requires that splitAttributes is set to true https://github.com/eclipse/lemminx/pull/1252/files#diff-c0186ecdc200d591a9caa3dadf037073d77443cee30423e92600647efbf3fddbR337

I already have these three tests for the case where split attribute is set to false:

xsiSchemaLocationSplitOnElementWithoutSplitAttribute()
xsiSchemaLocationSplitOnPairWithoutSplitAttribute()
xsiSchemaLocationSplitOnElementWithTabsWithoutSplitAttribute()

Is the idea to force split attribute to true when schema location split is set to either onPair or onElement?

[angelozerr]

I already have these three tests for the case where split attribute is set to false:

I think you should not have specific test with split attributes to true. The xsi settings (which format the attribute value of xsi:schemaLocation) should be totally independant from splitAttribute (which format attribute name location).

https://github.com/eclipse/lemminx/pull/1252/files#diff-c0186ecdc200d591a9caa3dadf037073d77443cee30423e92600647efbf3fddbR337

Is the idea to force split attribute to true when schema location split is set to either onPair or onElement?

Even if looks like the same behavior, we should distinguish them. SplitAttribute format the attribute name location, although xsi settings is for attribute value of xsi:schemaLocation.

[datho7561]

I already have these three tests for the case where split attribute is set to false:

I think you should not have specific test with split attributes to true.

Why not? If I understand correctly, the behaviour should be different, since you need to indent the schemaLocation entries a different amount. eg.

<?xml version="1.0" encoding="UTF-8"?>
<aaa xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="https://google.ca my-file.xsd
                                                                               https://stackoverflow.yaml my-other-schema.xsd
                                                                               https://github.com my-github-schema.xsd">
    <bbb> Hello</bbb>
    <ccc />
</aaa>

If you were to indent to the same level as you would do with splitAttributes: "true", then the entries don't line up.

[angelozerr]

Good catch @datho7561. Lets keep tests with Split attributes. But the main tests should be done with no Split attributes.

[angelozerr]

After playing with the PR, I noticed that it is important to support too https://github.com/redhat-developer/vscode-xml/blob/main/docs/Formatting.md#xmlformatpreserveattributelinebreaks

If xml.format.splitAttribute is set to false, and xml.format.preserveAttributeLineBreaks is set to true, it doesn't format correctly the XML.

I think we should have a test with xsiSchemaLocationSplitOnElementWithTabsWithPreserverAttributeLineBreaks which is a copy/paste of xsiSchemaLocationSplitOnElementWithTabsWithSplitAttribute by relacing https://github.com/eclipse/lemminx/pull/1252/files#diff-c0186ecdc200d591a9caa3dadf037073d77443cee30423e92600647efbf3fddbR298 with

settings.getFormattingSettings().setPreserveAttributeLineBreaks(true);

[datho7561]

Works really well. Thanks, Jessica!