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

Add support for lumino plugin deactivation #54

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fcollonval
Copy link
Member

This add the support for the plugin deactivate method that will be support in JupyterLab 3.6.

@welcome
Copy link

welcome bot commented Dec 14, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@bollwyvl
Copy link

y'all got any other breaking changes hidden up yer sleeve?

@bollwyvl
Copy link

(to be fair, many of these are very cool, but i'm really wondering about why these are being backported: every one of them reduces the chance that existing third-party extensions will work properly, which is going to lead to a lot of unhappy users on launch day)

@fcollonval
Copy link
Member Author

@bollwyvl we don't use the deactivate in lumino application nor in JupyterLab. And if we start to do so (for instance for the extension manager), we should really first have a discussion about which core plugin will support it.

But for this project to work with a code like the one below, we need a proper way to deactivate the previous version. Otherwise reloading the plugin will fail due to the command ID being already registered.

import {
  JupyterFrontEnd,
  JupyterFrontEndPlugin,
} from '@jupyterlab/application';
import {
  DisposableSet
} from '@lumino/disposable';

const resources = new DisposableSet();

const plugin: JupyterFrontEndPlugin<void> = {
  id: 'mydynamicplugin',
  autoStart: true,
  requires: ['@jupyterlab/apputils:ICommandPalette'],
  activate: (app, palette) => {
    console.log('Hello from a dynamically loaded plugin!');

    // We can use `.app` here to do something with the JupyterLab
    // app instance, such as adding a new command
    let commandID = 'MySuperCoolDynamicCommand';
    let toggle = true;
    const cmd = app.commands.addCommand(commandID, {
      label: 'My Super Cool  Command',
      isToggled: function () {
        return toggle;
      },
      execute: function () {
        console.log('Executed ' + commandID + 'uu');
        toggle = !toggle;
      }
    });
    resources.add(cmd);

    const item = palette.addItem({
      command: commandID,
      // make it appear right at the top!
      category: 'AAA',
      args: {}
    });
    resources.add(item);
  },
  deactivate: () => {
    resources.dispose();
    resources.clear();
  }
};

export default plugin;

And incidentally this raises a good question, is it really easier to create a plugin for JupyterLab as it requires a deactivate function that is otherwise not needed. And that function will require good knowledge to be written properly.

@bollwyvl
Copy link

Again, it's important that these things get improved... but on X, rather than X.y. In a playground setting, exposing these things with docs is great and helpful.

}
this.app.deregisterPlugin(plugin.id, true);
} catch (reason) {
showErrorMessage('Deactivation of the old plugin failed', reason);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably preserve compatibility with 3.x branch here. Below is a suggestion for one way to do so (though checking for this.app.deactivatePlugin/this.app.deregisterPlugin could be better).

Suggested change
showErrorMessage('Deactivation of the old plugin failed', reason);
try {
if (plugin.deactivate) {
plugin.deactivate()
}
delete (this.app as any)._pluginMap[plugin.id];
} catch (otherReason) {
showErrorMessage('Deactivation of the old plugin failed', reason, otherReason);
}

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

Successfully merging this pull request may close these issues.

3 participants