541665 - Sync Suite pref-smartupdate.* with mozilla-central

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Philip Chee]

1. Add UI for app.update.auto app.update.mode
2. Enable/disable the UI based on the "locked" states of associated preferences.

Comment 1

[Philip Chee]

Attachment: Patch v1.0

Initial patch as proposed.

Comment 2

[Ian Neal]

Comment on attachment 423140 [details] [diff] [review]
Patch v1.0

> function Startup()
> {
>-  toggleFrequency("extensions");
>-  toggleFrequency("app");
>+  UpdateAddonsItems();
>+  UpdateAppItems();
>+  UpdateAutoItems();
>+  UpdateModeItems();
Drop the calls to UpdateAutoItems() and UpdateModeItems() from here, then have UpdateAppItems() call UpdateAutoItems() and have UpdateAutoItems() call UpdateModeItems().

>+function UpdateAddonsItems()
> {
>+  document.getElementById("extensionsUpdatesEnabled").disabled =
>+    !document.getElementById("xpinstall.enabled").value ||
>+    document.getElementById("extensions.update.enabled").locked;
>+
>+  var locked = !document.getElementById("extensions.update.enabled").value ||
>+               document.getElementById("extensions.update.interval").locked ||
>+               !document.getElementById("xpinstall.enabled").value;
>+  document.getElementById("extensionsFreqDaily").disabled = locked;
>+  document.getElementById("extensionsFreqWeekly").disabled = locked;
Why not have here:
var disable = !document.getElementById("xpinstall.enabled").value ||
              document.getElementById("extensions.update.enabled").locked;
document.getElementById("extensionsUpdatesEnabled").disabled = disable;

disable = disable || !document.getElementById("extensions.update.enabled").value;
document.getElementById("extensionsFreqDaily").disabled = disable;
document.getElementById("extensionsFreqWeekly").disabled = disable;
> }
>+
>+function UpdateAppItems()
>+{

>+
>+  var enabledPref = document.getElementById("app.update.enabled");
>+  var appUpdatesEnabled = document.getElementById("appUpdatesEnabled");
This variable is only used once, so just inline it.
>+
>+  appUpdatesEnabled.disabled = !canCheckForUpdates || enabledPref.locked;
>+
>+  var locked = !enabledPref.value ||
>+               document.getElementById("app.update.interval").locked ||
>+               !canCheckForUpdates;
Keep the var name consistent so use disable as the name here, also put the !canCheckForUpdates on the same line as !enabledPref.value.
>+
>+  document.getElementById("appFreqDaily").disabled = locked;
>+  document.getElementById("appFreqWeekly").disabled = locked;
Call UpdateAutoItems() at then end of this function.
>+}

>+function UpdateAutoItems()
>+{
>+  var enabledPref = document.getElementById("app.update.enabled");
>+  var autoPref = document.getElementById("app.update.auto");
>+  var updateModeLabel = document.getElementById("updateModeLabel");
>+  var updateMode = document.getElementById("updateMode");
These last three variables are used only once, so inline them.
>+
>+  var disable = enabledPref.locked ||
>+                !enabledPref.value ||
>+                autoPref.locked;
Put the two enabledPref on the same line.
>+  updateModeLabel.disabled = disable;
>+  updateMode.disabled = disable;
Call UpdateModeItems() from here.
>+}

>+function UpdateModeItems()
>+{
>+  var enabledPref = document.getElementById("app.update.enabled");
>+  var autoPref = document.getElementById("app.update.auto");
>+  var modePref = document.getElementById("app.update.mode");
>+  var warnIncompatible = document.getElementById("warnIncompatible");
These last two variables are only used once so inline them.
>+
>+  var disable = enabledPref.locked ||
>+                !enabledPref.value ||
>+                autoPref.locked ||
>+                !autoPref.value ||
>+                modePref.locked;
Put the two enabledPref and the two autoPref on the same lines.
>+  warnIncompatible.disabled = disable;
>+}

>+function ReadAddonWarn()
>+{
>+  var preference = document.getElementById("app.update.mode");
>+  var doNotWarn = preference.value != 0;
>+  gModePreference = doNotWarn ? preference.value : 1;
>+  return doNotWarn;
>+}

>+function WriteAddonWarn()
Use the following and just pass "this" as argument from the caller.
function WriteAddonWarn(aWarn)
{
  return !aWarn.checked ? 0 : gModePreference;
}

>+function ShowUpdateHistory()
>+{
>+  Components.classes["@mozilla.org/updates/update-prompt;1"]
>+             .createInstance(Components.interfaces.nsIUpdatePrompt)
>+             .showUpdateHistory(window);
Indentation appears to be incorrect.

>--- a/suite/common/pref/pref-smartupdate.xul

>+      <preference id="xpinstall.enabled"
>+                  name="xpinstall.enabled"
>+                  type="bool"
>+                  onchange="UpdateAddonsItems();"/>
>+      <preference id="extensions.update.enabled"
>+                  name="extensions.update.enabled"
>+                  type="bool"
>+                  onchange="UpdateAddonsItems();"/>
>       <preference id="extensions.update.interval"
>-                  name="extensions.update.interval" type="int"/>
>+                  name="extensions.update.interval"
>+                  type="int"/>
>+
>+      <preference id="app.update.enabled"
>+                  name="app.update.enabled"
>+                  type="bool"
>+                  onchange="UpdateAppItems();UpdateAutoItems();UpdateModeItems();"/>
Just call UpdateAppItems() here, as that will be sufficient with the changes previously mentioned.
>+      <preference id="app.update.auto"
>+                  name="app.update.auto"
>+                  type="bool"
>+                  onchange="UpdateAutoItems();UpdateModeItems();"/>
Just call UpdateAutoItems() here, as that will be sufficient with the changes previously mentioned.

>+    <groupbox>
>+      <caption id="updateModeLabel"
>+               label="&whenUpdatesFound.label;"
>+               control="updateMode"/>
>+      <radiogroup id="updateMode"
>+                  class="indent"
>+                  preference="app.update.auto">
>+        <radio id="ask"
>+               label="&askMe.label;"
>+               accesskey="&askMe.accesskey;"
>+               value="false"/>
>+        <radio id="automatic"
>+               label="&modeAutomatic.label;"
>+               accesskey="&modeAutomatic.accesskey;"
>+               value="true"/>
Are these ids unique enough? Might be better to be updateModeAsk and updateModeAutomatic, if they're needed at all.

>+        <hbox class="indent">
>+          <checkbox id="warnIncompatible"
>+                    label="&modeAutoAddonWarn.label;"
>+                    accesskey="&modeAutoAddonWarn.accesskey;"
>+                    flex="1"
>+                    preference="app.update.mode"
>+                    onsyncfrompreference="return this.pane.ReadAddonWarn();"
>+                    onsynctopreference="return this.pane.WriteAddonWarn();"/>
Pass "this" as an argument for WriteAddonWarn

Comment 3

[Philip Chee]

Attachment: Patch v1.1

> Drop the calls to UpdateAutoItems() and UpdateModeItems() from here, then have
> UpdateAppItems() call UpdateAutoItems() and have UpdateAutoItems() call
> UpdateModeItems().
Fixed.

>>+function UpdateAddonsItems()
>> {
>>+  document.getElementById("extensionsUpdatesEnabled").disabled =
>>+    !document.getElementById("xpinstall.enabled").value ||
>>+    document.getElementById("extensions.update.enabled").locked;
>>+
>>+  var locked = !document.getElementById("extensions.update.enabled").value ||
>>+               document.getElementById("extensions.update.interval").locked ||
>>+               !document.getElementById("xpinstall.enabled").value;
>>+  document.getElementById("extensionsFreqDaily").disabled = locked;
>>+  document.getElementById("extensionsFreqWeekly").disabled = locked;

> Why not have here:
> var disable = !document.getElementById("xpinstall.enabled").value ||
>               document.getElementById("extensions.update.enabled").locked;
> document.getElementById("extensionsUpdatesEnabled").disabled = disable;
> 
> disable = disable ||
> !document.getElementById("extensions.update.enabled").value;
> document.getElementById("extensionsFreqDaily").disabled = disable;
> document.getElementById("extensionsFreqWeekly").disabled = disable;

One uses:
document.getElementById("extensions.update.enabled").locked;

The other uses:
!document.getElementById("extensions.update.enabled").value

Or are you saying that they are equivalent?
(Fix pending clarification...)

>>+  var appUpdatesEnabled = document.getElementById("appUpdatesEnabled");
> This variable is only used once, so just inline it.
Fixed.

>>+  var locked = !enabledPref.value ||
>>+               document.getElementById("app.update.interval").locked ||
>>+               !canCheckForUpdates;
> Keep the var name consistent so use disable as the name here, also put the
> !canCheckForUpdates on the same line as !enabledPref.value.
Fixed.

>>+  document.getElementById("appFreqDaily").disabled = locked;
>>+  document.getElementById("appFreqWeekly").disabled = locked;
> Call UpdateAutoItems() at then end of this function.
Fixed.

>>+  var autoPref = document.getElementById("app.update.auto");
>>+  var updateModeLabel = document.getElementById("updateModeLabel");
>>+  var updateMode = document.getElementById("updateMode");
> These last three variables are used only once, so inline them.
Fixed.

>>+  var disable = enabledPref.locked ||
>>+                !enabledPref.value ||
>>+                autoPref.locked;
> Put the two enabledPref on the same line.
Fixed.

>>+  updateModeLabel.disabled = disable;
>>+  updateMode.disabled = disable;
> Call UpdateModeItems() from here.
Fixed.

>>+  var modePref = document.getElementById("app.update.mode");
>>+  var warnIncompatible = document.getElementById("warnIncompatible");
> These last two variables are only used once so inline them.
Fixed.

>>+  var disable = enabledPref.locked ||
>>+                !enabledPref.value ||
>>+                autoPref.locked ||
>>+                !autoPref.value ||
>>+                modePref.locked;
>Put the two enabledPref and the two autoPref on the same lines.
Fixed.

>>+function WriteAddonWarn()
> Use the following and just pass "this" as argument from the caller.
> function WriteAddonWarn(aWarn)
> {
>   return !aWarn.checked ? 0 : gModePreference;
> }
Fixed.

>>+  Components.classes["@mozilla.org/updates/update-prompt;1"]
>>+             .createInstance(Components.interfaces.nsIUpdatePrompt)
>>+             .showUpdateHistory(window);
> Indentation appears to be incorrect.
Fixed.

>>+      <preference id="app.update.enabled"
>>+                  name="app.update.enabled"
>>+                  type="bool"
>>+                  onchange="UpdateAppItems();UpdateAutoItems();UpdateModeItems();"/>
> Just call UpdateAppItems() here, as that will be sufficient with the changes
> previously mentioned.
Fixed.

>>+      <preference id="app.update.auto"
>>+                  name="app.update.auto"
>>+                  type="bool"
>>+                  onchange="UpdateAutoItems();UpdateModeItems();"/>
> Just call UpdateAutoItems() here, as that will be sufficient with the changes
> previously mentioned.
Fixed.

>>+        <radio id="ask"
>>+        <radio id="automatic"
> Are these ids unique enough? Might be better to be updateModeAsk and
> updateModeAutomatic, if they're needed at all.
Fixed.

>>+          <checkbox id="warnIncompatible"
>>+                    label="&modeAutoAddonWarn.label;"
>>+                    accesskey="&modeAutoAddonWarn.accesskey;"
>>+                    flex="1"
>>+                    preference="app.update.mode"
>>+                    onsyncfrompreference="return this.pane.ReadAddonWarn();"
>>+                    onsynctopreference="return this.pane.WriteAddonWarn();"/>
> Pass "this" as an argument for WriteAddonWarn
Fixed.

Comment 4

[Philip Chee]

Attachment: Patch v1.2 Ignore legacy mode 2 in app.update.mode and treat it as a boolean.

Changes since the last patch:

> + * The app.update.mode preference is converted into a true/false value for
> + * use in determining whether the "Warn me if this will disable extensions
> + * or themes" checkbox is checked. Unlike other toolkit applications we
> + * don't care about supporting legacy mode 2. 

[2010-01-28 22:51:45] <RattyAway> so just have 0 and 1?
[2010-01-28 22:51:51] <RattyAway> and ignore > 1
[2010-01-28 22:51:58] * NeilAway doesn't think the pref window needs to cope with power lusers setting bogus pref values
[2010-01-28 22:52:05] <RattyAway> or treat anything >1 as 1?
[2010-01-28 22:52:31] <NeilAway> RattyAway: treat non-0 as 1, yes

> +      <preference id="app.update.disable_button.showUpdateHistory"
> +                  name="app.update.disable_button.showUpdateHistory"
> +                  type="bool"/> 
Somehow I missed out this preference until now.

> +          <checkbox id="warnIncompatible"
> +                    preference="app.update.mode"
> +                    onsyncfrompreference="return document.getElementById('app.update.mode').value != 0;"
> +                    onsynctopreference="return !this.checked ? 0 : 1;"/> 
In-lined these two functions and removed ReadAddonWarn()/WriteAddonWarn(aWarn) from pref-smartupdate.js

Comment 5

[[e-mail]]

Eww, accesskey alert - use of narrow letters and descenders! Try this:
a daily
b Warn me if this will disable any of my add-ons
c Automatically download and install the update
d daily
e Allowed Sites
h Help
k weekly
m Add-on Manager
n Ask me what I want to do
o Installed Add-ons
s SeaMonkey
u Show Update History
w weekly
x Allow web sites to install extensions and updates

Comment 6

[[e-mail]]

Comment on attachment 424182 [details] [diff] [review]
Patch v1.2 Ignore legacy mode 2 in app.update.mode and treat it as a boolean.

>+function UpdateAppItems()
>+{
>+  var aus = Components.classes["@mozilla.org/updates/update-service;1"]
Hmm, I don't have an update service in my build, makes testing harder ;-)

>+      <radiogroup id="updateMode"
>+                  class="indent"
Nothing to indent under. (Unlike the checkbox, which is correctly indented.)

>+        <hbox class="indent">
Lose the hbox and put the class on the checkbox.

>+          <checkbox id="warnIncompatible"
>+                    label="&modeAutoAddonWarn.label;"
>+                    accesskey="&modeAutoAddonWarn.accesskey;"
>+                    flex="1"
Don't need the flex (with or without the hbox)

>+                    onsyncfrompreference="return document.getElementById('app.update.mode').value != 0;"
Fortunately checkboxes already know how to convert integers to booleans.

>+                    onsynctopreference="return !this.checked ? 0 : 1;"/>
Switch the ? : around so you don't need the !
Ideally <preference>::getElementValue::getValue would be smarter and only try to convert strings to integers or booleans as necessary.

Comment 7

[[e-mail]]

Comment on attachment 424182 [details] [diff] [review]
Patch v1.2 Ignore legacy mode 2 in app.update.mode and treat it as a boolean.

>+  document.getElementById("extensionsUpdatesEnabled").disabled =
>+    !document.getElementById("xpinstall.enabled").value ||
Is this some sort of "master" preference?

>+function UpdateAppItems()
>+{
>+  var aus = Components.classes["@mozilla.org/updates/update-service;1"]
>+                      .getService(Components.interfaces.nsIApplicationUpdateService);
>+  // For 1.9.2 branch
>+  if (!("canCheckForUpdates" in aus))
>+    aus.QueryInterface(Components.interfaces.nsIApplicationUpdateService2);
>+  var canCheckForUpdates = aus.canCheckForUpdates;
>+
>+  var enabledPref = document.getElementById("app.update.enabled");
>+  document.getElementById("appUpdatesEnabled")
>+          .disabled = !canCheckForUpdates || enabledPref.locked;
This bit doesn't belong here since it never changes.

>+  var disabled = !enabledPref.value || !canCheckForUpdates ||
>+                 document.getElementById("app.update.interval").locked;
>+
>+  document.getElementById("appFreqDaily").disabled = disabled;
>+  document.getElementById("appFreqWeekly").disabled = disabled;
>+  UpdateAutoItems();
Or it would possibly be fairer to say that these belong together.

>+function UpdateAutoItems()
>+{
>+  var enabledPref = document.getElementById("app.update.enabled");
>+  var disable = enabledPref.locked || !enabledPref.value ||
That's not true, the locked state of app.update.enabled is irrelevant.
(But I guess canCheckForUpdates is.)

>+  var disable = enabledPref.locked || !enabledPref.value ||
>+                autoPref.locked || !autoPref.value ||
Again, locked state of other prefs is irrelevant.

Comment 8

[Ian Neal]

Comment on attachment 424182 [details] [diff] [review]
Patch v1.2 Ignore legacy mode 2 in app.update.mode and treat it as a boolean.

I'll wait til the next version of the patch.

Comment 9

[Philip Chee]

Attachment: Patch v1.3 Fixed Review Comments.

> Eww, accesskey alert - use of narrow letters and descenders! Try this:
> a daily
> b Warn me if this will disable any of my add-ons
> c Automatically download and install the update
> d daily
> e Allowed Sites
> H Help
> k weekly
> M Add-on Manager
> n Ask me what I want to do
> o Installed Add-ons
> S SeaMonkey
> u Show Update History
> w weekly
> x Allow web sites to install extensions and updates
Fixed.

>>+      <radiogroup id="updateMode"
>>+                  class="indent"
> Nothing to indent under. (Unlike the checkbox, which is correctly indented.)
Fixed.

>>+        <hbox class="indent">
>Lose the hbox and put the class on the checkbox.
Fixed.

>>+          <checkbox id="warnIncompatible"
>>+                    label="&modeAutoAddonWarn.label;"
>>+                    accesskey="&modeAutoAddonWarn.accesskey;"
>>+                    flex="1"
> Don't need the flex (with or without the hbox)
Fixed.

>>+                    onsyncfrompreference="return document.getElementById('app.update.mode').value != 0;"
> Fortunately checkboxes already know how to convert integers to booleans.
Fixed.

>>+                    onsynctopreference="return !this.checked ? 0 : 1;"/>
> Switch the ? : around so you don't need the !
> Ideally <preference>::getElementValue::getValue would be smarter and only try
> to convert strings to integers or booleans as necessary.
Fixed.

>>+  document.getElementById("extensionsUpdatesEnabled").disabled =
>>+    !document.getElementById("xpinstall.enabled").value ||
> Is this some sort of "master" preference?
I guess you could say this. If you can't install extensions, you can't update them either.

>>+function UpdateAppItems()
>>+{
>>+  var aus = Components.classes["@mozilla.org/updates/update-service;1"]
>>+                      .getService(Components.interfaces.nsIApplicationUpdateService);
>>+  // For 1.9.2 branch
>>+  if (!("canCheckForUpdates" in aus))
>>+    aus.QueryInterface(Components.interfaces.nsIApplicationUpdateService2);
>>+  var canCheckForUpdates = aus.canCheckForUpdates;
>>+
>>+  var enabledPref = document.getElementById("app.update.enabled");
>>+  document.getElementById("appUpdatesEnabled")
>>+          .disabled = !canCheckForUpdates || enabledPref.locked;
> This bit doesn't belong here since it never changes.
Moved down to the next section.

>>+  var disabled = !enabledPref.value || !canCheckForUpdates ||
>>+                 document.getElementById("app.update.interval").locked;
>>+
>>+  document.getElementById("appFreqDaily").disabled = disabled;
>>+  document.getElementById("appFreqWeekly").disabled = disabled;
>>+  UpdateAutoItems();
> Or it would possibly be fairer to say that these belong together.
Fixed.

>>+function UpdateAutoItems()
>>+{
>>+  var enabledPref = document.getElementById("app.update.enabled");
>>+  var disable = enabledPref.locked || !enabledPref.value ||
> That's not true, the locked state of app.update.enabled is irrelevant.
> (But I guess canCheckForUpdates is.)
Fixed. Made gCanCheckForUpdates a global so that it can be referenced by UpdateAutoItems().

>>+  var disable = enabledPref.locked || !enabledPref.value ||
>>+                autoPref.locked || !autoPref.value ||
> Again, locked state of other prefs is irrelevant.
Fixed.

Comment 10

[[e-mail]]

Comment on attachment 425633 [details] [diff] [review]
Patch v1.3 Fixed Review Comments.

>  * ***** END LICENSE BLOCK ***** */
>+var gCanCheckForUpdates;
Nit: blank line after licence block

>+  // Make it easier to access the pref pane from onsync.
>+  document.getElementById("warnIncompatible").pane = this;
Unused.

>+  document.getElementById("extensionsUpdatesEnabled").disabled =
>+    !document.getElementById("xpinstall.enabled").value ||
>+    document.getElementById("extensions.update.enabled").locked;
[See below.]

>+  document.getElementById("extensionsFreqDaily").disabled = locked;
>+  document.getElementById("extensionsFreqWeekly").disabled = locked;
Hmm, why did we never think to disable the <radiogroup> directly?

>+  document.getElementById("appUpdatesEnabled")
>+          .disabled = !gCanCheckForUpdates || enabledPref.locked;

>+  var disable = !enabledPref.value || !autoPref.value ||
>+                document.getElementById("app.update.mode").locked;
>+  document.getElementById("warnIncompatible").disabled = disable;
So, you seem to have three different disabling styles :-(

Comment 11

[Philip Chee]

Attachment: Patch v1.4 Fix more review comments.

>>  * ***** END LICENSE BLOCK ***** */
>>+var gCanCheckForUpdates;
> Nit: blank line after licence block
Fixed.

>>+  // Make it easier to access the pref pane from onsync.
>>+  document.getElementById("warnIncompatible").pane = this;
> Unused.
Fixed.

>>+  document.getElementById("extensionsUpdatesEnabled").disabled =
>>+    !document.getElementById("xpinstall.enabled").value ||
>>+    document.getElementById("extensions.update.enabled").locked;
> [See below.]
>
>>+  document.getElementById("extensionsFreqDaily").disabled = locked;
>>+  document.getElementById("extensionsFreqWeekly").disabled = locked;
> Hmm, why did we never think to disable the <radiogroup> directly?
Fixed. Now changed to disabling both extensionsUpdateFrequency and appUpdateFrequency.

>>+  document.getElementById("appUpdatesEnabled")
>>+          .disabled = !gCanCheckForUpdates || enabledPref.locked;
>
>>+  var disable = !enabledPref.value || !autoPref.value ||
>>+                document.getElementById("app.update.mode").locked;
>>+  document.getElementById("warnIncompatible").disabled = disable;
> So, you seem to have three different disabling styles :-(
I've tried to make things more consistent. I hope I've fixed all the bits.

Comment 12

[[e-mail]]

Comment on attachment 426206 [details] [diff] [review]
Patch v1.4 Fix more review comments.

There was one thing that did cross my mind when testing this... you have a checkbox for extensions, then a checkbox for SeaMonkey and a checkbox for extensions, then a radiogroup for SeaMonkey. In particular, the first checkbox disables the third checkbox, and the second checkbox disables the radiogroup. I just thought it looked a little odd, but I don't think we can switch around the rows due to the presence of the Add-on Manager button.

Comment 13

[[e-mail]]

Comment on attachment 426206 [details] [diff] [review]
Patch v1.4 Fix more review comments.

>+  // For 1.9.2 branch
>+  if (!("canCheckForUpdates" in aus))
>+    aus.QueryInterface(Components.interfaces.nsIApplicationUpdateService2);
This can go ;-) (plus the one in utilityOverlay.js while you're there)

Comment 14

[Philip Chee]

Attachment: Patch v1.5 Remove support for 1.9.2 sr=Neil

Carrying forward sr=Neil

>>+  // For 1.9.2 branch
>>+  if (!("canCheckForUpdates" in aus))
>>+    aus.QueryInterface(Components.interfaces.nsIApplicationUpdateService2);
> This can go ;-)
Fixed.

> (plus the one in utilityOverlay.js while you're there)
Sigh. Fixed.

Comment 15

[Ian Neal]

Comment on attachment 428195 [details] [diff] [review]
Patch v1.5 Remove support for 1.9.2 sr=Neil

Could you make sure that there is bug open for updating the help to reflect the changes in this bug (or that you do it in this bug).

Comment 16

[Philip Chee]

> Could you make sure that there is bug open for updating the help to reflect the
> changes in this bug (or that you do it in this bug).

Opened Bug 547931.

Comment 17

[Justin Wood (:Callek)]

Pushed to c-c: http://hg.mozilla.org/comm-central/rev/18bd4c198a74