Background update of addons: Make success notification/toast 'Installation complete' more informative or remove it

RESOLVED FIXED in Firefox 25

Status

()

defect
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: pwd.mozilla, Assigned: capella)

Tracking

Trunk
Firefox 25
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

7 years ago
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/16.0 Firefox/16.0a1
Build ID: 20120615040201

Steps to reproduce:

I see these sporadically, have no idea where they're from or what they refer to.
Reporter

Updated

7 years ago
OS: Windows XP → Android
Hardware: x86 → ARM
Screenshot? STR?
Reporter

Comment 2

7 years ago
They're random, so no STRs or screenshots sadly. I was hoping that we could trace what calls the string and work it out that way. I have a hunch it's caused by Add-Ons updating. In that case I think we should display a more specific message (Addons Updated) and perhaps even display something in the OS notification bar.
Could be this string?

http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/en-US/chrome/browser.properties#13

Which add-ons do you have installed? Maybe we can check the version history to see if there were a bunch of updates as of recent.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 4

7 years ago
I've got the dev version of Ad Block Plus installed but disabled. That does update a fair bit.
Summary: Random Installation Complete Popups → Background update of add-ons/addons should not show success notification/toast 'Installation complete'
Reporter

Comment 5

7 years ago
I vehemently disagree with the title change. It should show something, it should just simply be more informative. I also wouldn't be against us using croutons. https://github.com/keyboardsurfer/Crouton
Ok, changed the title to indicate that the toast requires action. The add-on developers can open tabs with information etc. on update and the desktop version doesn't show update notifications for add-ons by default, so disabling them by default is imho the way to go.
Summary: Background update of add-ons/addons should not show success notification/toast 'Installation complete' → Background update of add-ons/addons: Make success notification/toast 'Installation complete' more informative or remove it

Comment 7

7 years ago
I agree that the label should be more informative!

I was surfing on a website and as I saw the popup "Installation complete" I was very unsure what just happened. I even thought that the website might have installed some plugin / malware. The popup did not tell that it was an update ("installation" gives the user the feeling that it is something new, and not an update) nor it did tell which addon was updated.

You should change the messages to this:

<code>
alertAddonsDownloading=Downloading add-on %s
alertAddonsInstalling=Installing/Updating add-on %s
alertAddonsInstalled=Installation/Update of %s complete. Restart required.
alertAddonsInstalledNoRestart=Installation/Update of %s complete
</code>

where %s is the name of the add-on.

You could even split up the messages and divide between "Installation" and "Update".
Assignee

Comment 8

6 years ago
Posted patch bug766359 (obsolete) — Splinter Review
Spotted this a few times and thought malware was involved also !

Quick search turned up this old bug report ... how about this as a first thought?

(Not sure who to ask to review)
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #783606 - Flags: feedback?(aaron.train)
Assignee

Comment 9

6 years ago
(ooops - changed the toast from short to long for testing and forgot to change it back - I'll fix that next time around)
Comment on attachment 783606 [details] [diff] [review]
bug766359

Looks fine to me. I would imagine it would be better to show the add-on name for reference in the toast for clarity as to exactly what got updated, but that's not my call. Also, did you test this by installing an older version of an add-on on AMO?
Attachment #783606 - Flags: feedback?(aaron.train) → feedback+
Assignee

Comment 11

6 years ago
Heh - no, I didn't go that far yet. I shortcut tested by installing an addon on getting the |Installation complete| toast, then re-installed it on top of itself and got the new |Installed add-on updated| complete message.

But I do have an old version of AdBlock set aside and I can test that. (I haven't yet figured out what triggers auto add-on updates.)
Ah you can download and install older versions off of AMO for any add-on

https://addons.mozilla.org/en-US/android/addon/adblock-plus/versions/
Assignee

Comment 13

6 years ago
DOH! X-D yep... as usual I'm working too hard !
Assignee

Comment 14

6 years ago
Nice ... installed old version of Adblock, set |extensions.autoupdate.interval| to 60 seconds, waited and received |Installed add-on updated| message
Summary: Background update of add-ons/addons: Make success notification/toast 'Installation complete' more informative or remove it → Background update of addons: Make success notification/toast 'Installation complete' more informative or remove it
Reporter

Comment 15

6 years ago
(In reply to Aaron Train [:aaronmt] from comment #10)
> Comment on attachment 783606 [details] [diff] [review]
> bug766359
> 
> Looks fine to me. I would imagine it would be better to show the add-on name
> for reference in the toast for clarity as to exactly what got updated, but
> that's not my call. Also, did you test this by installing an older version
> of an add-on on AMO?

Setting needinfo from Ian.
Flags: needinfo?(ibarlow)
Assignee

Comment 16

6 years ago
Posted patch bug766359 (obsolete) — Splinter Review
This patch will display |AddonFoo Installation complete| and |AddonFoo Update complete| messages.
Attachment #783606 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #783960 - Attachment is patch: true
Are toasts really how we want to be doing this? 

I wonder if using our Android notification tray UI would be more appropriate / informative...
Flags: needinfo?(ibarlow)
Assignee

Comment 18

6 years ago
Posted patch bug766359 (obsolete) — Splinter Review
Ok, I'm adding margaret as reviewer because, well, she needs stuff to do.

This version of the patch adds a discrete message to better inform users when background updates have occurred ... and adds the name of the addon to the Installation Complete and Update Complete messages ... 

Switching to the Android notification tray UI could be considered ... though we also use toasts in this user interaction for |Downloading add-on| and |Installation failed| and would have to work those into it ...

Same bug / Follow-on bug?
Attachment #783960 - Attachment is obsolete: true
Attachment #783993 - Flags: review?(margaret.leibovic)
Reporter

Comment 19

6 years ago
(In reply to Ian Barlow (:ibarlow) from comment #17)
> Are toasts really how we want to be doing this? 
> 
> I wonder if using our Android notification tray UI would be more appropriate
> / informative...

That's far too global and ultimately unnecessary notification tray spam. In an ideal world we'd use Crutons (https://github.com/keyboardsurfer/Crouton) but that's outside of the scope of this bug. Toasts are definitely the lesser of the evils here.
I don't really agree with that, but I'll let you guys run with it since you seem to have a pretty clear idea of what you want to do here.

Please provide screenshots of whatever solution you want to proceed with.
Comment on attachment 783993 [details] [diff] [review]
bug766359


>diff --git a/mobile/android/locales/en-US/chrome/browser.properties b/mobile/android/locales/en-US/chrome/browser.properties

>+alertAddonsUpdatedNoRestart=%S Update complete
>+alertAddonsInstalledNoRestart2=%S Installation complete

With the "%S" in front, the "Update" and "Installation" should be lowercase:

"Finkletron3000 update complete"

Although, as I read that, it doesn't flow off the tongue (and it has nothing to do with 'Finkletron3000')

Maybe we could flip it:

"Update complete: Finkletron3000"

Hmm, still not a winner. More thinking might be needed.
(In reply to Ian Barlow (:ibarlow) from comment #17)
> Are toasts really how we want to be doing this? 
> 
> I wonder if using our Android notification tray UI would be more appropriate
> / informative...

I kinda want to beg you to not spam the system tray area. We used to put add-on messages up there and it felt wrong. I know that Google Play spams the hell out of the system tray when updating apps. I grind my teeth over that too.
Reporter

Comment 23

6 years ago
Given the global aspect of the toasts, can I suggest we go with something befitting their nature:

"CHANNEL* Add-On EXTENTION_NAME Updated"

*Firefox/Firefox Beta/Aurora/Nightly

It's informative for the user in that it tells them both the add-on and app names, which is helpful for when Fennec announces the completion of an update after you've left for another application. Plus as an added bonus for power users, they're able to differentiate between installation add-on updates.
(In reply to Mark Finkle (:mfinkle) from comment #22)
> (In reply to Ian Barlow (:ibarlow) from comment #17)
> > Are toasts really how we want to be doing this? 
> > 
> > I wonder if using our Android notification tray UI would be more appropriate
> > / informative...
> 
> I kinda want to beg you to not spam the system tray area. We used to put
> add-on messages up there and it felt wrong. I know that Google Play spams
> the hell out of the system tray when updating apps. I grind my teeth over
> that too.

Fair enough. I guess I am ok with the toast approach (at least getting the strings to make more sense) -- it's just that seeing a toast telling me something that has nothing to do with my current context feels a little odd, too. Generally speaking it's not really what that kind of UI is for...

Paul's 'crouton' idea might be interesting as something to explore down the road.
The flip side to all this is, if add-ons are set to automatically update, do we really need to tell our users when they do? Is it useful at all to tell them that?
Assignee

Comment 26

6 years ago
I like the previous patch, but ibarlows' idea (attached) is direct and simple !
Reporter

Comment 27

6 years ago
I'd prefer it if we kept the auto update toast given that we neither provide information to give the last update time nor do we provide the option to disable auto updates.
Comment on attachment 783993 [details] [diff] [review]
bug766359

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

This approach is good if this is the kind of change we want to make, but it sounds like we still need to sort out exactly what that is.
Attachment #783993 - Flags: review?(margaret.leibovic) → feedback+
(In reply to Ian Barlow (:ibarlow) from comment #25)
> The flip side to all this is, if add-ons are set to automatically update, do
> we really need to tell our users when they do? Is it useful at all to tell
> them that?

You read my mind. I'd rather skip the toast. What purpose does it serve to the user? None that I can think of. I think we should consider it a background process. A silent background process.
We plan on updating the Add-on UI soon. We could move information about 'last update' into the new UI. I think we shouldn't add anything at this point.
I was mid-aired, but here is what I was going to say...

(In reply to Paul [sabret00the] from comment #27)
> I'd prefer it if we kept the auto update toast given that we neither provide
> information to give the last update time nor do we provide the option to
> disable auto updates.

I assume we should be able to get the last update time, and if so, we could stick that information somewhere in the add-on detail page in the add-on manager.

Also, there must be APIs we could use to disable auto updates, since that's exposed as part of the desktop add-on manager UI. We'd just have to dig into the AddonManager code to figure that out.

These changes both feel out of scope for this bug, but I don't want us to make design decisions using these other things as constants, since we can definitely change them.

I agree with mfinkle that we should probably just keep these things in mind when we update the Add-on UI, rather than try to do them with the current UI.
Assignee

Comment 32

6 years ago
Comment on attachment 784378 [details] [diff] [review]
bug766359 - drop auto update toast

So we decided to go with this patch then, and just do the update silently..
Attachment #784378 - Flags: review?(margaret.leibovic)
Comment on attachment 784378 [details] [diff] [review]
bug766359 - drop auto update toast

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

::: mobile/android/chrome/content/browser.js
@@ +5485,5 @@
>  
>      if (needsRestart) {
>        this.showRestartPrompt();
>      } else {
> +      if (!aInstall.existingAddon || !AddonManager.shouldAutoUpdate(aInstall.existingAddon)) {

You can just turn the else above into an else if.

Also, add a comment explaining this logic.
Attachment #784378 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 783993 [details] [diff] [review]
bug766359

Obsoleting this patch, so that I don't get confused looking at the attachments :)
Attachment #783993 - Attachment is obsolete: true
(In reply to Mark Capella [:capella] from comment #35)
> (annoyance be-gone!)
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f115fb574d

Oops, you didn't address my else if comment... but it's not a big deal, not worth re-landing.
https://hg.mozilla.org/mozilla-central/rev/b1f115fb574d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Please can bugs changing these parts of the tree land on fx-team in the future to help balance load & avoid conflicts (for more info see Gavin's newsgroup post: https://mail.mozilla.org/pipermail/firefox-dev/2013-July/000618.html). Thank you :-)
Duplicate of this bug: 926335
You need to log in before you can comment on or make changes to this bug.