Closed
Bug 606482
Opened 14 years ago
Closed 14 years ago
_install_-updates preference wrongly labeled with "_check_ for"
Categories
(SeaMonkey :: Preferences, defect)
SeaMonkey
Preferences
Tracking
(Not tracked)
VERIFIED
FIXED
seamonkey2.1b3
People
(Reporter: dsb, Assigned: ewong)
References
Details
Attachments
(2 files, 8 obsolete files)
(deleted),
image/jpeg
|
Details | |
(deleted),
patch
|
ewong
:
review+
neil
:
ui-review+
|
Details | Diff | Splinter Review |
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?)
Assignee | ||
Comment 1•14 years ago
|
||
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
Assignee: nobody → ewong
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 2•14 years ago
|
||
This is the current behaviour 2.0.10.
Assignee | ||
Comment 3•14 years ago
|
||
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•14 years ago
|
||
(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.
Assignee | ||
Comment 5•14 years ago
|
||
Attachment #489764 -
Flags: review?(iann_bugzilla)
Attachment #489764 -
Flags: review?(iann_bugzilla) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #489764 -
Flags: superreview?(neil)
Comment 6•14 years ago
|
||
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•14 years ago
|
||
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.
Attachment #489764 -
Flags: superreview?(neil) → superreview-
Comment 8•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
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).
Assignee | ||
Comment 11•14 years ago
|
||
(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•14 years ago
|
||
(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.
Assignee | ||
Comment 13•14 years ago
|
||
Changed the organization of the Software Installation preference panel in accordance to comment #8.
Attachment #489764 -
Attachment is obsolete: true
Attachment #510911 -
Flags: review?(iann_bugzilla)
Comment 14•14 years ago
|
||
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.
Attachment #510911 -
Flags: review?(iann_bugzilla) → review-
Comment 15•14 years ago
|
||
(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?)
Assignee | ||
Comment 16•14 years ago
|
||
Fixed nits from comment #14.
Attachment #510911 -
Attachment is obsolete: true
Attachment #512099 -
Flags: review?(iann_bugzilla)
Comment 17•14 years ago
|
||
(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?
Assignee | ||
Comment 18•14 years ago
|
||
v2 + fix for comment #17.
Attachment #512099 -
Attachment is obsolete: true
Attachment #512099 -
Flags: review?(iann_bugzilla)
Assignee | ||
Updated•14 years ago
|
Attachment #512137 -
Flags: review?(iann_bugzilla)
Comment 19•14 years ago
|
||
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.
Attachment #512137 -
Flags: review?(iann_bugzilla) → review-
Comment 20•14 years ago
|
||
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.
Assignee | ||
Comment 21•14 years ago
|
||
Attachment #512137 -
Attachment is obsolete: true
Attachment #513409 -
Flags: review?(iann_bugzilla)
Comment 22•14 years ago
|
||
> + <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•14 years ago
|
||
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.
Attachment #513409 -
Flags: review?(iann_bugzilla) → review-
Assignee | ||
Comment 24•14 years ago
|
||
Attachment #513409 -
Attachment is obsolete: true
Attachment #513845 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #513845 -
Flags: review? → review?(iann_bugzilla)
Comment 25•14 years ago
|
||
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
Attachment #513845 -
Flags: review?(iann_bugzilla) → review-
Assignee | ||
Comment 26•14 years ago
|
||
Attachment #513845 -
Attachment is obsolete: true
Attachment #515059 -
Flags: review?(iann_bugzilla)
Comment 27•14 years ago
|
||
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.
Attachment #515059 -
Flags: review?(iann_bugzilla) → review+
Assignee | ||
Comment 28•14 years ago
|
||
Attachment #515059 -
Attachment is obsolete: true
Attachment #515267 -
Flags: ui-review?(neil)
Attachment #515267 -
Flags: review+
Comment 29•14 years ago
|
||
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 :-(
Assignee | ||
Comment 31•14 years ago
|
||
Attachment #515267 -
Attachment is obsolete: true
Attachment #518702 -
Flags: ui-review?(neil)
Attachment #518702 -
Flags: review+
Attachment #515267 -
Flags: ui-review?(neil)
Updated•14 years ago
|
Attachment #518702 -
Flags: ui-review?(neil) → ui-review+
Updated•14 years ago
|
Keywords: checkin-needed
Comment 32•14 years ago
|
||
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
Attachment #518702 -
Attachment description: Reorganized the Software Installation preferences panel. (v8) → Reorganized the Software Installation preferences panel. (v8) [Checkin: comment 32]
Updated•14 years ago
|
Keywords: checkin-needed
Target Milestone: --- → seamonkey2.1b3
Comment 33•14 years ago
|
||
Did you mean to close this now that it's checked in or anything else to do?
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
blocking-seamonkey2.1: ? → ---
Comment 34•14 years ago
|
||
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!
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•