Add methods to unregister menu, command and keybinding contributions

[thegecko]

Signed-off-by: Rob Moran [email protected]

This PR introduces functionality to allow a plugin developer to unregister existing menus, commands or key bindings.

An example of how this works:

import { injectable } from 'inversify';
import { CommandRegistry } from '@theia/core';
import { WorkspaceCommands } from '@theia/workspace/lib/browser';
import { MenuContribution, MenuModelRegistry, CommandContribution } from '@theia/core/lib/common';
import { KeybindingContribution, KeybindingRegistry, CommonCommands } from '@theia/core/lib/browser';

@injectable()
export class RemovalContribution implements CommandContribution, MenuContribution, KeybindingContribution {

    public registerCommands(commandRegistry: CommandRegistry): void {
        commandRegistry.unregisterCommand(CommonCommands.ABOUT_COMMAND);
    }

    public registerMenus(menus: MenuModelRegistry): void {
        menus.unregisterMenuAction(CommonCommands.ABOUT_COMMAND);
        menus.unregisterMenuAction(WorkspaceCommands.OPEN_RECENT_WORKSPACE);
        menus.unregisterMenuAction(WorkspaceCommands.OPEN_WORKSPACE);
        menus.unregisterMenuAction(WorkspaceCommands.CLOSE);
    }

    public registerKeybindings(keybindings: KeybindingRegistry): void {
        keybindings.unregisterKeybinding('f1');
    }
}

@svenefftinge

[akosyakov]

could be shorter: const key = typeof keyOrBinding === 'string' ? keyOrBinding : keyOrBinding.keybinding;

[thegecko]

Would you prefer this shortened style or an is added to the namespace as mentioned below?

[thegecko]

nm, I've used is :)

[akosyakov]

We usually have a namespace next to an interface to introduce auxiliary functions like is, i.e.

export interface Keybinding {
...
}
export namespace Keybinding {
    export function is(arg: Keybinding | any): arg is Keybinding {
        return !!arg && 'keybinding' in arg;
    }
}

just fyi: a runtime type check is better to do with in operator, there are 2 issues with (<Keybinding>binding).keybinding:

• it executes a getter and can cause side effects
• returned undefined does not mean that a property does not exist

[akosyakov]

@svenefftinge I find it useful. So far the way to go was to customize and rebind a corresponding contribution, but it is not always possible, e.g. if it is not bound explicitly, and clean, e.g. one needs to copy the code from original contribution.

The only concern is the order of loading extensions. We don't guarantee anything, it depends on how a developer declares extensions in package.json. We should make it more deterministic and sort topologically.

[thegecko]

I've been looking into doing this by unbinding or re-declaring modules instead and although it feels like the right way to go, I agree a lot of code needs to be written. For example, the core menu items are all declared in one contribution. Removing just the settings menus requires nearly all of that contribution to be written again.

To be clear, the approach I've proposed here still doesn't feel right. Perhaps the right thing to do would be to refactor the core contributions?

Regarding the dependency tree, as long as you import the contribution modules to unregister or unbind, the ordering should work.

[thegecko]

We've tested unbinding contributions using inversify for the menu items we are trying to unregister.

unbind(WorkspaceFrontendContribution);
unbind(PreferencesContribution);
unbind(OutlineViewContribution);
unbind(ThemingCommandContribution);

In all cases, the menu system stops working, so our only method of removing them is by using the unregister commands outlined here. I'd be keen to see this PR merged.

[svenefftinge]

I don't like relying on side effects and the order of which they get executed, but due to lack of better alternatives, I'm ok with adding this API.