710159 - GCLI needs a command to control addons

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Joe Walker [:jwalker]]

> plugins list
> plugins enable <name> --global
> plugins disable <name> --global

(or something)

Comment 1

[Joe Walker [:jwalker]]

Triage. Filter on PEGASUS.

Comment 2

[Joe Walker [:jwalker]]

GCLI Triage.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Command

Comment 4

[Joe Walker [:jwalker]]

Comment on attachment 637079 [details]
Command

Here's a hack until we have true async types:

AddonManager.getAddonsByTypes(["plugin"], function(addons) {
  // cache names
  gcli.addCommand(addonCommandSpec);
});

I wonder if there is an event that is published by the addon manager when the list changes?


Also, so we can share the list of plugin names with others, we should do:

// Create a custom type for plugin names.

function PluginNameType() {
  this.stringifyProperty = 'name';
}

PluginNameType.prototype = Object.create(SelectionType.prototype);

PluginNameType.prototype.name = 'pluginname';

PluginNameType.prototype.lookup = function() {
  return pluginList.map(function(plugin) {
    return { name: plugin.name, value: plugin };
  }, this);
};

// This is a hack, we should avoid doing this
Components.utils.import("resource:///modules/devtools/Require.jsm");
var types = require('gcli/types');

types.registerType(PluginNameType);

Comment 5

[Joe Walker [:jwalker]]

Just to make sure we don't lose things - did you see Pimm's contributions:
https://gist.github.com/2998412
pimm is often around on IRC so we might be able to get his help - pimmhogeling

Comment 6

[Joe Walker [:jwalker]]

Don't forget Panos's work in https://bugzilla.mozilla.org/attachment.cgi?id=636849

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Filter on teabags

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Pimm's contributions are awesome!

As discussed on irc we could add the following:
- addon list <type> where type is one of dictionary, extension, locale, plugin or theme
- Partial string matching (maybe regex matching)

We should run the addons tests.

I also need to add some addon events.

Comment 10

[Joe Walker [:jwalker]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #9)
> - Partial string matching (maybe regex matching)

Partial string matching should be done automatically by GCLI, and I think that we'll get confused if we try to add regexp matching to this.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

github repo https://github.com/Pimm/gcli-addon

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We should also include <type> in enable / disable and install / uninstall

Comment 13

[pimmhogeling]

Code is now at https://github.com/Pimm/gcli-addon

If anyone needs write access, mail me (or mention me on Twitter: @pimmhogeling)

Comment 14

[pimmhogeling]

Installing an add-on can take quite a while. Feedback is an absolute must.

Therefore, I'd say this "bug" depends on #768414, perhaps.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to pimmhogeling from comment #14)
> Installing an add-on can take quite a while. Feedback is an absolute must.
> 
> Therefore, I'd say this "bug" depends on #768414, perhaps.

I thought we decided that we would file a followup for install/uninstall. In that case, the followup would depend on bug 768414.

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Panos Astithas [:past] from comment #15)
> I thought we decided that we would file a followup for install/uninstall. In
> that case, the followup would depend on bug 768414.

We did, bug 771066 logged.

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: v1

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 639649 [details] [diff] [review]
v1

Review of attachment 639649 [details] [diff] [review]:
-----------------------------------------------------------------

(I only briefly looked at the AddonManager API usage, not anything else.)

::: browser/devtools/commandline/GcliCommands.jsm
@@ +579,5 @@
> +    }
> +
> +    function representDisabledAddon(aAddon) {
> +      return "<li class=\"gcli-addon-disabled\">" +
> +        "<![CDATA[" + aAddon.name + "\u2002" + aAddon.version + "]]></li>";

Might want to add something to indicate whether an addon is pending enable/disable/uninstall/update here and in representEnabledAddon. Can check for that using addon.pendingOperations and the various AddonManager.PENDING_* constants (ie, addon.pendingOperations & AddonManager.PENIDNG_UNINSTALL).

@@ +603,5 @@
> +      let enabledAddons = [];
> +      let disabledAddons = [];
> +
> +      aAddons.forEach(function(aAddon) {
> +        if (aAddon.userDisabled) {

Note that .userDisabled isn't a measure of whether an add-on is actually enabled or disabled. Should be using .isActive here.

@@ +660,5 @@
> +    let name = aAddon.name + " " + aAddon.version;
> +    return name.trim();
> +  }
> +
> +  let addonNameCache = [];

This is going to get outdated - at the very least, you know it gets outdated when the enable/disable commands are run.

@@ +713,5 @@
> +        let name = representAddon(addon);
> +
> +        if (!addon.userDisabled) {
> +          this.resolve("<![CDATA[" +
> +            gcli.lookupFormat("addonAlreadyEnabled", [name]) + "]]>");

If the user has enabled a disabled non-restartless add-on and hasn't yet restarted, then userDisabled will be true even though the add-on won't be enabled until after Firefox next starts up. Check addon.pendingOperations & AdonManager.PENDING_ENABLE. (Same for the add-on disable command.)

Comment 19

[Joe Walker [:jwalker]]

Comment on attachment 639649 [details] [diff] [review]
v1

Review of attachment 639649 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/commandline/GcliCommands.jsm
@@ +551,5 @@
> + */
> +gcli.addCommand({
> +  name: "addon",
> +  description: {
> +    "root": gcli.lookup("addonDesc")

We've got a mix of 2 ways to do l10n here.
What we mean is just:
 description: gcli.lookup("addonDesc")

@@ +567,5 @@
> +  params: [{
> +    name: 'type',
> +    type: {
> +      name: 'selection',
> +      data: ["dictionary", "extension", "locale", "plugin", "theme"]

Did we have a rationale for not having an "all" option (and maybe having that as the defaultValue?

@@ +672,5 @@
> +    },
> +    description: {
> +      "root": gcli.lookup("addonNameDesc")
> +    }
> +  };

Ug.
We really need to expose addType().

That way we could have:
{
  name: "name",
  type: "addonName",
  description: gcli.lookup("addonNameDesc")
}

And I don't think we'd feel the pain of the repetition and want to pull it out. Maybe even types could expose descriptions.

  { name: "name", type: "addonName" }

Anyway, not your job. Just thinking out loud.

::: browser/devtools/commandline/test/browser_gcli_addon.js
@@ +38,5 @@
> +      DeveloperToolbarTest.checkInputStatus({
> +        typed: "addon enable Test_Plug-in_1.0.0.0",
> +        status: "VALID"
> +      });
> +      DeveloperToolbarTest.exec({ completed:false });

So we're executing "addon enable Test_Plug-in_1.0.0.0" here. I think we need to make the command we're executing more explicit (but that's not your job, the test framework should be better here)

How about we exec "addon list plugin" and check that "Test_Plug-in_1.0.0.0" is part of the output?
I think that would be:

DeveloperToolbarTest.exec({
  typed: "addon list plugin",
  completed: false,
  outputMatch: /Test_Plug-in_1.0.0.0/
});

::: browser/themes/winstripe/devtools/gcli.css
@@ +127,5 @@
> +.gcli-addon-disabled {
> +  opacity: 0.6;
> +  text-decoration: line-through;
> +}
> +

This is a picky nit, I know, but please could you move .gcli-addon-disabled after all the .gcli-menu items. Unless we keep things in sections it's going to turn into a pigs ear in a few years.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: patch II - the sequel

(In reply to Blair McBride (:Unfocused) from comment #18)
> Comment on attachment 639649 [details] [diff] [review]
> v1
> 
> Review of attachment 639649 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (I only briefly looked at the AddonManager API usage, not anything else.)
> 
> ::: browser/devtools/commandline/GcliCommands.jsm
> @@ +579,5 @@
> > +    }
> > +
> > +    function representDisabledAddon(aAddon) {
> > +      return "<li class=\"gcli-addon-disabled\">" +
> > +        "<![CDATA[" + aAddon.name + "\u2002" + aAddon.version + "]]></li>";
> 
> Might want to add something to indicate whether an addon is pending
> enable/disable/uninstall/update here and in representEnabledAddon. Can check
> for that using addon.pendingOperations and the various
> AddonManager.PENDING_* constants (ie, addon.pendingOperations &
> AddonManager.PENIDNG_UNINSTALL).
> 

Done, I now add pending operations in brackets after the addon name.

> @@ +603,5 @@
> > +      let enabledAddons = [];
> > +      let disabledAddons = [];
> > +
> > +      aAddons.forEach(function(aAddon) {
> > +        if (aAddon.userDisabled) {
> 
> Note that .userDisabled isn't a measure of whether an add-on is actually
> enabled or disabled. Should be using .isActive here.
> 

Changed

> @@ +660,5 @@
> > +    let name = aAddon.name + " " + aAddon.version;
> > +    return name.trim();
> > +  }
> > +
> > +  let addonNameCache = [];
> 
> This is going to get outdated - at the very least, you know it gets outdated
> when the enable/disable commands are run.
> 

Added an addonListener to handle this.

> @@ +713,5 @@
> > +        let name = representAddon(addon);
> > +
> > +        if (!addon.userDisabled) {
> > +          this.resolve("<![CDATA[" +
> > +            gcli.lookupFormat("addonAlreadyEnabled", [name]) + "]]>");
> 
> If the user has enabled a disabled non-restartless add-on and hasn't yet
> restarted, then userDisabled will be true even though the add-on won't be
> enabled until after Firefox next starts up. Check addon.pendingOperations &
> AdonManager.PENDING_ENABLE. (Same for the add-on disable command.)

I now add pending operations in brackets after the addon name.

(In reply to Joe Walker from comment #19)
> Comment on attachment 639649 [details] [diff] [review]
> v1
> 
> Review of attachment 639649 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/GcliCommands.jsm
> @@ +551,5 @@
> > + */
> > +gcli.addCommand({
> > +  name: "addon",
> > +  description: {
> > +    "root": gcli.lookup("addonDesc")
> 
> We've got a mix of 2 ways to do l10n here.
> What we mean is just:
>  description: gcli.lookup("addonDesc")
> 

Fixed

> @@ +567,5 @@
> > +  params: [{
> > +    name: 'type',
> > +    type: {
> > +      name: 'selection',
> > +      data: ["dictionary", "extension", "locale", "plugin", "theme"]
> 
> Did we have a rationale for not having an "all" option (and maybe having
> that as the defaultValue?
> 

Added

> @@ +672,5 @@
> > +    },
> > +    description: {
> > +      "root": gcli.lookup("addonNameDesc")
> > +    }
> > +  };
> 
> Ug.
> We really need to expose addType().
> 
> That way we could have:
> {
>   name: "name",
>   type: "addonName",
>   description: gcli.lookup("addonNameDesc")
> }
> 
> And I don't think we'd feel the pain of the repetition and want to pull it
> out. Maybe even types could expose descriptions.
> 
>   { name: "name", type: "addonName" }
> 
> Anyway, not your job. Just thinking out loud.
> 

+1 from me ;o)

> ::: browser/devtools/commandline/test/browser_gcli_addon.js
> @@ +38,5 @@
> > +      DeveloperToolbarTest.checkInputStatus({
> > +        typed: "addon enable Test_Plug-in_1.0.0.0",
> > +        status: "VALID"
> > +      });
> > +      DeveloperToolbarTest.exec({ completed:false });
> 
> So we're executing "addon enable Test_Plug-in_1.0.0.0" here. I think we need
> to make the command we're executing more explicit (but that's not your job,
> the test framework should be better here)
> 
> How about we exec "addon list plugin" and check that "Test_Plug-in_1.0.0.0"
> is part of the output?
> I think that would be:
> 
> DeveloperToolbarTest.exec({
>   typed: "addon list plugin",
>   completed: false,
>   outputMatch: /Test_Plug-in_1.0.0.0/
> });
> 

That would be nice. Unfortunately checking the output in addition to using "completed: false" fails because the output has not yet been built.

> ::: browser/themes/winstripe/devtools/gcli.css
> @@ +127,5 @@
> > +.gcli-addon-disabled {
> > +  opacity: 0.6;
> > +  text-decoration: line-through;
> > +}
> > +
> 
> This is a picky nit, I know, but please could you move .gcli-addon-disabled
> after all the .gcli-menu items. Unless we keep things in sections it's going
> to turn into a pigs ear in a few years.

Done

Comment 21

[Joe Walker [:jwalker]]

Comment on attachment 640314 [details] [diff] [review]
patch II - the sequel

Review of attachment 640314 [details] [diff] [review]:
-----------------------------------------------------------------

I think we could s/cliArguments/args/g and extract some of the nested functions up a level.
But we also need to get this in. So I suggest that we raise a bug 'Polish needed in GCLI addons command'.
OK?

Comment 22

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: v3

Bug 772376 created for tidying the code.

Also renamed cliArguments.

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Added to master patch in bug 768998

Comment 24

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Copy / paste error ... added to master patch in but 771555.