Status

()

Firefox for Android
General
P1
normal
VERIFIED FIXED
6 years ago
9 months ago

People

(Reporter: johnath, Assigned: Margaret)

Tracking

({feature})

unspecified
ARM
Android
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

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

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
+++ 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.

Updated

6 years ago
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

Updated

6 years ago
Assignee: alexp → doug.turner
Whiteboard: [birch] [ux needed]
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)
Attachment #568125 - Flags: feedback?(mark.finkle)

Updated

6 years ago
Assignee: doug.turner → margaret.leibovic
(Assignee)

Comment 3

6 years ago
(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+
(Assignee)

Comment 6

6 years ago
(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.
(Assignee)

Comment 7

6 years ago
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.
Attachment #568125 - Attachment is obsolete: true
Attachment #568732 - Flags: feedback?(mark.finkle)
(Assignee)

Comment 8

6 years ago
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.
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).
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 14

6 years ago
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).
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+

Updated

6 years ago
Whiteboard: [birch] [ux needed] → [birch] [ux needed][QA+]
(Assignee)

Comment 16

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Blocks: 696911
(Assignee)

Updated

6 years ago
Blocks: 696912
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.
Attachment #569244 - Flags: review?(alexp)
(Assignee)

Comment 18

6 years ago
(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
Last Resolved: 6 years ago6 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+
status-firefox11: --- → fixed

Updated

5 years ago
Keywords: feature
You need to log in before you can comment on or make changes to this bug.