jetpack.notifications.show doesn't allow skipping options

RESOLVED FIXED in 0.5

Status

Mozilla Labs
Jetpack Prototype
P3
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: sephr, Assigned: atul)

Tracking

unspecified

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.10) Gecko/2009042523 Ubuntu/9.04 (jaunty) Firefox/3.0.10 GTB5
Build Identifier: 

There is no sane way to skip the body, title, or icon properties for jetpack.notifications.show without the function going back to defaults. Of course you could specify a string with a space, but you shouldn't have to do that. What should be checked is if the properties exist, not if they == true.

IMO, I think that the default title should be changed to "Firefox Notification" and there be no default icon. If anyone else shares this view, maybe it should be considered in the fixed jetpack.notifications.show.

Reproducible: Always

Steps to Reproduce:
1. jetpack.notifications.show({icon:null, body:"This notification has no icon", title:"Notification"})
Actual Results:  
Mozilla favicon is shown in notification.

Expected Results:  
No icon is shown in notification.

Maby a good fix could be something along the lines of this function (I also changed the error handling to log body instead of message):

function (message) {
    var body = message,
    title = "Firefox Notification",
    icon = null;
    if (typeof message == "object") {
        body = message.body;
        if ("title" in message) {
            title = message.title;
        }
        if ("icon" in message) {
            icon = message.icon;
        }
    }
    try {
        var classObj = Cc['@mozilla.org/alerts-service;1'];
        var alertService = classObj.getService(Ci.nsIAlertsService);
        alertService.showAlertNotification(icon, title, body);
        return true;
    } catch (e) {
        console.log("Unable to display notification:", body);
        return false;
    }
}

Comment 1

9 years ago
I also think there should be no default icon.
(Reporter)

Comment 2

9 years ago
In my exception catch, I just remembered that Firebug would just list the properties so
    console.log("Unable to display notification:", body);
should be
    console.log("Unable to display notification:", message);
instead.

Comment 3

9 years ago
Created attachment 379390 [details] [diff] [review]
Patch v0.1

Also added callbacks, cookies, and clickable features as outlined in nsIAlertsService.
Attachment #379390 - Flags: review?(avarma)

Comment 4

9 years ago
Created attachment 379395 [details] [diff] [review]
Patch v0.2

Refactored v0.1
Attachment #379390 - Attachment is obsolete: true
Attachment #379395 - Flags: review?(avarma)
Attachment #379390 - Flags: review?(avarma)

Comment 5

9 years ago
Comment on attachment 379395 [details] [diff] [review]
Patch v0.2

you still have tabs

+             title = message.title;

is overindented

and of course, you're using 'firefox' a branded string and 'notification' an unlocalized string.
(Assignee)

Comment 6

9 years ago
Thanks for the patch!  Hm, this looks good overall, but for now could you remove the extra options for "cookie", "callback", and "clickable" that you added?

The reason for this is because of the fact that jetpacks can be unloaded, we'll need to be able to remove that callback whenever the jetpack is unloaded.  You can look at the code for status-bar-panel.js or tabs.js for information on how to do this, but we'd really prefer that a new bug be created for that.  Hope you don't mind!
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 7

9 years ago
Yeah I can take those out for sure and file a new bug maybe. Although I think the new bug is kinda low priority right now for Jetpack. But yeah I'll submit a new patch soon.
atul to review for .5
Assignee: nobody → varmaa
Priority: -- → P3
Target Milestone: -- → 0.5
(Assignee)

Updated

9 years ago
Assignee: varmaa → avarma
Hi steven, 

Are you submitting the new patch with the changes Atul requested?
(Assignee)

Comment 10

9 years ago
Created attachment 395156 [details] [diff] [review]
Alternate patch that sets default title to "(app name) Notification" and default icon to null (which is ultimately the app's icon), and supports setting of title to null.
Attachment #379395 - Attachment is obsolete: true
Attachment #379395 - Flags: review?(avarma)
(Assignee)

Updated

9 years ago
Attachment #395156 - Attachment is patch: true
Attachment #395156 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 11

9 years ago
Ok, I've gone ahead and committed a version of the patch here:

  http://hg.mozilla.org/labs/jetpack/rev/a6b81b27db2c

Thanks!
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.