386706 - update check should be disabled if not possible

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

The update check in the Help menu should be disabled if it's not possible to use it (if it's locked or if the application directory is not writable for a user).

Firefox and Thunderbird do the same.

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch

This patch uses the same mechanisms used in Firefox to modify the menu item.

Comment 2

[Martin Schröder [:mschroeder]]

Wolfgang, please request review for your patch (see http://wiki.mozilla.org/Calendar:Module_Ownership for an appropriate reviewer).

Comment 3

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 270711 [details] [diff] [review]
patch

Was told that lilmatt might not have enough time to review

Comment 4

[Daniel Boelzle [:dbo]]

Comment on attachment 270711 [details] [diff] [review]
patch

Sorry for the delay...

>+++ locales/en-US/chrome/calendar/calendar.properties	3 Jul 2007 09:43:51 >+updatesItem_defaultFallback=Check for Updates...
I'd prefer "_fallback" identifiers.
Please use the ellipsis charaacter U+2026 like in <http://lxr.mozilla.org/mozilla1.8/source/calendar/locales/en-US/chrome/calendar/menuOverlay.dtd#45>.

>+++ resources/content/applicationUtil.js	3 Jul 2007 09:43:52 -0000
>+    var updates = 
>+        Components.classes["@mozilla.org/updates/update-service;1"].
>+        getService(Components.interfaces.nsIApplicationUpdateService);
>+    var um = 
>+        Components.classes["@mozilla.org/updates/update-manager;1"].
>+        getService(Components.interfaces.nsIUpdateManager);
Please use more verbose variable names, like updateService and updateManager.

>+    if (!canUpdate)
>+      return; 
use four spaces, add curly braces

>+    // If there's an active update, substitute its name into the label
>+    // we show for this item, otherwise display a generic label.
>+    function getStringWithUpdateName(key) {
>+      if (activeUpdate && activeUpdate.name)
>+        return strings.getFormattedString(key, [activeUpdate.name]);
>+      return strings.getString(key + "Fallback");
>+    }
Since this function is used only once, I'd like to get rid of it.

>+    // By default, show "Check for Updates..."
>+    var key = "default";
>+    if (activeUpdate) {
>+      switch (activeUpdate.state) {
>+      case "downloading":
... four spaces like all over established in applicationUtils.js

rest looks good, but r- for now.

Comment 5

[Wolfgang Rosenauer [:wolfiR]]

Just to make that clear. All this is a 1-to-1 copy from Firefox. I basically have no objections to diverge but I haven't expected that to be a requirement ;-)

Comment 6

[Martin Schröder [:mschroeder]]

Attachment: Patch v2

I take this bug to finish it before string freeze (and because Daniel asked me to ;)).

(In reply to comment #4)
> >+++ locales/en-US/chrome/calendar/calendar.properties	3 Jul 2007 09:43:51 >+updatesItem_defaultFallback=Check for Updates...
> I'd prefer "_fallback" identifiers.

I think, we shouldn't rename the properties. Localizers know them from Firefox and Thunderbird.

Comment 7

[Daniel Boelzle [:dbo]]

Comment on attachment 277402 [details] [diff] [review]
Patch v2

One last suggestion to be discussed: Since calendar.properties is shared with lightning, but this is a Sunbird-only feature, it IMO makes sense to create a new Sunbird-specific resource file, e.g. sunbird.properties. Opinions?

r=dbo with that suggestion resolved.

Comment 8

[Martin Schröder [:mschroeder]]

(In reply to comment #7)
> (From update of attachment 277402 [details] [diff] [review])
> One last suggestion to be discussed: Since calendar.properties is shared with
> lightning, but this is a Sunbird-only feature, it IMO makes sense to create a
> new Sunbird-specific resource file, e.g. sunbird.properties. Opinions?

We have no Sunbird specific properties file at the moment, so maybe this should be introduced in a follow-up bug. I also think there are Lightning-only properties in calendar.properties that should go into the existing lightning.properties file. Maybe there should be a bug for general properties cleanup.

Comment 9

[Daniel Boelzle [:dbo]]

(In reply to comment #8)
Ok, we should do that in a follow-up bug which revises the l10n files; would you please write one?
For now, please tag the added section in calendar.properties (e.g. *Sunbird only*), so it's easier when being moved later on.

Comment 10

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH; thanks for the patch, Wolfgang and Martin!

Comment 11

[Daniel Boelzle [:dbo]]

Martin, please mind to diff patches that properly reflect the files' directories which makes it easier to apply them.

Comment 12

[Martin Schröder [:mschroeder]]

Filed bug 395567 as follow-up.