Closed
Bug 494549
Opened 15 years ago
Closed 15 years ago
jetpack.notifications.show doesn't allow skipping options
Categories
(Mozilla Labs :: Jetpack Prototype, defect, P3)
Mozilla Labs
Jetpack Prototype
Tracking
(Not tracked)
RESOLVED
FIXED
0.5
People
(Reporter: sephr, Assigned: avarma)
Details
Attachments
(1 file, 2 obsolete files)
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•15 years ago
|
||
I also think there should be no default icon.
Reporter | ||
Comment 2•15 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•15 years ago
|
||
Also added callbacks, cookies, and clickable features as outlined in nsIAlertsService.
Attachment #379390 -
Flags: review?(avarma)
Comment 4•15 years ago
|
||
Refactored v0.1
Attachment #379390 -
Attachment is obsolete: true
Attachment #379395 -
Flags: review?(avarma)
Attachment #379390 -
Flags: review?(avarma)
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•15 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•15 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.
Comment 8•15 years ago
|
||
atul to review for .5
Assignee: nobody → varmaa
Priority: -- → P3
Target Milestone: -- → 0.5
Assignee | ||
Updated•15 years ago
|
Assignee: varmaa → avarma
Comment 9•15 years ago
|
||
Hi steven, Are you submitting the new patch with the changes Atul requested?
Assignee | ||
Comment 10•15 years ago
|
||
Attachment #379395 -
Attachment is obsolete: true
Attachment #379395 -
Flags: review?(avarma)
Assignee | ||
Updated•15 years ago
|
Attachment #395156 -
Attachment is patch: true
Attachment #395156 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 11•15 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
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•