Closed Bug 573062 Opened 14 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)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla2.0b3
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: jboriss, Assigned: asaf)

References

Details

(Whiteboard: [AddonsRewrite])

Attachments

(1 file, 2 obsolete files)

for checkin
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)
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
Yeah.
Depends on: 573149
Back-end patch is on bug 573149.
Attached 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-
Attached patch patch with test (obsolete) — — Splinter Review 

        Attachment #455430 -
        Attachment is obsolete: true

        Attachment #456126 -
        Flags: review?(bmcbride)

    
    

    
  
bmcbride: ping.

    
    

    
  
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;
> > > >+          }

    
    

    
  

      
      
      
      

        Attached patch
          
          
        for checkin
        — 
          — Splinter Review
      

    


  
before checkin, hg add toolkit/mozapps/extensions/test/browser/browser_bug573062.js

        Attachment #456126 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
  
Landed: http://hg.mozilla.org/mozilla-central/rev/5b4a338e639a
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3

    
  
Keywords: checkin-needed

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: