592088 - support progress indicators in android status bar notifications

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

We want this for download notifications and weave sync progress, perhaps addons as well.

Comment 2

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

I wanted to find a UI guideline or widget API for progressmeters in notifications. I haven't found a UI guideline.

As near as I can tell, you can put a progressbar in a notification expanded view:
http://developer.android.com/guide/topics/ui/notifiers/notifications.html#CustomExpandedView

Example here:
http://stackoverflow.com/questions/2689729/progress-bar-in-notification-bar/2725220#2725220

However, there doesn't appear to be any specific API for updating the progressmeter from external code. From working examples of progressmeters in Android notifications (Browser for example) we only really need to send the progress so the textual percentage and the meter could be updated.

We could add params to the current nsIAlertsService.showNotificationAlert(...) method, but I think that isn't right. However, I do think we can add the functionality without adding a new method.

I am thinking about the nsIObserver passed to showNotificationAlert. Currently, the listener is called when the user clicks the notification or it closes.

A few ideas come to mind. All of the ideas involve using a new listener topic, "alertprogress":
1. The nsIObserver listener is called periodically with the new topic. When called, the listener would return the current progress via the data argument.
2. The nsIObserver listener is called right away with the new topic. It also passes in one of the current progress interfaces supported by Mozilla as the subject argument: nsIWebProgressListener or nsIDownloadProgressListener.
3. The nsIObserver listener is called right away with the new topic. It also passes in a new, simpler, progress interface, specifically made for alerts service as the subject argument: nsIAlertsProgressListener.

Thoughts:
#1 is simple but sucks because it's a polling method. Not a good idea for a progress system. I am embarrassed for even suggesting it.
#2 is OK but those interfaces are way overkill for alerts. Too many extra methods (we only want onProgress) and many of the onProgress arguments are unneeded.
#3 is simple and clean but requires adding a new interface to the alerts service IDL. I can live with that.

I like #3 and the flow would work like this:
1. The front-end code defines a nsIObserver listener. We do this in several places already. This listener would also have access to the progress sensitive data. Don't worry about how it's done, just believe me that it can be done.
2. The front-end code calls showAlertNotification, passing the listener.
3. The nsIAlertService implementation would immediately check to nsIObserver support and call the observe method with the "alertprogress" topic and the nsIAlertsProgressListener implementation subject. Only the Android alerts service impl would support it at this time.
4. The front-end code catches the "alertprogress" topic and holds onto the nsIAlertsProgressListener. If the alerts service impl doesn't support progress, the code still works fine.
5. As the front-end is updated about the progress (download, addon install, update, whatever), it will call the nsIAlertsProgressListener method which will callback into the Android impl and that can update the progressbar in the expanded view however it needs to.

How does that sound? Minimal changes to existing interfaces and safe fallback for non Android implementations.

Comment 3

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

Oh I forgot my simple nsIAlertsProgressListener interface:

interface nsIAlertsProgressListener {
 void onProgress(in long long aCurProgress, in long long aMaxProgress, [optional] AString cookie, [optional] AString name);
};

I am suggesting a similar format to nsIWebProgressListener and nsIDownloadProgressListener, which pass the current and total. I am also sending the cookie and name along to help identify which alert the progress is associated with, in the case of multiple download alerts.

Comment 4

[Alex Pakhotin (:alexp)]

Looks a bit too complicated to me.

I've been thinking about something like this, very similar to nsIAlertsService.showAlertNotification:

void showAlertProgressNotification(in AString id,
                                   in AString imageUrl,
                                   in AString title,
                                   in AString text,
                                   [optional] in boolean textClickable,
                                   [optional] in AString cookie,
                                   [optional] in nsIObserver alertListener);

void updateAlertProgressNotification(in AString id,
                                     in AString imageUrl,
                                     in AString title,
                                     in AString text,
                                     in long progress,
                                     in long maxProgress);

The id parameter in both functions identifies the notification. We will need it anyway, as its hash value will be used as a notification ID required by Android (the same way as it works now for the name parameter in showAlertNotification).

A listener mechanism is already used in the update-service-progress-display machinery (nsIApplicationUpdateService.addDownloadListener). I believe introducing another one will complicate things too much. I'd consider this as just a bridge to the system API, and a couple of "push" functions using a "handle" to identify the notification object seem to work fine.

I've got a working prototype, which uses this approach to display the update download progress. Will prepare a WIP patch tomorrow.

Comment 5

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

(In reply to comment #4)
> Looks a bit too complicated to me.
> 
> I've been thinking about something like this, very similar to
> nsIAlertsService.showAlertNotification:
> 
> void showAlertProgressNotification(in AString id,
>                                    in AString imageUrl,
>                                    in AString title,
>                                    in AString text,
>                                    [optional] in boolean textClickable,
>                                    [optional] in AString cookie,
>                                    [optional] in nsIObserver alertListener);
> 
> void updateAlertProgressNotification(in AString id,
>                                      in AString imageUrl,
>                                      in AString title,
>                                      in AString text,
>                                      in long progress,
>                                      in long maxProgress);

This is not my favorite design, but it is simple. Some feedback:
* The showAlertProgressNotification is almost showAlertNotification. The params are almost identical, where "almost" is a problem imo. It could be confusing.
* updateAlertProgressNotification allows you to change all properties of the alert. Is this needed?

> The id parameter in both functions identifies the notification. We will need it
> anyway, as its hash value will be used as a notification ID required by Android
> (the same way as it works now for the name parameter in showAlertNotification).

Then just use "name" again. The more the APIs are the same, the better. Although, I don't like the complete duplication.

> A listener mechanism is already used in the update-service-progress-display
> machinery (nsIApplicationUpdateService.addDownloadListener). I believe
> introducing another one will complicate things too much. I'd consider this as
> just a bridge to the system API, and a couple of "push" functions using a
> "handle" to identify the notification object seem to work fine.

I'm not sure how the update service download listener complicates things. Personally, I like the idea that the alerts progress feature works like other progress features in Mozilla.

I haven't seen how these two methods are added to the platform, but I hope you don't add them to nsIAlertsService and make all other alert service implementations and just stub them out. One of the benefits of my proposal was the zero-pain fallback for impls that do not support progress features.

Comment 6

[Alex Pakhotin (:alexp)]

(In reply to comment #5)
> This is not my favorite design, but it is simple.

Yes, I understand it has its drawbacks, but I like the simplicity.

> Some feedback:
> * The showAlertProgressNotification is almost showAlertNotification. The params
> are almost identical, where "almost" is a problem imo. It could be confusing.

I tried to make it similar to the showAlertNotification, as the idea was to inherit a new interface from the nsIAlertsService.
At the last moment I changed the parameters, so they became the same as for the showAlertNotification, so now it definitely makes sense to rename and rearrange them the same way to avoid confusion.

> * updateAlertProgressNotification allows you to change all properties of the
> alert. Is this needed?

Not sure yet. Probably I'll get rid of some of them. For example imageUrl and title should probably stay the same, so they are not needed here.

> > The id parameter in both functions identifies the notification. We will need it
> > anyway, as its hash value will be used as a notification ID required by Android
> > (the same way as it works now for the name parameter in showAlertNotification).
> 
> Then just use "name" again. The more the APIs are the same, the better.

Yes, I'm not sure about renaming it to "id". Just wanted to increase the importance of this parameter. By the way, as it's mandatory now, it cannot be at the end of the list anyway.

> Although, I don't like the complete duplication.

showAlertProgressNotification being the same as showAlertNotification? But they will actually be functionally almost the same, at least on Android.

> > A listener mechanism is already used in the update-service-progress-display
> > machinery (nsIApplicationUpdateService.addDownloadListener). I believe
> > introducing another one will complicate things too much. I'd consider this as
> > just a bridge to the system API, and a couple of "push" functions using a
> > "handle" to identify the notification object seem to work fine.
> 
> I'm not sure how the update service download listener complicates things.

Well, just too many listeners.

> Personally, I like the idea that the alerts progress feature works like other
> progress features in Mozilla.

Yes, I understand this. Just the whole idea of the calls back and forth with passing a listener through another listener sounds a bit over-complicated to me.

> I haven't seen how these two methods are added to the platform, but I hope you
> don't add them to nsIAlertsService and make all other alert service
> implementations and just stub them out.

In my prototype I do just add new methods to the existing interface for the simplicity, but I don't mean to leave it like this. As far as I remember we wanted to create a new interface based on the nsIAlertsService.

> One of the benefits of my proposal was
> the zero-pain fallback for impls that do not support progress features.

Yes, I understand - I'll think more about this.

Comment 7

[Alex Pakhotin (:alexp)]

Attachment: [WIP] Progress notifications

- Added two methods to nsIAlertsService (to be moved into a separate interface);
- Implemented a custom view with progress bar for Android status bar notifications.

Comment 8

[Alex Pakhotin (:alexp)]

Attachment: [WIP] Update download progress notification (mobile)

Implemented update download listener in the UpdatePrompt using new nsIAlertsService methods to display progress.

Comment 9

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

Comment on attachment 477388 [details] [diff] [review]
[WIP] Progress notifications

OK, I see why you want showAlertProgressNotification. You make an entirely new type of notification in this method. I think that is an implementation detail that should not manifest itself as an almost duplicate method in the nsIAlertsService.

That was the main reason my proposal used the "alertprogress" observer notif. So the alerts impl could decide which type of notification to make: plain or progress.

I think I can simplify my proposal, without adding cruft to the nsIAlertsService. Basically, we could take the spirit of your proposal, but remove the cruft I don't like:

1. Add a new interface to nsIAlertsService. You suggested doing the same, but this interface would not have xxx. It would be the listener interface I proposed:

interface nsIAlertsProgressListener {
 void onProgress(in long long aCurProgress, in long long aMaxProgress, AString aName);
};

2. Change the Android implementation to always make notification views that support progressbars, but just hide the widgets until and onProgress call is made.

The flow would them be:
1. The front-end code calls showAlertNotification, passing the name.
2. Android alerts service implementation would make and display the notification, with the progress controls hidden.
3. At some point the front-end code gets the nsIAlertsService and QI to nsIAlertsProgressListener. The front-end code calls onProgress, passing the progress data and the name passed in #1.
4. Android alerts service implementation would display the hidden progress controls and update them based on the progress data.

I think this is as simple as your approach. The only downside is the progress controls are created but may never be used. Honestly, even that could be fixed. You could create the controls the first time onProgress is called.

This proposal keeps the nsIAlertsService interface clean and adds progress support in a way that fits into Mozilla patterns, as well as makes supporting alerts progress for a platform completely optional - without needing to stub out methods.

Comment 10

[Alex Pakhotin (:alexp)]

Attachment: Progress notifications

- As agreed, added nsIAlertsProgressListener interface to nsIAlertsService.
- Made status bar notifications expanded view display a progress bar when nsIAlertsProgressListener.onProgress is called.

Comment 11

[Alex Pakhotin (:alexp)]

Attachment: Progress notifications - Android resources (m-b)

Android resources required for the progress notifications:
- Animated download icon (to be replaced by our own version later);
- Notification expanded view layout XML.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 478924 [details] [diff] [review]
Progress notifications - Android resources (m-b)

why not put these in the platform?

Comment 13

[Alex Pakhotin (:alexp)]

(In reply to comment #12)
> why not put these in the platform?

Actually I'm not sure what would be the best place for these images, and especially the layout XML. I followed the same way as with the previous alert icons, which were decided to be put in m-b/app/android/drawable.
What do you guys suggest?

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 478922 [details] [diff] [review]
Progress notifications


>+$(RES_DRAWABLE): $(subst res/,$(topsrcdir)/mobile/app/android/,$(RES_DRAWABLE))
> 	$(NSINSTALL) -D res/drawable
> 	cp $(topsrcdir)/mobile/app/android/drawable/* res/drawable/
> 
>-$(RES_DRAWABLE_HDPI):
>+$(RES_DRAWABLE_HDPI): $(subst res/,$(topsrcdir)/mobile/app/android/,$(RES_DRAWABLE_HDPI))
> 	$(NSINSTALL) -D res/drawable-hdpi
> 	cp $(topsrcdir)/mobile/app/android/drawable-hdpi/* res/drawable-hdpi/
> 
>-R.java: $(MOZ_APP_ICON) $(RES_DRAWABLE) $(RES_DRAWABLE_HDPI)
>+$(RES_LAYOUT): $(subst res/,$(topsrcdir)/mobile/app/android/,$(RES_LAYOUT))
>+	$(NSINSTALL) -D res/layout
>+	cp $(topsrcdir)/mobile/app/android/layout/* res/layout/
>+

platform code shouldn't be referencing things from mobile-browser. Also, I think at least a default set of these icons should be in the platform.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #13)
> (In reply to comment #12)
> > why not put these in the platform?
> 
> Actually I'm not sure what would be the best place for these images, and
> especially the layout XML. I followed the same way as with the previous alert
> icons, which were decided to be put in m-b/app/android/drawable.
> What do you guys suggest?

The previous generic alert icon was the fennec icon, which makes sense to have in mobile-browser. I think it would be best to have a set of platform defaults that the application can override.

Comment 16

[Alex Pakhotin (:alexp)]

(In reply to comment #15)
> > I followed the same way as with the previous alert
> > icons, which were decided to be put in m-b/app/android/drawable.
> 
> The previous generic alert icon was the fennec icon, which makes sense to have
> in mobile-browser.

I meant alertaddons.png and alertdownloads.png.

> I think it would be best to have a set of platform defaults
> that the application can override.

I agree - never liked that dependency on m-b.
I'll move all the resources to a new embedding/android/resources dir.

Comment 17

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

I think the application specific packaging code should not be located in platform! The icons and images are part of the application theme.

At the same time, I agree that platform code should not be aware of application source locations.

Comment 18

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

Alex - What's the use case for passing the "text" parameter to the onProgress method?

>+    void onProgress(in AString text,

Just curious. I don't think we want the user to pass the text in all the time, especially if it never needs to change.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #17)
> I think the application specific packaging code should not be located in
> platform! The icons and images are part of the application theme.
> 
> At the same time, I agree that platform code should not be aware of application
> source locations.

The platform should provide the bare minimum needed for a simple hello XUL can show a progress notification. It should also allow for the application to provide its own icons and themes.

As I said on irc last night, layout.xml and a set of default icons should be in the platform.

Comment 20

[Michael Wu [:mwu]]

(In reply to comment #19)
> As I said on irc last night, layout.xml and a set of default icons should be in
> the platform.

AFAIK we already have a default icon. Why are we adding more to the platform?

Comment 21

[Alex Pakhotin (:alexp)]

(In reply to comment #20)
> AFAIK we already have a default icon. Why are we adding more to the platform?

The problem is that if we want to show some icon in the notification (and we already need three: for downloads, add-ons, and a new animated one for download progress), that icon must be in the resources (at least I don't know another way of providing one).

We can use the app icon for a bare minimum implementation, but any customization would involve not only copying the .png file to the res dir before compiling and packaging, but also changing the Java code to make it use a new resource ID.

I'm thinking about creating some kind of a resource container in form of a Java class returning the icon ID based on the icon name/URI:
  int getIcon(String iconUri)
That class would go through some kind of a preprocessing like the .in files we have. Default implementation of the method would just return the app icon ID for all the requests, but if we have some other icons, the pre-processor would add them to the list, and the compiled version of the class will be returning those by request.

Comment 22

[Alex Pakhotin (:alexp)]

(In reply to comment #18)
> Alex - What's the use case for passing the "text" parameter to the onProgress
> method?
> 
> >+    void onProgress(in AString text,
> 
> Just curious. I don't think we want the user to pass the text in all the time,
> especially if it never needs to change.

I answered to this question in the bug 599231, which uses this new API, but I'll copy that response here as well.

What exactly is displayed here is actually up to FE/UX. But I believe its usage
should not be hard-coded in the AlertService. It could be percentage, or could
be "n out of N bytes", and be translated, or both. In some cases it could be
even just some text, which changes periodically, for example for the Sync it
could show "Syncing Bookmarks..." first, then "Syncing Tabs...", then "Syncing
history...".

I do see this parameter to be really useful for such a feature.

Comment 23

[Alex Pakhotin (:alexp)]

Attachment: Progress notifications v2

- Made the feature usable without extra icons. When MOZ_ANDROID_RESOURCES_DIRECTORY is defined (see bug 600957), the icons from that directory are used, otherwise the app icon is used for notifications.
- Moved layout xml to the platform.

Comment 24

[Alex Pakhotin (:alexp)]

Attachment: Download progress draft icons (m-b)

These icons will be needed when bug 600957 lands. Without them the feature will be broken.

Comment 25

[Alex Pakhotin (:alexp)]

Attachment: Progress notifications v3

- Using another way of converting string icon names to integer resource IDs;
- Changed the order of parameters in nsIAlertsProgressListener.onProgress.

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 480027 [details] [diff] [review]
Progress notifications v3

java code looks good

In the makefile, the app should set a variable containing a space delineated list of icons, not a path to a directory of icons. i.e. the icons should be able to come form multiple directories.

Also, since alertaddons and alertdownloads shouldn't be hardcoded anymore. They should be provided by the app. The only icon that should be included by default is the app icon (R.drawable.icon), which is our fallback. Everything else needs to be explicitly included by the app

Finally, use CallStaticVoidMethod, not CallStaticVoidMethodA

Comment 27

[Alex Pakhotin (:alexp)]

Attachment: Progress notifications v4

Fixed as per review comments.

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

http://hg.mozilla.org/mozilla-central/rev/a3eadf884f81