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

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

Closed
JacksonBailey opened this issue Feb 3, 2017 · 14 comments · May be fixed by #47
Closed

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

JacksonBailey opened this issue Feb 3, 2017 · 14 comments · May be fixed by #47

Comments

@JacksonBailey
Copy link
Contributor

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
Copy link
Owner

ncjones commented Feb 3, 2017

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) {
        // ...
    }
}
  1. 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
Copy link
Owner

ncjones commented Feb 3, 2017

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

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.

ncjones pushed a commit that referenced this issue Apr 26, 2017
@ncjones
Add a method to get a property by name
45787ee 
- See: #41
ncjones pushed a commit that referenced this issue Apr 26, 2017
@ncjones
Give EditorConfigVisitor entire EditorFileConfig
596e78c 
This is a workaround for some of the editors needing more than a single
property during visiting.

- See: #41
ncjones pushed a commit that referenced this issue Apr 26, 2017
@ncjones
Add workaround for XML editor when using tabs
5d4a5f7 
The XML editor has two properties, `indentationChar` and
`indentationSize`. `indentationChar` is which character to use for
indentation (as expected) but `indentationSize` is how many of the
*`indentationChar` character* to use, now *how many columns one level of
indentation should be.*

As such, If your `.editorconfig` file has `indent_style = tab` and
`indent_size = 2` (something like below for example) then the XML editor
will use *2 tabs* for one level of indentation.

    [*]
    indent_size = 2

    [*.xml]
    indent_style = tab

Each of those 2 tabs will be 2 columns wide so the individual tabs are
being *displayed* correctly, just not *placed* correctly.

This change makes it so that the XML editor's `indentationSize` set to
the `.editorconfig` file's `indent_size`'s value *only* when the
`indent_style` is `space`. When it is `tab` the XML editor's
`indentationSize` is cleared.

- See: #41
@victornoel
Copy link

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

@JacksonBailey
Copy link
Contributor Author

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

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

@ncjones
Copy link
Owner

ncjones commented Jun 4, 2018

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

@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 :)

This was referenced Jun 9, 2018
Open
Open
@victornoel
Copy link

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

@SebastianDietrich
Copy link

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)

@victornoel
Copy link

I guess maybe some things changed in Eclipse and there is no more problems then :) I haven't really looked into it since then though

@JacksonBailey
Copy link
Contributor Author

@SebastianDietrich / @victornoel I no longer use Eclipse, if you're both saying this is no longer an issue let me know and I will close this issue. I just wanted to make sure.

@SebastianDietrich
Copy link

I can confirm that it works with eclipse 2021-12 (latest) on *.xml files

@JacksonBailey
Copy link
Contributor Author

Closing. 🎉 We did it folks! 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix xml editor victornoel/editorconfig-eclipse
4 participants
@victornoel @SebastianDietrich @ncjones @JacksonBailey