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

Extension Point to customize preferences for any editor #9

Open
angelozerr opened this issue Jul 17, 2015 · 13 comments
Open

Extension Point to customize preferences for any editor #9

angelozerr opened this issue Jul 17, 2015 · 13 comments

Comments

@angelozerr
Copy link
Contributor

No description provided.

@ncjones
Copy link
Owner

ncjones commented Jul 18, 2015

In the long-term I think exposing an extension point is the right way to go.

Currently configuring the indentation behavior requires knowing about the config settings of various editors:

setPreference("org.eclipse.ui.editors", "spacesForTabs", "true");
setPreference("org.eclipse.jdt.core", "org.eclipse.jdt.core.formatter.tabulation.char", "space");
setPreference("org.eclipse.wst.xml.core", "indentationChar", "space");
setPreference("org.eclipse.ant.ui", "formatter_tab_char", "false");

This approach has the following limitations:

  1. It is not scalable - it will result in an ever-growing list of configs for supported editors.
  2. It is not guaranteed to always work as some editors may not even have a config setting for indentation or may change config names between versions.
  3. It is a hack - it relies on changing global settings instead of the settings for an individual buffer. This is obvious and jarring when there are multiple editors visible with different editorconfig settings visible and focus shifts between them.

Moving the responsibility to the plugin that owns the editor will be more robust and easier to maintain.

What the extension point should look like is not clear to me though because not all editorconfig properties are supported yet. In particular, I am not sure whether properties will always require custom behavior for different editors. For example, the way I have been planning on implementing "trailing whitespace" and "final newline" is to use the same approach as Anyedit - detect file save events and rewrite the file content.

When more of the properties have been implemented we will have a better idea of what the relationship between this plugin and the various editor plugins should be.

@paulvi
Copy link

paulvi commented Sep 1, 2015

Well, there are 2 issues here:

  1. Eclipse should have a way to specify basic settings for all Editor
  2. editorconfig-eclipse should be able to use that way too (together with older setPreference(")

Part 1 is unlilely to come from anywhere except from editorconfig-eclipse itself

So when editorconfig-eclipse could be separated into 2 parts?

@ncjones
Copy link
Owner

ncjones commented Sep 1, 2015

@paulvi I agree. Do you have any ideas how to achieve "part 1"? AFAIK this is not possible in a plugin and needs to come from Eclipse core.

@angelozerr
Copy link
Contributor Author

The big problem with the existing Eclipse Editor is that preferences (like format) is for the whole editor of the project. With editorconfig you can have several .editorconfig files in the same project. So editor of the same project can have different preferences. I'm afraid that this feature is not possible -(

@ncjones
Copy link
Owner

ncjones commented Sep 2, 2015

@angelozerr to be clear you mean specifying settings for all editors is not possible right?

There is nothing stopping us declaring an extension point which editors can optionally use to access the .editorconfig settings.

@angelozerr
Copy link
Contributor Author

@angelozerr to be clear you mean specifying settings for all editors is not possible right?

I mean that Eclipse Editor like XML, JSDT, etc Editor uses preferences from

  • global preferences
  • project preferences.

With editorconfig you can have file preferences according this specification :

Where are these files stored?
When opening a file, EditorConfig plugins look for a file named .editorconfig in the directory 
of the opened file and in every parent directory.
A search for .editorconfig files will stop if the root filepath is reached or an EditorConfig 
file with root=true is found

I'm afraid that we cannot support that with Eclipse.

@ncjones
Copy link
Owner

ncjones commented Sep 2, 2015

Forgive my ignorance as I have never implemented an Eclipse editor, but surely an editor can source preferences from anywhere, not just global and project settings. If we create an extension point that exposes the editorconfig preferences then editors can opt-in to use our preferences instead of the gloabal ones.

The way I see this panning out is that authors of editors who wish to add support for editorconfig can simply add it to their own plugin instead waiting on us to add support for yet another editor. Doing it that way would also offer a smoother experience as the global settings would not need be abused (which causes the jarring experience when switching between editors that are both visible and have different editorconfig tab width settings).

We will need to continue to rely on global preference hacks for any editors that do not consume our extension point.

@paulvi
Copy link

paulvi commented Sep 2, 2015

Aren't all text-based editor to inherit TextEditor Preferences?

image

@angelozerr
Copy link
Contributor Author

@ncjones I have studied how to support with clean mean .editorconfig inside https://github.com/angelozerr/typescript.java an Eclipse plugin for TypeScript.

The challenge is:

  • use your generic plugin
  • apply the preferences for a given editor instance (and not for the whole preferences of the workspace)
  • don't have dependencies to your plugin inside my TypeScript editor.

I have found a solution and if you like my idea, I will create a PR.

The basic idea is to use an instance of IEclipsePreferences coming from the editor and not to use InstanceScope.INSTANCE.getNode(prefsNodeName)

The editorconfig plugin try to retrieve the eclipse preferences from the editor with getAdapter method like this:

@Override
public void selectionChanged(final IWorkbenchPart part, final ISelection selection) {
    if (part != currentEditorPart && part instanceof IEditorPart) {
        currentEditorPart = (IEditorPart) part;
        final IFile file = (IFile) currentEditorPart.getEditorInput().getAdapter(IFile.class);
                       // try to get the eclipse preferences from th eeditor
        IEclipsePreferences preferences = (IEclipsePreferences) currentEditorPart.getAdapter(IEclipsePreferences.class);
        editorActivationListener.editorActivated(file, preferences);
    }
}

And my TypeScript Editor implements getAdapter like this:

editorPreferences = new EclipsePreferences();
editorPreferences.addPreferenceChangeListener(
        new org.eclipse.core.runtime.preferences.IEclipsePreferences.IPreferenceChangeListener() {

            @Override
            public void preferenceChange(PreferenceChangeEvent event) {
                System.err.println(event);
            }
        });
...
@Override
public Object getAdapter(@SuppressWarnings("rawtypes") Class key) {
      if (key.equals(IEclipsePreferences.class)) {
      // support for .editorconfig
      return editorPreferences;
    }
        return super.getAdapter(key);
}

After that when editorconfig property must be applyed, it applies property of the instance of IEclipsePreference of global preferences if the editor doesn't return the eclipse preferences

@Override
    public void visitIndentStyle(final ConfigProperty<IndentStyleOption> property) {
        final Boolean spacesForTabs = property.getValue().equals(IndentStyleOption.SPACE);
        setPreference("org.eclipse.ui.editors", "spacesForTabs", spacesForTabs.toString());
        if (preferences != null) {
            preferences.put(property.getDisplayValue(), Boolean.toString(spacesForTabs));
        } else {
            setPreference("org.eclipse.jdt.core", "org.eclipse.jdt.core.formatter.tabulation.char",
                    spacesForTabs ? "space" : "tab");
            setPreference("org.eclipse.wst.xml.core", "indentationChar", spacesForTabs ? "space" : "tab");
            setPreference("org.eclipse.ant.ui", "formatter_tab_char", Boolean.toString(!spacesForTabs));
        }
    }

What do you think about this idea?

If you like it, I will create a PR with this idea. The only thing is that now the apply of the editorconfig is done by EditorFileConfig and not by EditorConfigEditorActivationHandler. So now you have eclipse dependencies IEclipsePreference inside EditorFileConfig (core package, but is it a problem?)

@angelozerr
Copy link
Contributor Author

@mickaelistria what do you think about my idea with

IEclipsePreferences preferences = (IEclipsePreferences) currentEditorPart.getAdapter(IEclipsePreferences.class);

I have suggested this idea too at https://bugs.eclipse.org/bugs/show_bug.cgi?id=463071#c9

@mickaelistria
Copy link

This conversation is deriving strongly from its initial purpose. I opened #30 for easier tracking of styling individual editors.

@ncjones
Copy link
Owner

ncjones commented May 25, 2016

@angelozerr really good work! Setting config per editor would be a big improvement. Will leave this issue as is because I still believe editors should be able to depend on this plugin to source configuration.

@angelozerr
Copy link
Contributor Author

I have created an new discussion about extension point at #37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants