1549242 - Add doorhanger notifications for app update

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Robert Strong (they/them - no direct email)]

Not sure what the best Thunderbird component for this is so going with general.

The old app update UI will be going away soon and Thunderbird will need new UI for app update. I went with doorhangers since that is what Firefox uses.

I have a patch almost finished to add app update doorhangers.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch

This basically does for app update what bug 1496632 did for add-ons.

The pref removals in all-thunderbird.js are for prefs that the code that used them was removed a very long time ago.

The change to the app.update.staging.enabled pref is because staging was enabled on all platforms after I fixed a couple of bugs.

I went with initializing AppUpdateUI.jsm in _onProfileStartup because there are cases where the notification can happen earlier than _onMailStartupDone.

AppUpdateUI.jsm does most of what toolkit/mozapps/update/UpdateListener.jsm does except it uses GlobalPopupNotifications.jsm instead of AppMenuNotifications.jsm.

The strings are the same or close to the same as the strings Firefox uses.

The images are the same as the ones Firefox uses but I changed their name from update.svg to app-update.svg and update-badge.svg to app-update.svg in case I move these files into toolkit to lessen confusion.

There was some missing css in messenger.css that prevents the anchor icon from not being displayed when the anchor didn't have notifications associated with it. I also moved the images into css so it is possible to override the images with css.

I have one more notification for elevation requests that I'll add in another bug for both Firefox and Thunderbird.

Feel free to change the images but I'd prefer if that were done in another bug so this doesn't hold up other planned work in toolkit/mozapps/update/

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: Screenshots

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: Screenshots

One of the screenshots in the previous png had an extra screenshot from an earlier version of the patch.

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: patch

hideClose for unsupported updates was incorrect in the previous patch

Comment 5

[Robert Strong (they/them - no direct email)]

I would like to have this land soon after the next merge which is 5/13.

Comment 6

[Richard Marti (:Paenglab)]

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

Many thanks, Robert for taking care of TB. I'll move the review to Geoff who is the better reviewer for the jsm stuff. The other parts are looking good to me.

Comment 7

[Robert Strong (they/them - no direct email)]

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

>diff --git a/mail/locales/jar.mn b/mail/locales/jar.mn
>--- a/mail/locales/jar.mn
>+++ b/mail/locales/jar.mn
>@@ -21,16 +21,17 @@
>   locale/@AB_CD@/messenger/aboutSupportMail.properties                  (%chrome/messenger/aboutSupportMail.properties)
>   locale/@AB_CD@/messenger/app-extension-fields.properties              (%chrome/messenger/app-extension-fields.properties)
>   locale/@AB_CD@/messenger/telemetry.properties                         (%chrome/messenger/telemetry.properties)
>   locale/@AB_CD@/messenger/accountCreation.dtd                          (%chrome/messenger/accountCreation.dtd)
>   locale/@AB_CD@/messenger/accountCreation.properties                   (%chrome/messenger/accountCreation.properties)
>   locale/@AB_CD@/messenger/accountCreationModel.properties              (%chrome/messenger/accountCreationModel.properties)
>   locale/@AB_CD@/messenger/accountCreationUtil.properties               (%chrome/messenger/accountCreationUtil.properties)
>   locale/@AB_CD@/messenger/addons.properties                            (%chrome/messenger/addons.properties)
>+  locale/@AB_CD@/messenger/appUpdate.properties                        (%chrome/messenger/appUpdate.properties)
Fixed indent locally

Comment 8

[Geoff Lankow (:darktrojan)]

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

Review of attachment 9062830 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for dragging us along with you, Robert, especially when that means figuring out how to make stuff work. A few nits, but on the whole, good!

::: mail/base/modules/AppUpdateUI.jsm
@@ +126,5 @@
> +      accessKey: appUpdateBundle.getString("updateAvailablePrimaryButtonAccessKey"),
> +      callback: () => {
> +        Cc["@mozilla.org/updates/update-service;1"].
> +          getService(Ci.nsIApplicationUpdateService).
> +          downloadUpdate(update, true);

Nit-pick: dots at the start of the line.

::: mail/locales/en-US/chrome/messenger/appUpdate.properties
@@ +1,1 @@
> +updateAvailableTitle=A new %S update is available.

There should be a localisation note saying what %S is, even if it's just one note at the top for the whole file.

::: mail/themes/shared/mail/messenger.css
@@ +71,5 @@
> +}
> +
> +.addons-icon {
> +  list-style-image: url("chrome://mozapps/skin/extensions/extensionGeneric-16.svg");
> +}

This is .install-icon in messenger.xul, so this rule isn't applied. I think I actually prefer .addons-icon, but change whichever you like.

Comment 9

[Geoff Lankow (:darktrojan)]

I've started a Try run, since Robert's fell at the first hurdle yesterday:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9da1d2ea0d9562e34131c6df97e41cc59728a168

Comment 10

[Robert Strong (they/them - no direct email)]

Hi Geoff, thanks for the try run! I've updated the patch to include your comments and fixed the lint errors from the try run. I'll figure out what is going on with the xpcshell failures and then submit an updated patch. Cheers!

Comment 11

[Robert Strong (they/them - no direct email)]

I filed bug 1549859 to fix the intermittent failures I saw on comm-central and bug 1549835 to remove a couple of tests that are no longer needed and also fail on comm-central with this patch so this should be good to go as soon as I resubmit.

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

Hi Geoff, besides the review comments, I also included a new default pref that should prevent Thunderbird users from accidentally getting BITS turned on while it is being evaluated. The pref can be removed after the BITS trial is completed.

Bug 1521427, bug 1549859, and bug 1549835 have all landed on autoland and it should be fine to land this patch after they have merged to mozilla-central. Since I have comm-central locally would you like me to land this patch after those bugs have merged?

Comment 13

[Geoff Lankow (:darktrojan)]

Comment on attachment 9063367 [details] [diff] [review]
patch rev2

Assuming autoland merges in 4-5 hours as per usual, I'll land this then.

Comment 14

[Robert Strong (they/them - no direct email)]

If you are ok with a couple of xpcshell tests failing until bug 1549835 merges over to mozilla-central then this can land now.

bug 1549859 will fix a pre-existing intermittent failure of a couple of other xpcshell tests and landing the patch in this bug won't have any affect on that.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5544aead04df
Add doorhanger notifications for app update; r=darktrojan

Comment 16

[Geoff Lankow (:darktrojan)]

Comment on attachment 9063367 [details] [diff] [review]
patch rev2

Review of attachment 9063367 [details] [diff] [review]:
-----------------------------------------------------------------

Found a bug I didn't spot earlier. Do you wish to write a follow-up patch, or shall I?

::: mail/base/modules/AppUpdateUI.jsm
@@ +302,5 @@
> +
> +    let update = subject && subject.QueryInterface(Ci.nsIUpdate);
> +    switch (topic) {
> +      case "quit-application":
> +        this.observers.forEach(function(observer) {

this.observers is undefined. Looks like it should be this._obs.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/22240cf3e73a
Follow-up: Fix copy/paste error in AppUpdateUI.jsm. r=darktrojan DONTBUILD

Comment 18

[Jorg K (CEST = GMT+2)]

r+ via IRC:
11:32:21 - darktrojan: go ahead then
11:32:29 - jorgk: r=darktrojan, yes?
11:32:35 - darktrojan: sure

Comment 19

[Robert Strong (they/them - no direct email)]

I did some basic checks and all looks well. I'll try to help out if any issues are reported.

Comment 20

[WaltS48 [:walts48]]

(In reply to Robert Strong (Robert they/them) [:rstrong] (use needinfo to contact me) from comment #19)

I did some basic checks and all looks well. I'll try to help out if any issues are reported.

Issue reported New arrow symbol

Comment 21

[WaltS48 [:walts48]]

Attachment: Update Icon Only on Win10

My testing results with Thunderbird Daily 69.0a1 and Beta 68.0b2 to 68.0b3

Update for Thunderbird Daily 69.0a1 on Ubuntu 18.04.2 LTS
I get the doorhanger and click "Download Update"
I don't get a prompt to restart when the update is downloaded and applied
I have to open Help > About Daily and click the "Restart to update Daily" button

Update for Thunderbird Daily 69.0a1 on Linux Mint 19.1 Cinnamon
Same as above

Update for Thunderbird Beta 68.0b2 to 68.0b3 on Linux Mint 19.1 Cinnamon (Ubuntu already updated using Help > About)
Same as Thunderbird Daily on Ubuntu Linux
I have to open Help > About Thunderbird and click the "Restart to update Thunderbird"

Update for Thunderbird Daily on Windows 10
Only the icon with the arrow inside the green circle appears
I have to open Help > About Daily
Click "Update to 69.0a1 (2019-07-04)"
Update downloaded and applied
Click "Restart to update Daily"

Comment 22

[Robert Strong (they/them - no direct email)]

(In reply to WaltS48 [:walts48] from comment #20)

(In reply to Robert Strong (Robert they/them) [:rstrong] (use needinfo to contact me) from comment #19)

I did some basic checks and all looks well. I'll try to help out if any issues are reported.

Issue reported New arrow symbol

Please file a Thunderbird > General bug.