Support for setting ANSI console enablement default from another Eclipse Plugin

[ddscharfe]

Hi,

first of all, thank you for providing the ANSI econsole plugin, it works really well :)

I'd like to integrate the ANSI econsole plugin into an RCP, but it should be disabled by default. I can set the default like this:

IPreferenceStore store = AnsiConsoleActivator.getDefault().getPreferenceStore();
store.setDefault(AnsiConsolePreferenceConstants.PREF_ANSI_CONSOLE_ENABLED, false);

However the current implementation of EnableDisableHandler uses the inital state defined in the plugin.xml:

<state
    class="org.eclipse.ui.handlers.RegistryToggleState:true"
    id="org.eclipse.ui.commands.toggleState">
</state>

Hence, the toggle button in the tool bar is still enabled. I solved this by explicitly setting the value of the toggle state to the value of the preference in the preference store:

ICommandService service = PlatformUI.getWorkbench().getService(ICommandService.class);
Command command = service.getCommand(EnableDisableHandler.COMMAND_ID);
State state = command.getState(RegistryToggleState.STATE_ID);
state.setValue(store.getBoolean(AnsiConsolePreferenceConstants.PREF_ANSI_CONSOLE_ENABLED));

However this is a little cumbersome. The current implementation explicitly executes the enable_disable command using a dummy event when calling performOK() in the preference page. Therefore the EnableDisableHandler has to implement IElementUpdater and distinguish between normal and dummy events. If I am not missing something, this can be removed. To synchronize the toggle state with the preference, a PropertyChangeListener can be used.

I will provide a pull request for this issue soon.

[mihnita]

Sorry I did not react a bit sooner, but I didn't have much time. Now I'm looking at the issue, and the code, and I am not sure about the "it should be disabled by default" part.

The whole thing with plugin.xml is so that the state is properly preserved between opening / closing Eclipse. This was a fix for issue #45, #47, #57. I had to move the whole "custom handling" I was doing before (using org.eclipse.jface.action) to use org.eclipse.core.commands

I agree about hack in performOK(), I don't like it either, and I would replace / remove it in an instant, if I could find something better.

But preserving the enable / disable state is intentional, I don't want "disabled by default" If your PR changes that, then sorry, no...

[mihnita]

What about something like this:

import org.eclipse.core.commands.Command;
import org.eclipse.core.commands.ExecutionException;
import org.eclipse.core.commands.State;
import org.eclipse.ui.PlatformUI;
import org.eclipse.ui.commands.ICommandService;
import org.eclipse.ui.handlers.HandlerUtil;
import org.eclipse.ui.handlers.RegistryToggleState;

public class ChangeAnsiConsoleState {
    private static final String ANSICON_ENABLE_DISABLE_COMMAND_ID = "AnsiConsole.command.enable_disable";

    public static enum AnsiConsoleCommand {
        Nothing,
        Enable,
        Disable,
        Toggle
    }

    public static void toggleDisableAnsicon(AnsiConsoleCommand accResult) {
        if (accResult.equals(AnsiConsoleCommand.Nothing))
            return;

        ICommandService service = PlatformUI.getWorkbench().getService(ICommandService.class);
        Command command = service.getCommand(ANSICON_ENABLE_DISABLE_COMMAND_ID);
        State state = command.getState(RegistryToggleState.STATE_ID);
        Boolean boolState = (Boolean) state.getValue();

        switch (accResult) {
            case Disable:
                if (boolState == false) return; // already false
                break; // Continue to toggle
            case Enable:
                if (boolState == true) return; // already true
                break; // Continue to toggle
            case Toggle:
                break; // Continue to toggle
            default:
                // Nothing already handled (returned)
                return;
        }
        try {
            HandlerUtil.toggleCommandState(command);
        } catch (ExecutionException e) {
            e.printStackTrace();
        }
    }
}

It uses the standard Eclipse command mechanism, so it should not break anything.

Although I am still uneasy about the idea that a plugin messes with the state of a different plugin without human intervention. Because users don't have a way to know, they will come right back here to complain :-)

[ddscharfe]

Sorry I did not react a bit sooner, but I didn't have much time.

Actually you responded quiet fast, so thanks for that :)

Now I'm looking at the issue, and the code, and I am not sure about the "it should be disabled by default" part.

Maybe I've expressed myself incorrectly. I only want to "disable by default" inside my specific Eclipse RCP application which contains the ansi econsole plugin. For the plugin itself, being enabled by default makes totally sense. I also still want the state to be preserved between opening/closing Eclipse, so if the user enables the console it should remain enabled after restart.

The whole thing with plugin.xml is so that the state is properly preserved between opening / closing Eclipse. This was a fix for issue #45, #47, #57. I had to move the whole "custom handling" I was doing before (using org.eclipse.jface.action) to use org.eclipse.core.commands

I understand and I also think that using the default command mechanism is the way to go.

I agree about hack in performOK(), I don't like it either, and I would replace / remove it in an instant, if I could find something better.

The code I committed replaces the perfomOK() workaround and is supposed to still keep the state preserved properly. The only thing it should change is that it is now possible to change the default enabled state easily via the IPreferenceStore API. If my implementation works (e.g. not breaking #45, #47, #57, etc.) I would consider this the "clean" solution.

But preserving the enable / disable state is intentional, I don't want "disabled by default"

I agree.

What about something like this: ... It uses the standard Eclipse command mechanism, so it should not break anything.

This should work if I call this method once for every new workspace, in order to set the default. However I basically want to set the default, so I would prefer the solution I committed (only if it works, of course).

Although I am still uneasy about the idea that a plugin messes with the state of a different plugin without human intervention.

I agree, but I think it is okay to change the default setting of another plugin.

Because users don't have a way to know, they will come right back here to complain :-)

And as we all know, users always complain ;)

Anyways, thanks for your feedback. If you decide to keep the current implementation, it is also fine for me, because as I mentioned in my first comment I found an okay solution which works for me:

ICommandService service = PlatformUI.getWorkbench().getService(ICommandService.class); Command command = service.getCommand(EnableDisableHandler.COMMAND_ID); State state = command.getState(RegistryToggleState.STATE_ID); state.setValue(store.getBoolean(AnsiConsolePreferenceConstants.PREF_ANSI_CONSOLE_ENABLED));

BR, Dominic