Closed Bug 1549242 Opened 6 months ago Closed 6 months ago

Add doorhanger notifications for app update

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 68.0

People

(Reporter: rstrong, Assigned: rstrong)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

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.

Attached patch patch (obsolete) — Splinter Review

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/

Attachment #9062826 - Flags: review?(richard.marti)
Attached image Screenshots

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

Attachment #9062827 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review

hideClose for unsupported updates was incorrect in the previous patch

Attachment #9062826 - Attachment is obsolete: true
Attachment #9062826 - Flags: review?(richard.marti)
Attachment #9062830 - Flags: review?(richard.marti)

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

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.
Attachment #9062830 - Flags: review?(richard.marti) → review?(geoff)
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 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.
Attachment #9062830 - Flags: review?(geoff) → review+

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!

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.

Attached patch patch rev2Splinter Review

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?

Attachment #9062830 - Attachment is obsolete: true
Attachment #9063367 - Flags: review?(geoff)
Comment on attachment 9063367 [details] [diff] [review]
patch rev2

Assuming autoland merges in 4-5 hours as per usual, I'll land this then.
Attachment #9063367 - Flags: review?(geoff) → review+

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.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/5544aead04df
Add doorhanger notifications for app update; r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0
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.
Flags: needinfo?(robert.strong.bugs)
Flags: needinfo?(robert.strong.bugs)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/22240cf3e73a
Follow-up: Fix copy/paste error in AppUpdateUI.jsm. r=darktrojan DONTBUILD

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

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

See Also: → 1546992

(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

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"

Flags: needinfo?(robert.strong.bugs)

(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.

Flags: needinfo?(robert.strong.bugs)
See Also: → 1578175
You need to log in before you can comment on or make changes to this bug.