XML Editor behaves incorrectly when using tabs and indent_size other than 1

[JacksonBailey]

Problem

If indent_style = tab and indent_size = 2 (or anything other than 1) the XML editor behaves incorrectly. The tabs will (correctly) be the width specified by indent_size but indentation will be done with a number of tabs equal to the indent_size instead of just a single tab. (The reason 1 works is because it will use only one tab which will be only one unit wide. It looks ugly but technically behaves correctly.)

Steps to reproduce

1. Have this plugin installed (of course)
2. Use indent_style = tab and any indent_size bigger than 1
3. Format any XML file

Notes

I believe the issue is on this line. The problem is due to an oddity of the XML editor. The "indentation size" option is not how many units the indentation is, it is how many of the indentation character (see here) to use. (Why they have it set up like this is beyond me. It seems silly. I can't imagine anyone ever wanting to use multiple tabs to represent a single level of indentation.)

The simplest solution would be something like below.

@Override
public void visitIndentStyle(final ConfigProperty<IndentStyleOption> property) {
	// ...
	setPreference("org.eclipse.wst.xml.core", "indentationChar", spacesForTabs ? "space" : "tab");
	// add this conditional to set size to 1 is using tabs
	if (!spacesForTabs) {
		setPreference("org.eclipse.wst.xml.core", "indentationSize", "1");
	}
	// ...
}

@Override
public void visitIndentSize(final ConfigProperty<Integer> property) {
	// ...
	// add this conditional to only set indent size when using spaces
	if (<using spaces>) { // Not sure how to get that property here though...
		setPreference("org.eclipse.wst.xml.core", "indentationSize", indentSizeString);
	}
	// ...
}

The only issue is not having access to the ConfigProperty for IndentStyleOption inside visitIndentSize. An alternative would be to make sure visitIndentStyle is called after visitIndentSize so it can override if it is needed, but I don't know how easy that would be with this code base, I haven't looked too far into it.

[ncjones]

Thanks for the feedback. I think your analysis and proposal are sound. The following two enhancements will be required:

1. Add a method to EditorFileConfig to get a property by name:

public class EditorFileConfig {
    // ...
    public ConfigProperty getConfigProperty(String name) {
        // ...
    }
}

2. Make the visitor implementation stateful so it holds the EditorFileConfig instance and can use the above method to apply your suggested logic. For example line 44 of EditorConfigEditorActivationHandler would change to:

configProperty.accept(new EditocConfigVisitor(fileEditorConfig));

With these changes, your logic above can be implemented in the new visitor as:

public void visitIndentSize(final ConfigProperty<Integer> property) {
    // ...
    if (IndentStyleOption.SPACES.equals(this.fileEditorConfig.getConfigProperty("INDENT_STYLE").getValue())) { 
        setPreference("org.eclipse.wst.xml.core", "indentationSize", indentSizeString);
    }
    // ...
}

You would also need to do null checking and write some automated tests.

[ncjones]

In the case where the indent style is tabs then you will probably want to actually clear the value of the XML editor indent size.

[JacksonBailey]

I'm going to start taking a stab at this but I may have some questions since I've never done anything with Eclipse plugins. I think most of it should be pretty straight forward though.

[victornoel]

@ncjones is this going to be merged one day? :)

[JacksonBailey]

@victornoel you are welcome to pick up the code from where I left off (or start fresh). I have stopped working on this a long time ago since I stopped using this plugin.

[victornoel]

Thanks @JacksonBailey, I'm more worried about @ncjones answer to the question though ;)

[ncjones]

@victornoel this change was being driven by @JacksonBailey. I believe the changes are in PR #42 which I commented on but the requested changes were never made. If you are really keen then you can pick up that branch and get it working. Sorry I don't have time or incentive to work on this myself.

[victornoel]

@ncjones no problem if you don't have the time, I mostly wanted to know if you were available to merge and release new version. In that case I will take a look at it this week-end :)

[victornoel]

@ncjones I hope you get the time to review #47, merge it and release a new version of your plugin :)

[SebastianDietrich]

I did no excessive testing, but my .editorconfig with [*.{groovy,java,kt,xml}] and 4 spaces didn't have any effect on pom.xml files in eclipse (but worked correctly with java files)