970203 - animate webapp update "checking" and "downloading" notification icons

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

The "checking" and "downloading" notification icons that the webapp manager displays while checking for and downloading updates are inanimate, which is inconsistent with the icons that the native Android updater displays and makes it hard to tell that the activity is ongoing.  Thus those icons should be animated.

Note that Fennec's UpdateService class (which updates Fennec itself) uses android.R.drawable.stat_sys_download.  But WebappManager is implemented in JavaScript and specifies icons via drawable: URLs, which don't support locating native Android icons.

(And in any case docs around the internet recommend against using native Android icons because they can change or go away on different versions of Android.)

In bug 934760, comment 30, wesj says Fennec is "moving to use the system service," so perhaps we could do the same.  Or make our own animated icons, perhaps by copying Android drawables into Fennec and then creating an animation from them as described in this StackOverflow thread <http://stackoverflow.com/questions/8094079/is-there-a-way-to-access-standard-animated-drawables-in-android>.

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: Animate webapp update checking and downloading icons

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8396403 [details] [diff] [review]
Animate webapp update checking and downloading icons

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

Looks nice!  And it works pretty well, except:

1. When no updates are available, the animated icon continues to be animated after Fennec updates the notification to inform the user that no updates are available.  The icon should stop animating at that point, since Fennec is no longer waiting for a response from the service.

2. When downloading a file, the icon continues to animate after the download is complete and Fennec has updated the notification to say "Download complete".  The icon should stop animating at that point.

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: Animate webapp update checking and downloading icons

Icon animations should now work as expected

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8402714 [details] [diff] [review]
Animate webapp update checking and downloading icons

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> 2. When downloading a file, the icon continues to animate after the download
> is complete and Fennec has updated the notification to say "Download
> complete".  The icon should stop animating at that point.

This is still happening.

::: mobile/android/app/mobile.js
@@ +836,5 @@
>  // The URL of the service that checks for updates.
>  // To test updates, set this to http://apk-update-checker.paas.allizom.org,
>  // which is a test server that always reports all apps as having updates.
> +//pref("browser.webapps.updateCheckUrl", "https://controller.apk.firefox.com/app_updates");
> +pref("browser.webapps.updateCheckUrl", "http://apk-update-checker.paas.allizom.org");

This needs to be changed back to the production URL.

Comment 5

[Martyn Haigh (:mhaigh)]

Myk:  I'm not seeing the same behaviour as you.  This is what I see:
1, Rocket ship animating up whilst checking for updated
2, Non animating arrow to indicate there are updates to download
3, upon clicking on the notification, a animated arrow notification shows to indicate that we're downloading the updates
4, once updates are downloaded a non animating rocket ship shows to indicate that the downloading is complete
5, clicking on the rocket ship opens up the install prompts for the downloaded apps and the notification is removed

https://dl.dropboxusercontent.com/u/7163922/iconanimation.mp4

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(In reply to Martyn Haigh (:mhaigh) from comment #5)
> Myk:  I'm not seeing the same behaviour as you.  This is what I see:
  …

I see all that too!  But when downloading a *non-app* file, f.e. a PDF, the icon stays animated.  For example, go to the following URL:

  http://www.irs.gov/pub/irs-pdf/fw4.pdf

It should trigger a download, and the icon should remain animated until the download is complete, at which time it should become inanimate.  But instead it stays animated.

Comment 7

[Martyn Haigh (:mhaigh)]

Attachment: Animate webapp update checking and downloading icons

Fixed the issue of the animated icon for normal downloads.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8411785 [details] [diff] [review]
Animate webapp update checking and downloading icons

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

It looks like you removed the download animation entirely, which is fine.  That's a separate issue in any case, and we can tackle it in a different bug.

::: mobile/android/base/resources/drawable/alert_app_animation.xml
@@ +1,2 @@
> +<animation-list xmlns:android="http://schemas.android.com/apk/res/android"
> +    android:oneshot="false">

Nit: here and in the other case, line this up with the attribute on the line above it:

<animation-list xmlns:android="http://schemas.android.com/apk/res/android"
                android:oneshot="false">

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 8411785 [details] [diff] [review]
Animate webapp update checking and downloading icons

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

::: mobile/android/base/resources/drawable/alert_app_animation.xml
@@ +1,1 @@
> +<animation-list xmlns:android="http://schemas.android.com/apk/res/android"

Make sure there's a license header in here. If we're copying this file exactly from Android, feel free to leave their Apache license in place. If you've written your own, just use ours.

::: mobile/android/base/resources/drawable/alert_download_animation.xml
@@ +1,1 @@
> +<animation-list xmlns:android="http://schemas.android.com/apk/res/android"

Same license info

Comment 10

[Martyn Haigh (:mhaigh)]

Attachment: Animate webapp update checking and downloading icons

Added license info to xml files

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 8416562 [details] [diff] [review]
Animate webapp update checking and downloading icons

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

We'd usually put it at the top (under an <xml> declaration. Like this:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/arrow_popup.xml

Sorry for the pain :( I only really care about this in this case because I wanted to document if we were using some Apache stuff from Android.

Comment 12

[Martyn Haigh (:mhaigh)]

Attachment: Animate webapp update checking and downloading icons

Moved the license info to top of the xml files

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8416574 [details] [diff] [review]
Animate webapp update checking and downloading icons

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

::: mobile/android/base/resources/drawable/alert_app_animation.xml
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<animation-list xmlns:android="http://schemas.android.com/apk/res/android"
> +                android:oneshot="false">

Nit: it would be better to follow the exact style used by the file WesJ referenced:

<?xml version="1.0" encoding="utf-8"?>
<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - License, v. 2.0. If a copy of the MPL was not distributed with this
   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

<animation-list xmlns:android="http://schemas.android.com/apk/res/android"
                android:oneshot="false">

@@ +10,5 @@
> +    <item android:drawable="@drawable/alert_app_animation_4" android:duration="150" />
> +    <item android:drawable="@drawable/alert_app_animation_5" android:duration="150" />
> +    <item android:drawable="@drawable/alert_app_animation_6" android:duration="150" />
> +    <item android:drawable="@drawable/alert_app_animation_7" android:duration="150" />
> +</animation-list>
\ No newline at end of file

Nit: add a line of whitespace between the last child element and the closing tag.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8416574 [details] [diff] [review]
Animate webapp update checking and downloading icons

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

::: mobile/android/base/resources/drawable/alert_download_animation.xml
@@ +9,5 @@
> +    <item android:drawable="@drawable/alert_download_animation_3" android:duration="150" />
> +    <item android:drawable="@drawable/alert_download_animation_4" android:duration="150" />
> +    <item android:drawable="@drawable/alert_download_animation_5" android:duration="150" />
> +    <item android:drawable="@drawable/alert_download_animation_6" android:duration="150" />
> +</animation-list>
\ No newline at end of file

Erm, and add newlines to the ends of the files!

Comment 15

[Martyn Haigh (:mhaigh)]

Attachment: Animate webapp update checking and downloading icons

Corrected license info and added newlines as request

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8416579 [details] [diff] [review]
Animate webapp update checking and downloading icons

The two XML files are still missing trailing newlines.

Comment 17

[Martyn Haigh (:mhaigh)]

Attachment: Animate webapp update checking and downloading icons

A thousand apologies - it seems that somewhere along my dev pipe something is automatically removing a newline at the end of a file.  I've fixed this by cunningly adding two newlines to the end of the file which now gives me one newline at the end of the file in my patch.  

I'm still confused as to what is doing this, and why!

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8418055 [details] [diff] [review]
Animate webapp update checking and downloading icons

(In reply to Martyn Haigh (:mhaigh) from comment #17)
> A thousand apologies - it seems that somewhere along my dev pipe something
> is automatically removing a newline at the end of a file.  I've fixed this
> by cunningly adding two newlines to the end of the file which now gives me
> one newline at the end of the file in my patch.

It looks like you now have an extra newline at the end of the file!

05-08 16:06 > git apply ~/0001-Animate-webapp-update-checking-and-downloading-icons.patch
/Users/myk/0001-Animate-webapp-update-checking-and-downloading-icons.patch:667: new blank line at EOF.
+
/Users/myk/0001-Animate-webapp-update-checking-and-downloading-icons.patch:690: new blank line at EOF.
+
warning: 2 lines add whitespace errors.


It's visible in the patch as a trailing "+" on a line by itself:

diff --git a/mobile/android/base/resources/drawable/alert_app_animation.xml b/mobile/android/base/resources/drawable/alert_app_animation.xml
…
+</animation-list>
+
diff --git a/mobile/android/base/resources/drawable/alert_download_animation.xml b/mobile/android/base/resources/drawable/alert_download_animation.xml
…
+</animation-list>
+

Comment 19

[Martyn Haigh (:mhaigh)]

Attachment: Animate webapp update checking and downloading icons

If this isn't correct I'll eat a shoe.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8421782 [details] [diff] [review]
Animate webapp update checking and downloading icons

It's correct!  And wesj already reviewed this patch, so it's ready for commission!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/a7d6986abdc4

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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