606482 - _install_-updates preference wrongly labeled with "_check_ for"

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Daniel B.]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.13) Gecko/20100914 SeaMonkey/2.0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.13) Gecko/20100914 SeaMonkey/2.0.8

In the Software Installation preferences node, the "Automatically check 
for updates" label is wrong.  

The SeaMonkey check box control below it does not control just whether 
SeaMonkey _checks_ for updates--it also controls whether SeaMonkey 
downloads and automatically _installs_ updates without otherwise asking 
the user.

The control should be labeled accurately.  The label text should also say
something like "and automatically install."



Reproducible: Always

Steps to Reproduce:
1.  Have the checkbox checked.
2.  Wait for an update to SeaMonkey and for SeaMonkey to notice it.

Actual Results:  
SeaMonkey presents a "Software Update" box that says "... The update 
will be installed the next time SeaMonkey starts."  (SeaMonkey does
not ask before installing the checked-for update.)


Expected Results:  
With a preferences label saying said "... check for ...", SeaMonkey 
should only check for (and possibly download and ask the user about 
installing) the update.  It should not install the update.  

If the preferences label had said that the control controlled automatic
installation, then installing the update without further confirmation
might be appropriate.  

(Also the minimum fix it to make the wording and behavior match, and 
better change would probably be to have two checkboxes--one for
checking, and a second for automatically proceeding with installation.

This is a data-loss bug.  (The update overwrites the user's SeaMonkey
installation, right?)

Comment 1

[Edmund Wong (:ewong)]

Confirmed with the following setup:

Build identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.15) Gecko/20101027 SeaMonkey/2.0.10

Assigning to myself

Comment 2

[Edmund Wong (:ewong)]

Attachment: Screen shot of current Software Installation preference screen.

This is the current behaviour 2.0.10.

Comment 3

[Edmund Wong (:ewong)]

A possible suggestion is to replace the current wording with
the following:

  "Automatically check for and install updates to:"

Another possible (for future bug reference) option is to
separate the 'check for updates' and 'install updates'
functionality.  This is along the lines of telling the
user that an update exists; but lets the user download
the update manually.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #3)
> A possible suggestion is to replace the current wording with
> the following:
> 
>   "Automatically check for and install updates to:"

I'd go for that solution for the time being.

> Another possible (for future bug reference) option is to
> separate the 'check for updates' and 'install updates'
> functionality.

AFAIK that functionality lives in Toolkit (shared code), i.e. that back-end first had to offer that possibility (file or search for a bug in that component, get it fixed etc.) before we could adapt our pref UI for it.

Comment 5

[Edmund Wong (:ewong)]

Attachment: Changed label to reflect the fact that it both checks and installs updates without user intervention.

Comment 6

[[e-mail]]

The new Add-ons Manager actually has a preference for this,
extensions.update.autoUpdateDefault

Application updates are managed by the existing app.update.auto preference.

Comment 7

[[e-mail]]

Comment on attachment 489764 [details] [diff] [review]
Changed label to reflect the fact that it both checks and installs updates without user intervention.

So, I checked and extensions.update.autoUpdateDefault is indeed the default setting (it can be overridden on a per-extension basis, but I don't think that concerns us here) for whether extensions should automatically update (assuming automatic checking). This means we ought to make this preference more visible (it's a little hidden in the add-ons manager.) We should probably reorganise the pane into two sections, one for add-ons and one for SeaMonkey.

Comment 8

[[e-mail]]

Something like this, perhaps?

-- Add-ons -------------------------------------------------------------
  [X] Allow websites to install add-ons and updates (Allowed Websites)
  [X] Automatically check for updates (.) daily ( ) weekly
  [X] Automatically download and install the updates
-- SeaMonkey -----------------------------------------------------------
  [X] Automatically check for updates (.) daily ( ) weekly
  [X] Automatically download and install the update
      [X] Warn me if this will disable any of my add-ons
                                                 (Show Update History)

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #8)
>  [X] Allow websites to install add-ons and updates (Allowed Websites)

It just appeared to me that Personas (lightweight themes) also fall into this category... Help! :-)

>  [X] Automatically download and install the updates

Are add-ons really installed automatically nowadays? I'd guess the user is still presented a list of add-on updates and can decide whether to install or not, no?

Otherwise looks good to me.

Hmm, the pane is called "Software Installation" which is problematic by itself IMHO (maybe should be renamed to "Install & Update").

Comment 10

[Robert Kaiser]

From a fast look, I think comment #8 is pretty much what I had proposed earlier on as well. IMHO, this bug's description is actually wrong, as the label is not wrong, just that we right now don't expose the pref that tells us to automatically install updates when we detect some to be available when we check.

Also, I think we should add some descriptions that clearly say that turning off automatic installation is a potential security risk as those updates often fix potential security issues. We may also want to make clear that in the case of application updates, the pref to automatically install is only about security/stability updates and NOT about versions with new features (we'll always prompt and ask about the latter, never download or install them automatically).

Comment 11

[Edmund Wong (:ewong)]

(In reply to comment #9)
> Otherwise looks good to me.
> 
> Hmm, the pane is called "Software Installation" which is problematic by itself
> IMHO (maybe should be renamed to "Install & Update").

I think "Updates" would suffice enough as this section/pane is just
what it is about.  The problem I'm having is whether or not this is
consistent with the other advanced panes' names.

Comment 12

[Robert Kaiser]

(In reply to comment #11)
> I think "Updates" would suffice enough as this section/pane is just
> what it is about.

No, it's also about allowing add-on installation at all.

Comment 13

[Edmund Wong (:ewong)]

Attachment: Reorganized the Software Installation preference panel. (v1)

Changed the organization of the Software Installation preference panel in accordance to comment #8.

Comment 14

[Ian Neal]

Comment on attachment 510911 [details] [diff] [review]
Reorganized the Software Installation preference panel. (v1)

>+++ b/suite/common/pref/pref-smartupdate.xul
>@@ -63,6 +64,9 @@
>       <preference id="extensions.update.interval"
>                   name="extensions.update.interval"
>                   type="int"/>
>+      <preference id="extensions.update.autoUpdateDefault"
>+                  name="extensions.update.autoUpdateDefault"
>+                  type="bool"/>
> 
Nit: remove this unneeded blank line.
>       <preference id="app.update.enabled"
>                   name="app.update.enabled"
>@@ -85,91 +89,84 @@
>     </preferences>
> 
>     <groupbox>
>+      <caption label="&addOnsTitle.label;"/>
>+        <hbox>
>+          <checkbox id="XPInstallEnabled" label="&addOnsAllow.label;" flex="1"
>+                    accesskey="&addOnsAllow.accesskey;" preference="xpinstall.enabled"/>
Nit: one attribute per line please, and throughout this file.
>+          <button id="allowedSitesButton"
>+                  label="&allowedWebsites.label;"
>+                  accesskey="&allowedWebsites.accesskey;"
>+                  oncommand="toDataManager('|permissions');"/>
This button is too tall now, still need the align="center" on the hbox perhaps?

>+              <checkbox id="addOnsUpdatesEnabled"
>+                        label="&autoAddOnsUpdates.label;"
>+                        accesskey="&autoAddOnsUpdates.accesskey;"
>+                        preference="extensions.update.enabled"/>
Shouldn't the unchecking of this checkbox also disable the addOnsModeAutoEnabled checkbox?
If it does then that checkbox should be indented too.
>+          <button id="startAddonManager"
>+                  oncommand="toEM('addons://list/extensions');"
>+                  label="&addonManagerButton.label;"
>+                  accesskey="&addonManagerButton.accesskey;"/>
I think this button looks better on its own line (also too tall but that will probably be address when on its own line).

>+              <checkbox id="appUpdatesEnabled"
>+                        label="&autoAppUpdates.label;"
>+                        accesskey="&autoAppUpdates.accesskey;"
>+                        preference="app.update.enabled"/>
Shouldn't the unchecking of this checkbox also disable the appModeAutoEnabled and appWarnIncompatible checkboxes?
If it does then the checkboxes should be indented further.

>+++ b/suite/locales/en-US/chrome/common/pref/pref-smartupdate.dtd

>+<!ENTITY addOnsTitle.label                    "Add-ons">
>+<!ENTITY addOnsAllow.label                    "Allow websites to install add-ons and updates">
>+<!ENTITY addOnsAllow.accesskey                "t">
Maybe use "b" here.

>+<!ENTITY allowedWebsites.label                "Allowed Websites">
>+<!ENTITY allowedWebsites.accesskey            "W">
>+<!ENTITY autoAddOnsUpdates.label              "Automatically check for updates">
>+<!ENTITY autoAddOnsUpdates.accesskey          "S">
Maybe use "o" here.

>+<!ENTITY addOnsDaily.label                    "daily">
Didn't spot this earlier but we don't need to have separate entities for the daily and weekly labels for addons vs app, so leave as daily.label and weekly.label.

>+<!ENTITY addOnsDaily.accesskey                "d">
>+<!ENTITY addOnsWeekly.label                   "weekly">
>+<!ENTITY addOnsWeekly.accesskey               "k">
>+<!ENTITY addOnsModeAutomatic.label            "Automatically download and install the updates">
>+<!ENTITY addOnsModeAutomatic.accesskey        "c">
>+<!ENTITY addonManagerButton.label             "Add-on Manager">
>+<!ENTITY addonManagerButton.accesskey         "M">
> 
>+<!ENTITY appUpdates.caption                   "&brandShortName;">
>+<!ENTITY autoAppUpdates.label                 "Automatically check for updates">
>+<!ENTITY autoAppUpdates.accesskey             "p">
Maybe use "t" here.

>+<!ENTITY appDaily.label                       "daily">
>+<!ENTITY appDaily.accesskey                   "i">
We're not using "a" so use that here.

>+<!ENTITY appWeekly.label                      "weekly">
>+<!ENTITY appWeekly.accesskey                  "e">
>+<!ENTITY appModeAutomatic.label               "Automatically download and install the update">
>+<!ENTITY appModeAutomatic.accesskey           "u">
>+<!ENTITY appModeAutoAddonWarn.label           "Warn me if this will disable any of my add-ons">
>+<!ENTITY appModeAutoAddonWarn.accesskey       "b">
Maybe use "n" here.

>+<!ENTITY updateHistory.label                  "Show Update History">
>+<!ENTITY updateHistory.accesskey              "H">
Cannot use "H" as it is used for Help button, so use "S" here.

> <!ENTITY extensionsUpdates.label              "Installed Add-ons">
> <!ENTITY extensionsUpdates.accesskey          "o">
> <!ENTITY extensionsDaily.accesskey            "a">
> <!ENTITY extensionsWeekly.accesskey           "k">
You forget to delete these entities.
> 
> <!ENTITY whenUpdatesFound.label               "When updates to &brandShortName; are found">
> <!ENTITY askMe.label                          "Ask me what I want to do">
> <!ENTITY askMe.accesskey                      "n">
And these ones.

Comment 15

[[e-mail]]

(As a side note, the add-ons manager button doesn't work for me, either with or without the patch. It just says I don't have any add-ons of this type installed. Anyone know what's going wrong there?)

Comment 16

[Edmund Wong (:ewong)]

Attachment: Reorganized the Software Installation preference panel. (v2)

Fixed nits from comment #14.

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #15)
> (As a side note, the add-ons manager button doesn't work for me, either with or
> without the patch. It just says I don't have any add-ons of this type
> installed. Anyone know what's going wrong there?)

Typo: Cf.
http://mxr.mozilla.org/comm-central/source/suite/common/pref/pref-smartupdate.xul#138
vs.
http://mxr.mozilla.org/comm-central/source/suite/common/bindings/notification.xml#1388
or
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/extensions/test/browser/browser_bug581076.js#123

Edmund, maybe you can piggy-back fix it here?

Comment 18

[Edmund Wong (:ewong)]

Attachment: Reorganized the Software Installation preferences panel. (v3)

v2 + fix for comment #17.

Comment 19

[Ian Neal]

Comment on attachment 512137 [details] [diff] [review]
Reorganized the Software Installation preferences panel. (v3)

>+++ b/suite/common/pref/pref-smartupdate.xul
>+      <preference id="extensions.update.autoUpdateDefault"
>+                  name="extensions.update.autoUpdateDefault"
>+                  type="bool"/>
> 
Nit: Remove this extra blank line.
>       <preference id="app.update.enabled"
>                   name="app.update.enabled"

>+      <caption label="&addOnsTitle.label;"/>
>+        <hbox>
>+          <checkbox id="XPInstallEnabled"
>+                    label="&addOnsAllow.label;"
>+                    flex="1"
>+                    accesskey="&addOnsAllow.accesskey;"
>+                    preference="xpinstall.enabled"/>
>+          <button id="allowedSitesButton"
>+                  label="&allowedWebsites.label;"
>+                  accesskey="&allowedWebsites.accesskey;"
>+                  oncommand="toDataManager('|permissions');"/>
>+        </hbox>
Still a problem with the button being too tall, having hbox align="center" seemed to work before, but test and see.

>+              <checkbox id="addOnsUpdatesEnabled"
>+                        class="indent"
>+                        label="&autoAddOnsUpdates.label;"
>+                        accesskey="&autoAddOnsUpdates.accesskey;"
>+                        preference="extensions.update.enabled"/>
Unchecking this checkbox should disable the checkbox addOnsModeAutoEnabled shouldn't it? If so, that second checkbox should be further indented.

>+        <checkbox id="appModeAutoEnabled"
>                   class="indent"
>+                  label="&appModeAutomatic.label;"
>+                  flex="1"
>+                  accesskey="&appModeAutomatic.accesskey;"
>+                  preference="app.update.auto"/>
Unchecking this checkbox should disable the checkbox appWarnIncompatible shouldn't it? If so, that second checkbox should be further indented.

>+++ b/suite/locales/en-US/chrome/common/pref/pref-smartupdate.dtd
> <!ENTITY updateHistory.label                  "Show Update History">
>+<!ENTITY updateHistory.accesskey              "H">
This still needs changing to "S" as "H" is used for help.

Comment 20

[Philip Chee]

Err. Two one row grids doesn't make much sense to me especially since they are in different groupboxes. Previous to this patch we have a grid with two rows to make the checkboxes and radio buttons line up.

Also class=indent applies to any xul element not just checkboxes.

Comment 21

[Edmund Wong (:ewong)]

Attachment: Reorganized the Software Installation preference panel. (v4)

Comment 22

[Philip Chee]

> +        <hbox>
> +           <checkbox id="addOnsUpdatesEnabled"
> +                     class="indent"
I think putting the indent on the hbox instead should work.

> +        <checkbox id="appWarnIncompatible"
> +                  class="indenttwice"
Try wrapping the checkbox in a <hbox class="indent"> instead. Then you don't have to fudge around with global.css.

Comment 23

[Ian Neal]

Comment on attachment 513409 [details] [diff] [review]
Reorganized the Software Installation preference panel. (v4)

>+++ b/suite/common/pref/pref-smartupdate.xul
>+        <hbox>
>+           <checkbox id="addOnsUpdatesEnabled"
>+                     class="indent"
As Philip suggest, try moving the class to the hbox and then you should be able to just use class="indent" on the second checkbox.

>+        <checkbox id="appModeAutoEnabled"
>                   class="indent"
Again as Philip suggests, wrap in an hbox with the class="indent" on it, then you don't need on this checkbox and the second checkbox just needs class="indent".

>+++ b/suite/themes/modern/global/global.css
>+.indenttwice {
>+  -moz-margin-start: 40px;
>+}
>+
Not needed now, but if it was you'd forgotten classic!

r- for the moment, but almost there. Sorry I missed the grid thing.

Comment 24

[Edmund Wong (:ewong)]

Attachment: Reorganized the Software Installation preferences panel. (v5)

Comment 25

[Ian Neal]

Comment on attachment 513845 [details] [diff] [review]
Reorganized the Software Installation preferences panel. (v5)

You've now got the indentation correct. You now just need to sort out the disabling/enabling of the double indented checkboxes when the one above is unchecked/checked

Comment 26

[Edmund Wong (:ewong)]

Attachment: Reorganized the Software Installation preferences panel. (v6)

Comment 27

[Ian Neal]

Comment on attachment 515059 [details] [diff] [review]
Reorganized the Software Installation preferences panel. (v6)

>+++ b/suite/common/pref/pref-smartupdate.js
> function UpdateAddonsItems()
> {
>+  if (!document.getElementById("xpinstall.enabled").value) {
>+    document.getElementById("addOnsUpdatesEnabled").value = false;
>+  }
>+
>+  document.getElementById("addOnsUpdatesEnabled").disabled =
>     !document.getElementById("xpinstall.enabled").value ||
>     document.getElementById("extensions.update.enabled").locked;
> 
>+  document.getElementById("addOnsUpdateFrequency").disabled =
>     !document.getElementById("xpinstall.enabled").value ||
>     !document.getElementById("extensions.update.enabled").value ||
>     document.getElementById("extensions.update.interval").locked;
>+
>+  document.getElementById("allowedSitesButton").disabled =
>+    !document.getElementById("xpinstall.enabled").value ||
>+    document.getElementById("extensions.update.enabled").locked;
>+
>+  document.getElementById("addOnsModeAutoEnabled").disabled =
>+    !document.getElementById("xpinstall.enabled").value ||
>+    !document.getElementById("extensions.update.enabled").value ||
>+    document.getElementById("extensions.update.enabled").locked;
>+

This:
     !document.getElementById("xpinstall.enabled").value ||
     document.getElementById("extensions.update.enabled").locked;
seems to be used 3 times in the above, you could possible set a variable to this and use it in those 3 locations.


> 
>@@ -114,22 +117,11 @@ function UpdateAutoItems()
> {
>   var disabled = !gCanCheckForUpdates||
>                  !document.getElementById("app.update.enabled").value ||
>+                 !document.getElementById("app.update.auto").value ||
>                  document.getElementById("app.update.auto").locked;
>+  document.getElementById("appWarnIncompatible").disabled =
>+    disabled;
As the var disabled is only used the once now, you could inline it.

r=me with those fixed/addressed.

Comment 28

[Edmund Wong (:ewong)]

Attachment: Reorganized the Software Installation preference panel. (v7)

Comment 29

[[e-mail]]

Comment on attachment 515267 [details] [diff] [review]
Reorganized the Software Installation preference panel. (v7)

Is it me or does addOns look odd to you. (addOnsUpdates is doubly odd; only one of the words should be plural.)

It probably sounds easy to me because I'm used to it, but an element should be disabled if either a) other prefs make it unavailable or meaningless or b) the element's own pref is locked. You seem to have got a) right in all cases, but unfortunately b) seems to be inconsistent at best...

> function UpdateAddonsItems()
> {
>+  var addOnsCheck = !document.getElementById("xpinstall.enabled").value ||
>+                      document.getElementById("extensions.update.enabled").locked;
Only the addonsUpdateEnabled element cares whether this pref is locked, so that check belongs to that element only.

>+  if (!document.getElementById("xpinstall.enabled").value) {
>+    document.getElementById("addOnsUpdatesEnabled").value = false;
>+  }
This is just completely bogus. Fortunately it doesn't do anything.

>+  document.getElementById("addOnsUpdateFrequency").disabled =
>     !document.getElementById("xpinstall.enabled").value ||
>     !document.getElementById("extensions.update.enabled").value ||
>     document.getElementById("extensions.update.interval").locked;
[This one is right, of course...]

>+  document.getElementById("allowedSitesButton").disabled =
>+    addOnsCheck;
This is a special case (it doesn't make sense to lock it) so moving the pref locked check out of the addonsCheck variable will automatically fix it!

>+  document.getElementById("addOnsModeAutoEnabled").disabled =
>+    addOnsCheck ||
>+    !document.getElementById("extensions.update.enabled").value;
This isn't checking the locked status of the correct preference. (And moving the pref locked check only solves half of the problem this time.)

>   document.getElementById("appUpdateFrequency").disabled =
>     !enabledPref.value || !gCanCheckForUpdates ||
>     document.getElementById("app.update.interval").locked;
>+
>+  document.getElementById("appModeAutoEnabled").disabled =
>+    !enabledPref.value || !gCanCheckForUpdates ||
>+    document.getElementById("app.update.interval").locked;
Too much copy/paste...

>+    !gCanCheckForUpdates||
Nit: space before ||

>-    document.getElementById("app.update.mode").locked;
>+    document.getElementById("app.update.auto").locked;
The old line was correct :-(

Comment 30

[Jens Hatlak (:InvisibleSmiley)]

Contains string changes -> should block b3.

Comment 31

[Edmund Wong (:ewong)]

Attachment: Reorganized the Software Installation preferences panel. (v8) [Checkin: comment 32]

Comment 32

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 518702 [details] [diff] [review]
Reorganized the Software Installation preferences panel. (v8) [Checkin: comment 32]

http://hg.mozilla.org/comm-central/rev/3d14636e266f

Comment 33

[rsx11m]

Did you mean to close this now that it's checked in or anything else to do?

Comment 34

[rsx11m]

Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110316 SeaMonkey/2.1b3pre now that all follow-ups are in. Updating the help content is bug 641252. Edmund's redesign of this page looks great!