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

Fix Issue 8463 - calling extension activate() by other extension #8542

Merged
merged 1 commit into from
Oct 15, 2020
Merged

Fix Issue 8463 - calling extension activate() by other extension #8542

merged 1 commit into from
Oct 15, 2020

Conversation

tomer-epstein
Copy link
Contributor

@tomer-epstein tomer-epstein commented Sep 22, 2020

Fix Issue 8463 - calling extension activate() by other extension doesn't work.

Signed-off-by: Tomer Epstein tomer.epstein@sap.com

Issue: #8463

What it does

This change proposal adds a call to activate plugin when ext isn't active and developer called extension.activate() method.

How to test

  1. git clone https://github.com/tomer-epstein/vscode-extension-activate.git
  2. Pack (create vsix) for https://github.com/tomer-epstein/vscode-extension-activate/tree/master/vscode-manager
  3. Pack (create vsix) for https://github.com/tomer-epstein/vscode-extension-activate/tree/master/vscode-contrib1
  4. execute 'extension.activateContributor' command.

image
image
image

Review checklist

Reminder for reviewers

@tomer-epstein
Copy link
Contributor Author

@akosyakov @amiramw can you please assign yourself as reviewers?

@amiramw amiramw self-requested a review September 22, 2020 08:18
@vince-fugnitto vince-fugnitto added plug-in system issues related to the plug-in system vscode issues related to VSCode compatibility labels Sep 22, 2020
@@ -412,9 +412,10 @@ export class PluginManagerExtImpl implements PluginManagerExt, PluginManager {

if (this.activatedPlugins.get(pluginId)) {
deferred.resolve();
return deferred.promise;
Copy link
Member

Choose a reason for hiding this comment

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

deferred seems redundant in this function as it does not defer to something asynchronous

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't added the deferred. do you suggest removing it?

}
this.pluginActivationPromises.set(pluginId, deferred);
return deferred.promise;
return this.$activatePlugin(pluginId);
Copy link
Member

Choose a reason for hiding this comment

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

this.activatedPlugins should be updated so if it is called twice you return it from there, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's being updated at startPlugin() method

@@ -412,9 +412,10 @@ export class PluginManagerExtImpl implements PluginManagerExt, PluginManager {

if (this.activatedPlugins.get(pluginId)) {
deferred.resolve();
return deferred.promise;
}
this.pluginActivationPromises.set(pluginId, deferred);
Copy link
Member

Choose a reason for hiding this comment

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

is this map set only when calling activate plugin programatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's being set for all activate plugin methods.

@amiramw
Copy link
Member

amiramw commented Oct 1, 2020

@tomer-epstein @akosyakov @svor @benoitf

Looking at the code again I do not understand why we maintain this.pluginActivationPromises as a way to let activatePlugin get the plugin started eventually.
Why not replace activatePlugin implementation to simply call $activatePlugin?

If we are worried about load flow running twice in parallel for the same plugins then there is already a check for that here:

let loading = this.loadedPlugins.get(plugin.model.id);

@amiramw
Copy link
Member

amiramw commented Oct 8, 2020

@tomer-epstein

  1. you should remove this.pluginActivationPromises. it is not set anywhere now.
  2. can you add test scenario for calling activatePlugin when it is already activated by event and make sure it work?

…n't work.

Signed-off-by: Tomer Epstein <tomer.epstein@sap.com>
@tomer-epstein
Copy link
Contributor Author

@tomer-epstein

  1. you should remove this.pluginActivationPromises. it is not set anywhere now.
  2. can you add test scenario for calling activatePlugin when it is already activated by event and make sure it work?

test scenario added.

  1. git clone https://github.com/tomer-epstein/vscode-extension-activate.git
  2. Pack (create vsix) for https://github.com/tomer-epstein/vscode-extension-activate/tree/master/vscode-activation-event
  3. execute 'extension.activateMyself' command.
  4. execute again 'extension.activateMyself' command.

image
image
image

Copy link
Member

@amiramw amiramw left a comment

Choose a reason for hiding this comment

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

Code look good to me now and tested successfully both scenarios.

If someone else can also take a look that would be great.

@amiramw amiramw merged commit 329a3ef into eclipse-theia:master Oct 15, 2020
@tomer-epstein tomer-epstein deleted the extension.activate branch October 16, 2020 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants