Last Comment Bug 695461 - Download notifications
: Download notifications
Status: VERIFIED FIXED
[birch] [ux needed][QA!][testday-2011...
: feature
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: ---
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on:
Blocks: 696912 695178 696911
  Show dependency treegraph
 
Reported: 2011-10-18 12:49 PDT by Johnathan Nightingale [:johnath]
Modified: 2012-01-10 11:35 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
[WIP] Patch (28.52 KB, patch)
2011-10-19 11:25 PDT, Alex Pakhotin (:alexp)
mark.finkle: feedback+
Details | Diff | Review
WIP patch v2 (35.58 KB, patch)
2011-10-21 12:11 PDT, :Margaret Leibovic
no flags Details | Diff | Review
WIP patch v3 (35.99 KB, patch)
2011-10-21 13:47 PDT, :Margaret Leibovic
mark.finkle: feedback+
Details | Diff | Review
patch (using prompt service) (32.56 KB, patch)
2011-10-24 14:14 PDT, :Margaret Leibovic
mark.finkle: review+
Details | Diff | Review
Patch avoiding styles (2.94 KB, patch)
2011-10-24 17:26 PDT, Sriram Ramasubramanian [:sriram]
alex.mozilla: review+
Details | Diff | Review

Description Johnathan Nightingale [:johnath] 2011-10-18 12:49:25 PDT
+++ This bug was initially created as a clone of Bug #695178 +++

Discussed during the October 2011 work week - if we can't get a complete download manager implemented (bug 695178), it would be nice to at least use the Android notification system for downloads in progress or completed.
Comment 1 Gian-Carlo Pascutto [:gcp] 2011-10-18 14:27:48 PDT
I'm moving the deps around here. It makes sense to implement the simple thing here first, which we know will work, and then attempt to make the complete download manager afterwards.
Comment 2 Alex Pakhotin (:alexp) 2011-10-19 11:25:51 PDT
Created attachment 568125 [details] [diff] [review]
[WIP] Patch

A work-in-progress patch.

In general the download notifications work now.
Just some things to improve:
- Cancel download in progress by clicking on the notification. Do not open an incomplete file.
- Hide the notification when downloaded file opens on completion (or just do not open the file automatically)
- Open downloaded file by clicking on the notification when Fennec is not running (make sure the '-alert xxx' command line argument is handled properly on startup)
Comment 3 :Margaret Leibovic 2011-10-19 13:12:13 PDT
(In reply to Alex Pakhotin (:alexp) from comment #2)

> Just some things to improve:
> - Cancel download in progress by clicking on the notification. Do not open
> an incomplete file.

I don't think we should cancel the download in progress - that seems unexpected. Maybe just do nothing and make sure we don't show an error message? I'm not sure what the user expectations are here.
Comment 4 Alex Pakhotin (:alexp) 2011-10-19 13:29:30 PDT
(In reply to Margaret Leibovic [:margaret] from comment #3)
> (In reply to Alex Pakhotin (:alexp) from comment #2)
> 
> I don't think we should cancel the download in progress - that seems
> unexpected.

Agree. I just meant the current behavior needs to be fixed.

> Maybe just do nothing and make sure we don't show an error
> message? I'm not sure what the user expectations are here.

That's why we have "[ux needed]" here. Need to decide what to do for now until we will have a complete download manager.
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-20 18:28:04 PDT
Comment on attachment 568125 [details] [diff] [review]
[WIP] Patch

* You can assume _always_ ANDROID, so cleanup the #ifdef ANDROID
* I agree, we need to find a good solution for an in progress file download.

Generally, this is a good direction.
Comment 6 :Margaret Leibovic 2011-10-20 20:05:19 PDT
(In reply to Mark Finkle (:mfinkle) from comment #5)
> * You can assume _always_ ANDROID, so cleanup the #ifdef ANDROID

Excellent, I already did that! :)

> * I agree, we need to find a good solution for an in progress file download.

Today I made a dialog that appears when you click on an in-progress download notification, and it gives you the option to cancel the download if you want. I think this is a good compromise until we have a full download manager, since users may accidentally start a large download on a slow or expensive network connection, so it would be really great to give them the option to cancel the download.

The patch is still pretty rough, but I'll upload it once I clean it up a bit more.
Comment 7 :Margaret Leibovic 2011-10-21 12:11:36 PDT
Created attachment 568732 [details] [diff] [review]
WIP patch v2

Here's the patch that adds a dialog to let the user cancel a download. The main problem I've found with it is that I should find a way to dismiss the dialog if the download has finished since the dialog was opened. I've been trying to read the Android docs, and I'm worried that there may be a better way to make this dialog, so I want to get feedback to see if this is the way to go.
Comment 8 :Margaret Leibovic 2011-10-21 13:47:48 PDT
Created attachment 568763 [details] [diff] [review]
WIP patch v3

This patch more closely follows what the Android docs say about alert dialogs, and it lets me dismiss the dialog when the download finishes. However, right now I'm just removing the dialog regardless of which download has finished, so that probably still needs some work. I also don't really like that the current download id is being stored separately from the dialog, but I don't think this should cause problems with multiple downloads, since there should only ever be one dialog showing at a time.
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-24 08:58:03 PDT
Comment on attachment 568763 [details] [diff] [review]
WIP patch v3

This is pretty good. How much more do you want to add before asking for review? Tracking multiple downloads?

Can you get a screen shot of the dialog? It would help with the UX review.
Comment 10 Wesley Johnston (:wesj) 2011-10-24 09:02:13 PDT
We could do all of the prompt stuff in javascript if we wanted (and if my prompts patch manages to ever land). I'm not sure if there's a huge advantage there or not (although it does move the prompt code off the main ui thread).
Comment 11 :Margaret Leibovic 2011-10-24 09:31:36 PDT
(In reply to Mark Finkle (:mfinkle) from comment #9)
> This is pretty good. How much more do you want to add before asking for
> review? Tracking multiple downloads?

I think tracking multiple downloads should work now, but I need to test it. I was going to address Alex's other concerns from comment 2, but as long as they're not regressions from the current state of things, we could probably put them in follow-up bugs.

(In reply to Wesley Johnston (:wesj) from comment #10)
> We could do all of the prompt stuff in javascript if we wanted (and if my
> prompts patch manages to ever land). I'm not sure if there's a huge
> advantage there or not (although it does move the prompt code off the main
> ui thread).

Wes, how soon do you think that will land? What I have right now seems to work and is pretty simple, so we can probably just convert them into JS prompts in another bug after your patch lands.
Comment 12 Wesley Johnston (:wesj) 2011-10-24 09:35:13 PDT
Hopefully lands today. Just need one more review. Low priority follow up sounds fine to me too though. Just wanted to throw out the idea.
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-24 09:48:00 PDT
(In reply to Wesley Johnston (:wesj) from comment #12)
> Hopefully lands today. Just need one more review. Low priority follow up
> sounds fine to me too though. Just wanted to throw out the idea.

Yeah, this sounds like a good idea. We can address in a followup.
Comment 14 :Margaret Leibovic 2011-10-24 14:14:41 PDT
Created attachment 569180 [details] [diff] [review]
patch (using prompt service)

I used the prompt service, and it makes the patch much nicer. Thanks, Wes!

I can file bugs for these, but I think the follow-up issues are:
-Hide the notification if the download opens on completion.
-Make the layout of the progress notification match native progress notifications (I noticed the progress bar doesn't align with the progress bars in other notifications, and that really bothers me).
Comment 15 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-24 14:43:30 PDT
Comment on attachment 569180 [details] [diff] [review]
patch (using prompt service)

Nice and clean.
Comment 16 :Margaret Leibovic 2011-10-24 15:02:04 PDT
Oops, I forgot to do r=mfinkle in the commit message, but people can follow the bug number:

https://hg.mozilla.org/projects/birch/rev/6f66e72d73d7

I'll file those follow-ups.
Comment 17 Sriram Ramasubramanian [:sriram] 2011-10-24 17:26:28 PDT
Created attachment 569244 [details] [diff] [review]
Patch avoiding styles

Remoteviews cannot use styles. Hence the layout's orientation should be specified in the XML. This patch does that.
Comment 18 :Margaret Leibovic 2011-10-24 17:47:10 PDT
(In reply to Sriram Ramasubramanian [:sriram] from comment #17)
> Created attachment 569244 [details] [diff] [review] [diff] [details] [review]
> Patch avoiding styles
> 
> Remoteviews cannot use styles. Hence the layout's orientation should be
> specified in the XML. This patch does that.

Maybe this patch should be in bug 696912 instead?
Comment 19 Alex Pakhotin (:alexp) 2011-10-24 18:07:29 PDT
Comment on attachment 569244 [details] [diff] [review]
Patch avoiding styles

If it actually fixes the bug 696912 it should be there though.
Comment 20 Dave Hunt (:davehunt) 2011-11-11 07:57:31 PST
Tested on Samsung Galaxy S2 during 11/11/11 testday.

It's possible to tap the in-progress download multiple times by ignoring the cancel prompt. This causes multiple prompts to build up. If you never choose to cancel the download then after the download is complete and you return to Fennec you then must dismiss all accumulated prompts. If you do choose to cancel the download then the download is cancelled and the file is not opened, but you still need to dismiss all of the prompts.

Expected:
Subsequently tapping the in-progress download should focus the existing cancel prompt.
Comment 21 Brad Lassey [:blassey] (use needinfo?) 2011-11-11 08:05:34 PST
This bug is fixed, file a new one
Comment 22 Dave Hunt (:davehunt) 2011-11-11 08:07:31 PST
Done. See bug 701722.

Note You need to log in before you can comment on or make changes to this bug.