Closed Bug 695461 Opened 8 years ago Closed 8 years ago

Download notifications

Categories

(Firefox for Android :: General, defect, P1)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: johnath, Assigned: Margaret)

References

Details

(Keywords: feature, Whiteboard: [birch] [ux needed][QA!][testday-20111111])

Attachments

(2 files, 3 obsolete files)

+++ 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.
Assignee: nobody → alexp
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.
Blocks: 695178
No longer depends on: 695178
Assignee: alexp → doug.turner
Whiteboard: [birch] [ux needed]
Attached patch [WIP] Patch (obsolete) — Splinter Review
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)
Attachment #568125 - Flags: feedback?(mark.finkle)
Assignee: doug.turner → margaret.leibovic
(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.
(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 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.
Attachment #568125 - Flags: feedback?(mark.finkle) → feedback+
(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.
Attached patch WIP patch v2 (obsolete) — Splinter Review
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.
Attachment #568125 - Attachment is obsolete: true
Attachment #568732 - Flags: feedback?(mark.finkle)
Attached patch WIP patch v3 (obsolete) — Splinter Review
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.
Attachment #568732 - Attachment is obsolete: true
Attachment #568732 - Flags: feedback?(mark.finkle)
Attachment #568763 - Flags: feedback?(mark.finkle)
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.
Attachment #568763 - Flags: feedback?(mark.finkle) → feedback+
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).
(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.
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.
(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.
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).
Attachment #568763 - Attachment is obsolete: true
Attachment #569180 - Flags: review?(mark.finkle)
Comment on attachment 569180 [details] [diff] [review]
patch (using prompt service)

Nice and clean.
Attachment #569180 - Flags: review?(mark.finkle) → review+
Whiteboard: [birch] [ux needed] → [birch] [ux needed][QA+]
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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 696911
Blocks: 696912
Remoteviews cannot use styles. Hence the layout's orientation should be specified in the XML. This patch does that.
Attachment #569244 - Flags: review?(alexp)
(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 on attachment 569244 [details] [diff] [review]
Patch avoiding styles

If it actually fixes the bug 696912 it should be there though.
Attachment #569244 - Flags: review?(alexp) → review+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [birch] [ux needed][QA+] → [birch] [ux needed][QA+][testday-20111111]
This bug is fixed, file a new one
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Done. See bug 701722.
Status: RESOLVED → VERIFIED
Whiteboard: [birch] [ux needed][QA+][testday-20111111] → [birch] [ux needed][QA!][testday-20111111]
tracking-fennec: --- → 11+
Keywords: feature
You need to log in before you can comment on or make changes to this bug.