Closed
Bug 573062
Opened 15 years ago
Closed 14 years ago
Add "(restart required)" to tooltip for "Remove," "Disable," "Enable" buttons for add-ons requiring restart
Categories
(Toolkit :: Add-ons Manager, defect, P2)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla2.0b3
| Tracking | Status | |
|---|---|---|
| blocking2.0 | --- | final+ |
People
(Reporter: jboriss, Assigned: asaf)
References
Details
(Whiteboard: [AddonsRewrite])
Attachments
(1 file, 2 obsolete files)
|
15.86 KB,
patch
|
Details | Diff | Splinter Review |
(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)
|
||
Updated•15 years ago
|
Whiteboard: [AddonsRewrite]
Version: unspecified → Trunk
|
||
Comment 1•15 years ago
|
||
Needs API support. While we're at it, may as well cover updates as well. Filed bug 573149.
|
||
Updated•15 years ago
|
Assignee: nobody → felipc
Status: NEW → ASSIGNED
|
Assignee | |
Comment 2•15 years ago
|
||
felipe, I'm already working on this. If you haven't started yet, please assign it to me. If you did, thanks ;)
|
|
Assignee | |
Comment 3•15 years ago
|
||
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.
|
|
||
Comment 4•15 years ago
|
||
I haven't started yet on these bugs, Mano. Are you working on the API side too?
Assignee: felipc → mano
|
|
Assignee | |
Comment 5•15 years ago
|
||
Yeah.
|
|
Assignee | |
Comment 6•15 years ago
|
||
Back-end patch is on bug 573149.
|
|
Assignee | |
Comment 7•15 years ago
|
||
This patch depends on the back-end part.
Attachment #455430 -
Flags: review?(dtownsend)
|
|
||
Comment 8•15 years ago
|
||
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-
|
|
Assignee | |
Comment 9•15 years ago
|
||
Attachment #455430 -
Attachment is obsolete: true
Attachment #456126 -
Flags: review?(bmcbride)
|
|
Assignee | |
Comment 10•15 years ago
|
||
bmcbride: ping.
|
||
Comment 11•15 years ago
|
||
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 12•15 years ago
|
||
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+
|
|
Assignee | |
Comment 13•15 years ago
|
||
> >+ }
> >+ 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...
|
|
||
Comment 14•15 years ago
|
||
(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.
|
||
Comment 15•15 years ago
|
||
Can this land?
|
|
Assignee | |
Comment 16•15 years ago
|
||
I'll fix that patch tomorrow morning, and then, will do.
|
|
||
Updated•15 years ago
|
blocking2.0: beta2+ → final+
|
|
||
Comment 17•15 years ago
|
||
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;
> > > >+ }
|
|
Assignee | |
Comment 18•14 years ago
|
||
before checkin, hg add toolkit/mozapps/extensions/test/browser/browser_bug573062.js
Attachment #456126 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•14 years ago
|
Keywords: checkin-needed
|
|
||
Comment 19•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
|
||
Updated•14 years ago
|
Keywords: checkin-needed
|
||
Updated•14 years ago
|
Status: RESOLVED → VERIFIED
|
|
||
Comment 20•14 years ago
|
||
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.
Description
•