Add "(restart required)" to tooltip for "Remove," "Disable," "Enable" buttons for add-ons requiring restart

VERIFIED FIXED in mozilla2.0b3

Status

()

defect
P2
normal
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: jboriss, Assigned: mano)

Tracking

Trunk
mozilla2.0b3
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [AddonsRewrite])

Attachments

(1 attachment, 2 obsolete attachments)

(restart required) should be added to the end of the tooltip string for add-ons that require a restart.  This makes the full string:

- Uninstall this add-on (restart required)
- Disable this add-on (restart required)
- Enable this add-on (restart required)
Whiteboard: [AddonsRewrite]
Version: unspecified → Trunk
Needs API support. While we're at it, may as well cover updates as well. Filed bug 573149.
Assignee: nobody → felipc
Status: NEW → ASSIGNED
felipe, I'm already working on this. If you haven't started yet, please assign it to me. If you did, thanks ;)
Oh, FYI, I talked to Dave and about the API required here.  We agreed that a operationsWouldRequireRestart bitfield would work best because there's a separate requires-restart check for each operation.
I haven't started yet on these bugs, Mano. Are you working on the API side too?
Assignee: felipc → mano
Posted patch patch (obsolete) — Splinter Review
This patch depends on the back-end part.
Attachment #455430 - Flags: review?(dtownsend)
Comment on attachment 455430 [details] [diff] [review]
patch

>+      if ("getTooltip" in cmd) {
>+        if (addon)
>+          cmdElt.setAttribute("tooltiptext", cmd.getTooltip(addon));
>+        else
>+          cmdElt.removeAttribute("tooltiptext");
>+      }

I think it should be letting cmd.getTooltip() determine what to do if there's no addon - as there are cases where no addon is perfectly ok (not in the case of these specific commands, but I'm thinking future-proofing).


>       <method name="_updateState">
...
>+            this._enableBtn.setAttribute("tooltiptext", tooltip);
>+          }
>+          else {
>+            this._enableBtn.hidden = true;
>+          }

For consistency with the rest of the file, remove the newline between "}" and "else" (and the other cases this happens).


>+            let tooltip = gViewController.commands["cmd_disableItem"]
>+                                         .getTooltip(this.mAddon);

I wonder if gViewController should have a function to do this - something like getTooltipForCommand(command, addon). Up to you - I'm fine with either way.


Can you add tests for this - one for the list view (testing the xbl binding), and one for the detail view (testing the command updater). Can use the mocking provider in /toolkit/mozapps/extensions/test/browser/head.js.

r- because I'd like to see the tests.
Attachment #455430 - Flags: review?(dtownsend) → review-
Posted patch patch with test (obsolete) — Splinter Review
Attachment #455430 - Attachment is obsolete: true
Attachment #456126 - Flags: review?(bmcbride)
Comment on attachment 456126 [details] [diff] [review]
patch with test

>diff -r c1bb84d2470d toolkit/mozapps/extensions/content/extensions.js

>     cmd_disableItem: {
>       isEnabled: function(aAddon) {
>         if (!aAddon)
>           return false;
>         return hasPermission(aAddon, "disable");
>       },
>       doCommand: function(aAddon) {
>         aAddon.userDisabled = true;
>+      },
>+      getTooltip: function(aAddon) {
>+        if (!aAddon)
>+          return "";
>+
>+        if (aAddon.operationsRequiringRestart & AddonManager.OP_NEEDS_RESTART_DISABLE)
>+          return gStrings.ext.GetStringFromName("disableAddonRestartRequiredTooltip");
>+
>+        return gStrings.ext.GetStringFromName("disableAddonTooltip");

You have an extra newline there that doesn't match the others, might as well make them consistent.

>diff -r c1bb84d2470d toolkit/mozapps/extensions/content/extensions.xml

>       <method name="_updateState">
>         <body><![CDATA[
>           this.setAttribute("incompatible", !this.mAddon.isCompatible);
> 
>           this._preferencesBtn.hidden = !this.mAddon.optionsURL;
> 
>-          this._enableBtn.hidden = !this.hasPermission("enable");
>-          this._disableBtn.hidden = !this.hasPermission("disable");
>-          this._removeBtn.hidden = !this.hasPermission("uninstall");
>+          if (this.hasPermission("enable")) {
>+            this._enableBtn.hidden = false;
>+            let tooltip = gViewController.commands["cmd_enableItem"]
>+                                         .getTooltip(this.mAddon);
>+            this._enableBtn.setAttribute("tooltiptext", tooltip);
>+          }
>+          else
>+            this._enableBtn.hidden = true;

The code style here requires braces around the else part too when the if part has them. Same for the others in this function.
Comment on attachment 456126 [details] [diff] [review]
patch with test

>diff -r c1bb84d2470d toolkit/mozapps/extensions/content/extensions.js

>       var canUpdate = hasPermission(aAddon, "upgrade");
>       document.getElementById("detail-autoUpdate").hidden = !canUpdate;
>       document.getElementById("detail-findUpdates").hidden = !canUpdate;
>       document.getElementById("detail-prefs").hidden = !aAddon.optionsURL;
> 
>       self.updateState();
> 
>       gViewController.updateCommands();
>+     // aWindow.alert("updated");
>       gViewController.notifyViewChanged();
>     });
>   },

Debugging code that snuck in I guess?
Attachment #456126 - Flags: review?(bmcbride) → review+
> >+          }
> >+          else
> >+            this._enableBtn.hidden = true;
> 
> The code style here requires braces around the else part too when the if part
> has them. Same for the others in this function.

Dave, see comment 8...
(In reply to comment #13)
> > >+          }
> > >+          else
> > >+            this._enableBtn.hidden = true;
> > 
> > The code style here requires braces around the else part too when the if part
> > has them. Same for the others in this function.
> 
> Dave, see comment 8...

Huh well I guess Blair wrote this code so he knows best. Must have snuck past me in the uber-review. Stick with the consistency.
I'll fix that patch tomorrow morning, and then, will do.
blocking2.0: beta2+ → final+
Sorry I didn't get to this - was on PTO for most of last week.


(In reply to comment #14)
> > Dave, see comment 8...
> 
> Huh well I guess Blair wrote this code so he knows best. Must have snuck past
> me in the uber-review. Stick with the consistency.

Er, actually, I meant remove the newline - not remove the braces. ie, instead of this:

>+          }
>+          else {
>+            this._enableBtn.hidden = true;
>+          }


It should be:

> > > >+          } else {
> > > >+            this._enableBtn.hidden = true;
> > > >+          }
Posted patch for checkinSplinter Review
before checkin, hg add toolkit/mozapps/extensions/test/browser/browser_bug573062.js
Attachment #456126 - Attachment is obsolete: true
Landed: http://hg.mozilla.org/mozilla-central/rev/5b4a338e639a
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3

Updated

9 years ago
Status: RESOLVED → VERIFIED
Verified on :
Mozilla/5.0 (Windows; Windows NT 6.0; rv:2.0b3) Gecko/20100805 Firefox/4.0b3

and

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3) Gecko/20100805
Firefox/4.0b3
You need to log in before you can comment on or make changes to this bug.