Add Pause and Cancel actions to download notifications

RESOLVED FIXED in Firefox 27

Status

()

RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: wesj, Assigned: fedepaol)

Tracking

unspecified
Firefox 27
x86
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=java][mentor=wesj])

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

6 years ago
Starting with JB, we can (easily) add actions to notifications. Right now if you tap a download notification we automatically cancel it. It might be nice to add an explicit button for that, along with an explicit Pause option.

Would provide some nice platform integration.
(Reporter)

Updated

6 years ago
Whiteboard: [lang=java][mentor=wesj]

Comment 1

6 years ago
Hello, I'm looking for an issue to work on for my Software Engineering class at school and was wondering if my partner and I can help with this one?  We are very proficient in java and this looks like a perfect issue for us. Thank you for the consideration!
Feel free to take it as it's currently unassigned. I suggest joining #mobile on irc.mozilla.org (IRC) to ask us any questions. Wes  is currently the mentor for this bug.
Assignee: nobody → aaronnguyen.h
OS: Linux → Android
(Reporter)

Comment 3

6 years ago
To do this guy, we currently use the nsIAlertsService for these notifications. I don't really want to overload it with more of this, so instead I'd like to implement a new NativeWindow api. Something like:

var id = NativeWindow.notifications.show(aIcon, aTitle, aMsg, aOptions);
NativeWindow.notifications.hide(id);

aOptions could include things like progress, buttons, and callbacks for when the notification is clicked. i.e. This will be a bit of work to get up and running.

Comment 4

6 years ago
I just got the source and will be looking into it. I'll be sure to join the irc channel as well.

Comment 5

6 years ago
Created attachment 718228 [details] [diff] [review]
Partial Patch Setting up the Notification with Buttons (WIP)

Takes place of the original Toast:show and sets up a 4.1+ android notification with buttons (function not implemented yet). Work In Progress. 

Build failed with errors.
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
11 errors
make[6]: *** [jars/gecko-browser.jar] Error 1
make[6]: Leaving directory `/home/fire/src/objdir-droid/mobile/android/base'
make[5]: *** [libs] Error 2
make[5]: Leaving directory `/home/fire/src/objdir-droid/mobile/android'
make[4]: *** [libs_tier_app] Error 2
make[4]: Leaving directory `/home/fire/src/objdir-droid'
make[3]: *** [tier_app] Error 2
make[3]: Leaving directory `/home/fire/src/objdir-droid'
make[2]: *** [default] Error 2
make[2]: Leaving directory `/home/fire/src/objdir-droid'
make[1]: *** [realbuild] Error 2
make[1]: Leaving directory `/home/fire/src'
make: *** [build] Error 2

We believe its the version of android that we are using for fennec and the items that aren't found are the default ICS button objects, and other objects. Would you like me to post the builderrors log that we have? Just let me know.

Thanks
Attachment #718228 - Flags: feedback-

Comment 6

6 years ago
Comment on attachment 718228 [details] [diff] [review]
Partial Patch Setting up the Notification with Buttons (WIP)

This apparently got lost in the shuffle.
Attachment #718228 - Flags: feedback- → feedback?(wjohnston)
(Reporter)

Comment 7

6 years ago
Comment on attachment 718228 [details] [diff] [review]
Partial Patch Setting up the Notification with Buttons (WIP)

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

Seems like a good first start, but this bug is a bit more complex than I thought maybe. I wrote out some thoughts and general ideas below. I'd be happy to help write some of this to get things moving, but it'd be good to see what mfinkle thinks about having something like this around... Would you rather move this into the XPCOM AlertsService (and keep expanding it like we did for progress). Lets ping him.

::: mobile/android/base/GeckoApp.java
@@ +755,5 @@
>              if (event.equals("Toast:Show")) {
>                  final String msg = message.getString("message");
>                  final String duration = message.getString("duration");
>                  handleShowToast(msg, duration);
> +            } else if (event.equals("ToastFennec:Show")) {

Hmm... So what we really want here are notifications, not toasts, but that's nomenclature here.

@@ +757,5 @@
>                  final String duration = message.getString("duration");
>                  handleShowToast(msg, duration);
> +            } else if (event.equals("ToastFennec:Show")) {
> +                //Figure what information the downloader needs to execute the download
> +                //Activate the download

This code should probably not know anything about what the toast is (i.e. downloads).

@@ +1096,5 @@
>          });
>      }
>  
> +    void handleShowToastFennec() {
> +        //Notification code goes here.

I'm going to recommend we move this into the NotificationClient class to save having so much code in GeckoApp. I wonder if we can just add an addFromJSON(JSONObject obj) method there that would pull apart the message. You can use a GeckoAppShell.sNotificationCLient from this code (tbh, that object should be lazily created instead of created in onCreate...)

That, in turn, can push the data to NotificationHandler, along with a String array for buttons.

@@ +1107,5 @@
> +                .setContentText("Download Bar? ")
> +                //set icon to firefox
> +                .setSmallIcon(R.drawable.icon)
> +                .setContentIntent(pendingIntent)
> +                .addAction(R.drawable.ic_media_pause, "Pause", pendingIntent)

We'll want to add some data to each pendingIntent we pass here (probably pendingIntent.clone().putExtra("Button", 1) so that we can tell what happened when you click. We should also sue the NotificationCompat stuff here. It will fall back gracefully on older versions for us:

http://developer.android.com/reference/android/support/v4/app/NotificationCompat.Builder.html

::: mobile/android/chrome/content/browser.js
@@ +1346,5 @@
> +        message: aMessage,
> +        duration: aDuration
> +      });
> +    }
> +  },

I'd recommend we pull all this out into its own js file too. Maybe named NotificationBuilder.js? Then we can do something Androidy like:

var guid = NotificationBuilder.create()
                              .setTitle("title")
                              .setMessage("message")
                              .
                              .addAction("Pause")
                              .addAction("Cancel")
                              .show();

NotificationBuilder.get(guid)
                   .setProgress(cur/total)
                   .update();

Android has a crazy rich set of stuff we can do here (and would save us some of our current work with progressbars too) I'd love to expose it to JS/Addons without having to write an XPCOM service for it. But as a start, I'd just make sure we can show a title, large and small icons, a message, and a progressbar.

And then we can add these buttons!
Attachment #718228 - Flags: feedback?(wjohnston)
(Reporter)

Updated

6 years ago
Flags: needinfo?(mark.finkle)
If work on this issue has stalled, I'd like to start working on this as my first contribution to Fennec.

Comment 9

6 years ago
(In reply to Marius Cristian Solomon from comment #8)
> If work on this issue has stalled, I'd like to start working on this as my
> first contribution to Fennec.

There hasn't been activity on this patch for a while, so it's probably safe to pick it up and start working on it. However, it looks like it may be a tricky first bug, so it may require a lot of work :)

Aaron, do you intend to finish this patch, or do you want to let Marius take over here?

We should also hear what mfinkle has to say in response to wesj's comment. I haven't spent much time in this code, but I think it would be nice to start with something simple, then decide whether or not to make it part of the AlertsService later.
Flags: needinfo?(aaronnguyen.h)

Comment 10

6 years ago
Hi Margaret, it's fine by me to hand it over to Marius. I have had little time to work on this patch and would probably be for the best.

Hey Marius, here is a link to a conversation with wesj about how he wanted to handle this feature: http://pastebin.mozilla.org/2369623 . some of the links to the code are off due to changes but hope this helps.
Flags: needinfo?(aaronnguyen.h)
Thanks Margaret and Aaron. I'll post updates as I make progress.
I'd be OK with creating a NativeWindow.Notification system
Flags: needinfo?(mark.finkle)
(Assignee)

Comment 13

5 years ago
Hi there, can I grab this one or is Marius still working on it?

Comment 14

5 years ago
(In reply to Federico Paolinelli from comment #13)
> Hi there, can I grab this one or is Marius still working on it?

There hasn't been any activity here in a while, so it's probably okay to take it, but let's ask Marius if he still wants to try working on this.
Flags: needinfo?(i.code.in.binary)
(Assignee)

Comment 15

5 years ago
Any news about this? Can I take the bug?

Comment 16

5 years ago
(In reply to Federico Paolinelli from comment #15)
> Any news about this? Can I take the bug?

Yeah, enough time has passed that you should just go ahead and work on this.
Assignee: aaronnguyen.h → fedepaol
Flags: needinfo?(i.code.in.binary)
(Reporter)

Comment 17

5 years ago
I really want to do this by writing an extended notifications api. Something similar to the Android builder interface:

http://developer.android.com/reference/android/support/v4/app/NotificationCompat.Builder.html

maybe with a hint of the chrome extension api thrown in (actually, the Chrome api covers most of what we'd want to do I think...):

http://developer.chrome.com/dev/extensions/notifications.html

I started this a little in WebrtcUI.js and NotificationsHelper.java, but I'd really like to see it made into a real .jsm file like Prompt.jsm:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Prompt.jsm

and accessible to addons.
(Assignee)

Comment 18

5 years ago
Created attachment 785970 [details] [diff] [review]
First prototype

There is some commented out code because I am trying to figure out how to make the pending intents work through broadcasts.

At the moment the pending intents are firing the activity which has the effect to minimize the notification.

It's a decent first prototype anyway, there is enough material to receive feedback..
Attachment #785970 - Flags: feedback?(wjohnston)
(Assignee)

Updated

5 years ago
Attachment #785970 - Attachment is patch: true
(Assignee)

Comment 19

5 years ago
Created attachment 786372 [details] [diff] [review]
Second version

Moved the pending intent from being an Intent directed to the GeckoApp activity to a broadcast intercepted by a broadcast receiver installed in the helper. This way fennec does not get brought on front when pressing pause / resume
Attachment #785970 - Attachment is obsolete: true
Attachment #785970 - Flags: feedback?(wjohnston)
Attachment #786372 - Flags: feedback?(wjohnston)
(Reporter)

Comment 20

5 years ago
Comment on attachment 786372 [details] [diff] [review]
Second version

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

Still looking, but some comments.

::: mobile/android/base/NotificationHelper.java
@@ +34,5 @@
> +
> +    // Attributes that can be used while sending a notification from js.
> +    private static final String NOTIFICATION_ATTRIBUTE_TITLE = "title";
> +    private static final String NOTIFICATION_ATTRIBUTE_TEXT = "text";
> +    private static final String NOTIFICATION_ATTRIBUTE_ID = "id";

I think title, text, id, and smallIcon are all required. I wonder if we should put them in a special section here.

@@ +56,2 @@
>  
>      public NotificationHelper(Context context) {

Not 100% related, but I'd like to make this a singleton, and not require GeckoApp to hold on to it (that should also help if the activity is killed/restored).

@@ +83,5 @@
> +    }
> +
> +    private void registerReceiver(Context context) {
> +        IntentFilter filter = new IntentFilter(HELPER_BROADCAST_ACTION);
> +        filter.addDataScheme("alert");

You should add a comment about why we need this data scheme.

@@ +110,5 @@
> +
> +        // In case the user swiped out the notification, we empty the id
> +        // set.
> +        if ("delete".equals(notificationType)) {
> +            mShowing.remove(id);

We should notify Javascript about this if we can too (maybe only if Gecko is running, although I think this broadcast receiver dies when Gecko dies anyway...)

@@ +125,5 @@
> +            JSONObject args = new JSONObject();
> +            try {
> +                args.put(NOTIFICATION_ID, id);
> +                if (cookie != null) {
> +                    args.put(NOTIFICATION_ATTRIBUTE_COOKIE, data.getQueryParameter(NOTIFICATION_ATTRIBUTE_COOKIE));

You don't need to get this again, right? You can just reuse the string you have.

@@ +166,5 @@
> +            final String id = message.getString(NOTIFICATION_ATTRIBUTE_ID);
> +            final long progress = message.getLong(NOTIFICATION_ATTRIBUTE_PROGRESS_VALUE);
> +            final long progressMax = message.getLong(NOTIFICATION_ATTRIBUTE_PROGRESS_MAX);
> +            final String text = message.getString(NOTIFICATION_ATTRIBUTE_TEXT);
> +            GeckoAppShell.sNotificationClient.update(id.hashCode(), progress, progressMax, text);

Why are we going through the client here? This worries me a bit because I think, unless you pass a Notification directly to NotificationHandler, we'll create an AlertNotification which uses a remote view for progress and things. I created a new version of add for this originally as well:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/NotificationClient.java#93

Then again, I don't see anyone sending Notification:UpdateProgress messages anyway...

@@ +233,5 @@
> +                    JSONObject action = actions.getJSONObject(i);
> +                    final PendingIntent pending = buildNotificationPendingIntent(message, action.getString(NOTIFICATION_ATTRIBUTE_ACTION));
> +                    final String actionTitle = action.getString(NOTIFICATION_ATTRIBUTE_ACTION_TITLE);
> +                    final Uri actionImage = Uri.parse(message.optString(NOTIFICATION_ATTRIBUTE_ACTION_ICON));
> +                    builder.addAction(BitmapUtils.getResource(actionImage, R.drawable.ic_status_logo),

I think we should probably just ignore the icon if its not specified.

::: mobile/android/chrome/content/downloads.js
@@ +12,5 @@
>  const URI_GENERIC_ICON_DOWNLOAD = "drawable://alert_download";
>  
> +const PAUSE_ACTION = {
> +  actionKind : "pause",
> +  title : Strings.browser.GetStringFromName("alertDownloadsPause"),

I wonder if defining these at startup has any startup time implication.

@@ +49,5 @@
>      this._initialized = true;
>  
>      // Monitor downloads and display alerts
>      this._dlmgr = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
> +    this._bridge = Cc["@mozilla.org/android/bridge;1"].getService(Ci.nsIAndroidBridge);

We'd usually make this a lazy loaded thing.

get _bridge() {
  delete this._bridge;
  return this._bridge = stuff...
}

@@ +122,5 @@
> +  handleNotificationEvent: function dl_handleNotificationEvent(aNotifData, aDownload) {
> +    switch (aNotifData.kind) {
> +      case "click": {
> +        // Use this flag to make sure we only show one prompt at a time
> +        let cancelPrompt = false;

I don't think you want to initialize this here....

@@ +204,5 @@
>        let availableSpace = aDownload.targetFile.diskSpaceAvailable;
>      } catch(ex) { }
>      let contentLength = aDownload.size;
>      if (availableSpace > 0 && contentLength > 0 && contentLength > availableSpace) {
> +      Downloads.showAlert(aDownload, strings.GetStringFromName("lertDownloadsNoSpace"),

Dropped a letter here. Also we should probably just rename Downloads.showAlert to Downloads.showNotification. That way English speakers like me can have life even easier.
(Assignee)

Comment 21

5 years ago
(In reply to Wesley Johnston (:wesj) from comment #20)

> @@ +233,5 @@
> > +                    JSONObject action = actions.getJSONObject(i);
> > +                    final PendingIntent pending = buildNotificationPendingIntent(message, action.getString(NOTIFICATION_ATTRIBUTE_ACTION));
> > +                    final String actionTitle = action.getString(NOTIFICATION_ATTRIBUTE_ACTION_TITLE);
> > +                    final Uri actionImage = Uri.parse(message.optString(NOTIFICATION_ATTRIBUTE_ACTION_ICON));
> > +                    builder.addAction(BitmapUtils.getResource(actionImage, R.drawable.ic_status_logo),
> 
> I think we should probably just ignore the icon if its not specified.
> 

I guess we have to choose here, between not showing the action or showing it with a default icon. I'd prefer showing the action anyway, but both cases are fallbacks for a wrong usage of the api. I am leaving as it is at the moment.
(Assignee)

Comment 22

5 years ago
Created attachment 787179 [details] [diff] [review]
bugfix-815202
Attachment #786372 - Attachment is obsolete: true
Attachment #786372 - Flags: feedback?(wjohnston)
Attachment #787179 - Flags: review?(wjohnston)

Comment 23

5 years ago
I do have some knowledge in this language. i really, find confident in fixing this bug.
(Reporter)

Comment 24

5 years ago
(In reply to bennymartin from comment #23)
> I do have some knowledge in this language. i really, find confident in
> fixing this bug.

Hey, thanks for looking. Federico already has a mostly complete patch for this bug, but we have java things to work on at http://www.joshmatthews.net/bugsahoy/?mobile=1&java=1 (you'll have to manually see if anyone is assigned yet and if they're actively working on it). We have plenty of things that need done and need to add more to that list.
(Reporter)

Comment 25

5 years ago
Comment on attachment 787179 [details] [diff] [review]
bugfix-815202

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

I like this a lot. These are almost all nits.

I still have problems with this on my S3 (Samsung insists on hiding the buttons by default, and hides them again every time the progress updates). I think that's an S3 bug TBH, and since they're hidden by default, most users probably won't even know. I'm not sure what the right soluion is.

Let's needinfo ian for some button icons (required by Android), and I threw a build up for him at:
http://people.mozilla.com/~wjohnston/notification.apk

::: mobile/android/base/NotificationHelper.java
@@ +49,5 @@
> +    private static final String NOTIFICATION_ATTRIBUTE_EVENT_KIND = "kind";
> +    private static final String NOTIFICATION_ATTRIBUTE_ACTIONS = "actions";
> +    private static final String NOTIFICATION_ATTRIBUTE_ACTION = "actionKind";
> +    private static final String NOTIFICATION_ATTRIBUTE_ACTION_TITLE = "title";
> +    private static final String NOTIFICATION_ATTRIBUTE_ACTION_ICON = "icon";

We never use this many constants for JSON. I'm not sure if I like it or not. Having huge contants written out everywhere feels... like a lot of words. But I'm willing to give it a try in this file. Maybe we can shorten these a little?

PROGRESS_VALUE_MSG/JSON?
PROGRESS_MAX_MSG/JSON?
LIGHT_MSG/JSON?

@@ +68,5 @@
>          mContext = context;
>          mShowing = new HashSet<String>();
>          registerEventListener("Notification:Show");
>          registerEventListener("Notification:Hide");
> +        registerReceiver(context);

Do we need to do this all the time? Or would we be better to wait until we're showing a notification?

@@ +91,5 @@
> +
> +    private static void registerReceiver(Context context) {
> +        IntentFilter filter = new IntentFilter(HELPER_BROADCAST_ACTION);
> +        // Scheme is needed, otherwise only broadcast with no data will be catched.
> +        filter.addDataScheme("alert");

"alert" feels likely to collide at some point. Can we do something a bit more specific? "moz-notification" or something?

@@ +117,5 @@
> +        }
> +
> +        // In case the user swiped out the notification, we empty the id
> +        // set.
> +        if ("cleared".equals(notificationType)) {

Can we make this (and the "click") string constants?

@@ +148,5 @@
> +        final boolean ongoing = message.optBoolean(NOTIFICATION_ATTRIBUTE_ONGOING);
> +        notificationIntent.putExtra(NOTIFICATION_ATTRIBUTE_ONGOING, ongoing);
> +
> +        Uri.Builder b = new Uri.Builder();
> +        b.scheme("alert").appendQueryParameter(NOTIFICATION_ATTRIBUTE_EVENT_KIND, kind);

constant?

::: mobile/android/chrome/content/downloads.js
@@ +87,5 @@
> +        type: "Notification:Show",
> +        id: this.getNotificationIdFromDownload(aDownload),
> +        title: aTitle,
> +        smallIcon: URI_GENERIC_ICON_DOWNLOAD,
> +        text: aMessage,        

whitespace

@@ +118,5 @@
> +    this._bridge.handleGeckoMessage(JSON.stringify(msg));
> +  },
> +
> +  handleNotificationEvent: function dl_handleNotificationEvent(aNotifData, aDownload) {
> +    switch (aNotifData.kind) {

Lets name things something better than "kind". Maybe aNotifData.responseType?

@@ +136,5 @@
> +                                                     null, null, null, null, {});
> +          if (choice == 0)
> +            this.cancelDownload(aDownload);
> +          this._showingPrompt = false;
> +        }

Lets pull all "click" handling this out into its own function if we can, like the other guys below.

@@ +144,5 @@
> +        this.cancelDownload(aDownload);
> +      break;
> +      case "pause":
> +        aDownload.pause();
> +      break;

indent "break;" to match aDownload.pause();. same for others.

@@ +148,5 @@
> +      break;
> +      case "resume":
> +        aDownload.resume();
> +      break;
> +      case "cleared":

I wonder if we should name this "notification-cleared" to avoid confusion with buttons people might add. Same with the "click" message above. Also, maybe we should throw somewhere if people try to use these "reserved" names.

@@ +163,5 @@
> +        let guid = data.cookie;
> +        this._dlmgr.getDownloadByGUID(guid, (function(status, download) {
> +          if (Components.isSuccessCode(status)){
> +            this.handleNotificationEvent(data, download);
> +          }

No need for braces here.

@@ +171,5 @@
> +      case "last-pb-context-exited": {
> +        let download;
> +        while ((download = this._privateDownloads.pop())) {
> +          try {
> +            Downloads.removeNotification(download); 

trailing whitespace
Attachment #787179 - Flags: review?(wjohnston) → review+
(Reporter)

Comment 26

5 years ago
Also, there's a huge-ish screenshot of this at:

http://dl.dropboxusercontent.com/u/72157/buttons.png

Needs some nice pause/resume/cancel icons.
Flags: needinfo?(ibarlow)
(Assignee)

Comment 27

5 years ago
Created attachment 796237 [details] [diff] [review]
bugfix-815202

Added the changes requested.
Attachment #787179 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 909932
(Assignee)

Comment 28

5 years ago
Created attachment 801254 [details] [diff] [review]
bugfix-815202

- Fixed a small bug in icons url passing (I was getting the icon url from the main message instead of from the action sub-object)

- Tried to draw the (very very basic) icons for notifications buttons (pause, resume, cancel). The icons are 32 dip big and I made the three different versions for mdpi, hdpi, xhdpi. If needed, I can also provide the svg.

Since this is the first time I attach a binary to a patch, I hope I am not messing anything up.
Attachment #796237 - Attachment is obsolete: true
Attachment #801254 - Flags: feedback?(wjohnston)
Attachment #801254 - Flags: feedback?(ibarlow)
(Assignee)

Comment 29

5 years ago
Created attachment 801256 [details] [diff] [review]
bugfix-815202

Just realized that I left a couple of "FEDE" debugging traces in the previous patch :P .
Attachment #801254 - Attachment is obsolete: true
Attachment #801254 - Flags: feedback?(wjohnston)
Attachment #801254 - Flags: feedback?(ibarlow)
Attachment #801256 - Flags: feedback?(wjohnston)
Attachment #801256 - Flags: feedback?(ibarlow)
(Reporter)

Comment 30

5 years ago
Comment on attachment 801256 [details] [diff] [review]
bugfix-815202

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

I found a bug here with clicking on the notification while its paused that I think we need to fix, and maybe one with clicking the cancel button. I also tried to get some screenshots so ian can give this at least an "I'm ok with landing this and tweaking it" thumbs up/down.

fedepaol's icons: http://dl.dropboxusercontent.com/u/72157/buttons1.png
some stock icons: http://dl.dropboxusercontent.com/u/72157/buttons3.png

::: mobile/android/base/NotificationHelper.java
@@ +231,5 @@
> +                for (int i = 0; i < actions.length(); i++) {
> +                    JSONObject action = actions.getJSONObject(i);
> +                    final PendingIntent pending = buildNotificationPendingIntent(message, action.getString(ACTION_ATTR));
> +                    final String actionTitle = action.getString(ACTION_TITLE_ATTR);
> +                    final Uri actionImage = Uri.parse(action.optString(ACTION_ICON_ATTR));

Sorry I missed this :(

::: mobile/android/chrome/content/downloads.js
@@ +11,5 @@
>  
>  const URI_GENERIC_ICON_DOWNLOAD = "drawable://alert_download";
> +const URI_PAUSE_ICON = "drawable://ic_notif_button_pause";
> +const URI_CANCEL_ICON = "drawable://ic_notif_button_cancel";
> +const URI_RESUME_ICON = "drawable://ic_notif_button_resume";

I wonder if we're better to just use stock icons here for now?

@@ +119,5 @@
> +        id: this.getNotificationIdFromDownload(aDownload)
> +    };
> +    this._bridge.handleGeckoMessage(JSON.stringify(msg));
> +  },
> +  

ws, just because I can.

@@ +124,5 @@
> +  handleClickEvent: function dl_handleClickEvent(aDownload) {
> +    if (aDownload.state == Ci.nsIDownloadManager.DOWNLOAD_FINISHED) {
> +      // Only open the downloaded file if the download is complete
> +      this.openDownload(aDownload);
> +    } else if (aDownload.state == Ci.nsIDownloadManager.DOWNLOAD_DOWNLOADING && !this._showingPrompt) {

I think we may need to extend this to run if its paused now. Right now, pausing and clicking the notification just hides the download forever.

@@ +145,5 @@
> +      case "notification-clicked":
> +        this.handleClickEvent(aDownload);
> +        break;
> +      case "cancel":
> +        this.cancelDownload(aDownload);

I also think we may need to prompt here. Its easy to accidentally click on the wrong place, and just force killing the download feels wrong...

@@ +241,5 @@
>                              aDownload.displayName);
>          break;
> +      case Ci.nsIDownloadManager.DOWNLOAD_PAUSED:
> +        Downloads.showNotification(aDownload, aDownload.percentComplete + "%", 
> +                            aDownload.displayName, { percentComplete: aDownload.percentComplete, 

ws
Attachment #801256 - Flags: feedback?(wjohnston)
(Assignee)

Comment 31

5 years ago
Created attachment 804029 [details] [diff] [review]
bugfix-815202

I fixed the click on paused bug. I also tried to show the confirm on cancel prompt while the cancel button was pressed, but the weird effect is that the prompt is being shown *under* the notification "curtain".
From the user's point of view, what happens is that he presses the button, nothing happens because the prompt is hidden, and when he returns to fennec he finds a prompt he might not remember of. In any case, if you prefer this other counter-intuitive approach, it's easy to implement since I moved the show prompt stuff in a separate function.
Attachment #801256 - Attachment is obsolete: true
Attachment #801256 - Flags: feedback?(ibarlow)
Attachment #804029 - Flags: feedback?(wjohnston)
(Assignee)

Comment 32

5 years ago
Created attachment 808780 [details] [diff] [review]
bugfix-815202

Changes to resolve conflicts after update
Attachment #804029 - Attachment is obsolete: true
Attachment #804029 - Flags: feedback?(wjohnston)
(Reporter)

Comment 33

5 years ago
Comment on attachment 808780 [details] [diff] [review]
bugfix-815202

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

I think this looks good now. Anthony sent me some icons. I'm going to land it with them and we can move on from there :)
Attachment #808780 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3181933b1780
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27

Updated

5 years ago
Flags: needinfo?(ibarlow)
Depends on: 932816
Depends on: 933275
Is it possible to use the area left of the Pause/Resume button to widen both buttons (or reducing the margin of the icons)? At the moment, the German translation for the "Cancel" button ("Abbrechen") gets cropped.
Please file a new bug.
You need to log in before you can comment on or make changes to this bug.