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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul [pwd]]

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.

Comment 1

[Aaron Train [:aaronmt]]

Screenshot? STR?

Comment 2

[Paul [pwd]]

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.

Comment 3

[Aaron Train [:aaronmt]]

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.

Comment 4

[Paul [pwd]]

I've got the dev version of Ad Block Plus installed but disabled. That does update a fair bit.

Comment 5

[Paul [pwd]]

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

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 7

[Daniel Marschall]

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

Comment 8

[Mark Capella [:capella]]

Attachment: bug766359

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)

Comment 9

[Mark Capella [:capella]]

(ooops - changed the toast from short to long for testing and forgot to change it back - I'll fix that next time around)

Comment 10

[Aaron Train [:aaronmt]]

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?

Comment 11

[Mark Capella [:capella]]

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

Comment 12

[Aaron Train [:aaronmt]]

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/

Comment 13

[Mark Capella [:capella]]

DOH! X-D yep... as usual I'm working too hard !

Comment 14

[Mark Capella [:capella]]

Nice ... installed old version of Adblock, set |extensions.autoupdate.interval| to 60 seconds, waited and received |Installed add-on updated| message

Comment 15

[Paul [pwd]]

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

Comment 16

[Mark Capella [:capella]]

Attachment: bug766359

This patch will display |AddonFoo Installation complete| and |AddonFoo Update complete| messages.

Comment 17

[Ian Barlow (:ibarlow)]

Are toasts really how we want to be doing this? 

I wonder if using our Android notification tray UI would be more appropriate / informative...

Comment 18

[Mark Capella [:capella]]

Attachment: bug766359

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?

Comment 19

[Paul [pwd]]

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

Comment 20

[Ian Barlow (:ibarlow)]

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 21

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 23

[Paul [pwd]]

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.

Comment 24

[Ian Barlow (:ibarlow)]

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

Comment 25

[Ian Barlow (:ibarlow)]

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?

Comment 26

[Mark Capella [:capella]]

Attachment: bug766359 - drop auto update toast

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

Comment 27

[Paul [pwd]]

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 28

[:Margaret Leibovic]

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.

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 30

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 31

[:Margaret Leibovic]

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.

Comment 32

[Mark Capella [:capella]]

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

Comment 33

[: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.

Comment 34

[:Margaret Leibovic]

Comment on attachment 783993 [details] [diff] [review]
bug766359

Obsoleting this patch, so that I don't get confused looking at the attachments :)

Comment 35

[Mark Capella [:capella]]

(annoyance be-gone!)

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f115fb574d

Comment 36

[:Margaret Leibovic]

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

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b1f115fb574d

Comment 38

[Ed Morley [:emorley]]

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 :-)