Closed Bug 622714 Opened 14 years ago Closed 10 years ago

ChatZilla - Provide visual notification of events

Categories

(Other Applications :: ChatZilla, enhancement)

Product:
Other Applications
Component:
ChatZilla
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: trevorhemail-mozbug, Assigned: hunboy)

References

Details

(Whiteboard: [cz-0.9.91])

Attachments

(8 files, 18 obsolete files)

cz-plugin-toaster-popup.js v1.6
13.32 KB, text/plain
Details
spbubble.png
60.15 KB, image/png
Details
spbubble-off.png
54.26 KB, image/png
Details
spbubble-16x16.png
663 bytes, image/png
Details
spbubble-off-16x16.png
710 bytes, image/png
Details
graphical presentation of preferences ,and statusbar button
111.21 KB, text/html
Details
ACTION message sample layout
4.84 KB, image/png
Details
patch v1.4.0
131.41 KB, patch
hunboy
: review+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-AU; rv:1.9.2.13) Gecko/20101206 Ubuntu/10.10 (maverick) Firefox/3.6.13 Build Identifier: Currently you can use sounds to provide notification of events. It would be nice if a visual alternative could be available as well. Reproducible: Always
Attached file Proposed patch (obsolete) —
Needs refinement and localization.
Attached file cz-plugin-toaster-popup.js v1.0 (obsolete) —
Trevor: thank you, it works pretty well :) I prepared a plugin from your patch with some modifications. I kept the stuff at the end of the __display function, and left away the setTabState, because modifying the interface of setTabState is inconsistent with the channel-tree plugin, and this is rather a display issue and not setting a state of a tab. It doesn't disturb anything at the end of __display. And merged your 2 functions to 1, and added some separator spaces to the message strings. This is backwards compatible, works pretty well on my Netscape72 and on ff4 also. I couldn't check what happens in that case, when the toaster is unavailable, maybe we need a MAC user to test it, the plugin never will be enabled if the nsIAlertsService doesnt work, but in the native integration the __display always will call the functions for alerts. Thank you again, this was what needed to me, it's on my client also. Feel free to improve the plugin :)
Attached file cz-plugin-toaster-popup.js v1.1 (obsolete) —
adding current channelname or current servername for toaster instead of "channel" and "network" mark
Attached file cz-plugin-toaster-popup.js v1.2 (obsolete) —
removing mirc formatting codes from the popup message general revision: fixing: we don't want to get popup in PM either when that is current and window is focused removing pref: user.stalk (no stalkmatch in PM) creating pref: user.chat handling DCC-CHAT, DCC-FILE, INVITE, NOTICE, WALLOPS permanently az user.chat applying stalk only for stalkmatch adding networkname to popup for user.chat if avail hiding channel.start, user.start coz does nothing (that should be a different function) Is start event nessesary so far? I mean if a new channel opens that always sends a superfluous event, and if a pm opens that always opens with a message. Autoopen only at startup, but that does popupflood if too much channels are in auto-connect. And I'm about here when happens.
Attachment #501864 - Attachment is obsolete: true
Attachment #501999 - Attachment is obsolete: true
Attached file cz-plugin-toaster-popup.js v1.3 (obsolete) —
Adding exception-handler to showAlertNotification. We can't be sure if it is a MAC OS X and Growl is now turned off or not. --- Adding overlap-delay. We dont want to get 10 popups in the same time, when someone else quits the server and leaves 10 channels in the same time where we are on also. This one not too high for not to lose too much messages. The initial value is: /plugin-pref toaster-popup overlapDelay 50 --- Introducing the flood protector. When the massive flood attack comes, it (hopefully) stops showing popups during the attack. (Btw this stuff requires a piano repairman to accord it up properly.) The initial values are: /plugin-pref toaster-popup floodDensity 290 /plugin-pref toaster-popup floodDispersion 200
Attachment #502342 - Attachment is obsolete: true
Attached file cz-plugin-toaster-popup.js v1.4 (obsolete) —
moving "if (!plugin.enabled || !event) return;" from the showEventAlerts to the hook, because that is better place for it. clearing out the view.start prefs and related functions, because that is useless so far (a new view never opens just for fun). (code is available from the obsolete versions) Adding new pref nonFocusedOnly. Someone hates getting popups when the chatzilla is focused, but someone likes follow the other chats via popup without view-changing. Default value is false, means popups come from non-current views also. Menu item added. Adding pref clickable. When this is true, the popup itself is clickable, and focuses to the chatzilla by setting up to the current view where that message comes from. Default value is false (perhaps because that blue underlined text is ugly when the popup is clickable) (this required a new attrib at the function showEventAlerts) Menu item added.
Attachment #506056 - Attachment is obsolete: true
Attached file cz-plugin-toaster-popup.js v1.5 (obsolete) —
Using dispatch instead of direct call of setCurrentObject() at alertListener (plugin.clicker) observe and onAlertClickCallback. The command: set-current-view is hookable and perhaps channel-tree plugin uses that.
Attachment #507033 - Attachment is obsolete: true
Removing MacNames from the showAlertNotification, because that kills Growl on Mac. Not a joke :) Now this plugin as a plugin is stable on all of 3 major OS's. When Growl is not installed on MAC, plugin disables himself at startup. After installing Growl it works fine. Plugin is ready to think forward for more userfriendly behaviors such as network/channel dependent prefs etc. And ready to think of the native integration.
Attachment #507558 - Attachment is obsolete: true
Attached image spbubble.png —
Assignee: rginda → utasir
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached image spbubble-off.png —
Attached image spbubble-16x16.png —
Attached image spbubble-off-16x16.png —
Version: unspecified → Trunk
Silver, I'm about to prepare this for native integration, so I need your pre-review before doing anything. Shortly, I plan to ingegrate in a same way as view logging is implemented, this is the reason I uploaded here icons to show them. (transparent bg) With this the first question, clicking bubble permanently en/disables try notification, or only for that view? Other questions, what do you want as default behavior of try notification? (see plugin) Do you have any notices and/or requirements of this feature? (Plugin is stable after years of testing period, passes floodattacks perhaps by floodprotector. Tested on win,linux,mac, works well everywhere)
Flags: needinfo?(bugzilla-mozilla-20020327)
Comment on attachment 500930 [details] Proposed patch Trevor, i set up obsolote your patch, not to make confusion.
Attachment #500930 - Attachment is obsolete: true
Attachment #500930 - Attachment is patch: false
(In reply to Robert Utasi [:hunboy] from comment #13) > With this the first question, clicking bubble permanently en/disables try > notification, or only for that view? Hmm, that's actually a tricky one since the logging icon is this-view and the offline icon is global. Since the preferences seem to be global let's go with that - toggle the alert.enabled pref. > Other questions, what do you want as default behavior of try notification? > (see plugin) > > Do you have any notices and/or requirements of this feature? I guess I'll have to try out the plugin. It's pretty hard to impress me when it comes to UI features so I might be disgusted by what Firefox does for the alerts... but we should probably have some reasonable defaults to notify people here. :)
Flags: needinfo?(bugzilla-mozilla-20020327)
Okay, so the patch doesn't apply because it's using code from Nov 2010. Let me know when you've updated to tip so I can try it out. :)
patch was the simplesample, but buggy I developed the plugin from that as v1.0, Now the plugin is v1.6 this is the stable version.
the current default is PM and stalk at other views. And some oper messages. I stopped implement the network/channel dependent settings because this is easier in the native integration than in a plugin. I just need cloning the logging feature. You can try plugin, actually i use this one for following chats while doing anything else around, and if the popup shows interesting mess I go to chat, otherwise not :D Yeah I think the global off/on is the good, we will turn off/on locally at preferences anyway the specified channels/networks, but better turning off globally during watching dvd on computer or any other reasons.
Attached patch patch v1.0 (obsolete) — — Splinter Review
Silver, Here is the first version of patch, newly implemented parts are: global/network/channel/user dependent prefs based on inheritance. This means you can choose which view you want to follow by popup and how. I've tested it, works properly. Old implementations are explained at plugin contrib comments, pl. read basically what "only when non focused" and "popup is clickable" means punctually and why I added them. These ones are requested. Pl. review my english help labels as well, and fix it if necessary, just because I speak Hunglish =)) I wasnot too creative during plugin integration, just used the builtin functions (or cloned them) everywhere was possible, so the integration is really fancy in my opinion. No menu item for it, settings are avail in the preferences, and global turning on/off is a statusbar icon stuff. I added source pics as well for future usage in a same way as your sources for logging pics also avail in the repo.
Attachment #8410506 - Flags: review?(bugzilla-mozilla-20020327)
Flags: needinfo?(bugzilla-mozilla-20020327)
Attached patch patch v1.0 (obsolete) — — Splinter Review
regenerate with functionnames
Attachment #8410506 - Attachment is obsolete: true
Attachment #8410506 - Flags: review?(bugzilla-mozilla-20020327)
Attached patch patch v1.0.1 (obsolete) — — Splinter Review
move updateAlertIcon() to init() from setCurrentObject() not to refresh all the time we change view
Attachment #8410703 - Attachment is obsolete: true
Attached patch patchv1.0.2 (obsolete) — — Splinter Review
some prefs prefixes were missing. Silver, this one still not for r+ , this one for textual review
Attachment #8412027 - Attachment is obsolete: true
Attached patch patch v1.1 (obsolete) — — Splinter Review
While Bug 1001204 is fixed on the trunk, I have been turning it clickable as default and the property to hidden. Now the nsIAlertsService looks well and clearly readable.
Attachment #8412589 - Attachment is obsolete: true
Attachment #8423433 - Flags: review?(bugzilla-mozilla-20020327)
Comment on attachment 8423433 [details] [diff] [review] patch v1.1 Review of attachment 8423433 [details] [diff] [review]: ----------------------------------------------------------------- Overall, I think there are a few editing errors (like double 'enabled' strings) and certainly some code needs to conform better to ChatZilla's coding style but in general it looks okay. I have no idea if the flood protector works or how it works - I have no looked at that code in detail yet. Let me know in #chatzilla if anything's unclear or you'd like help with the style guide. ::: locales/en-US/chrome/chatzilla.properties @@ +1032,5 @@ > msg.logging.icon.off = Logging is off. Click the icon to start logging this view. > msg.logging.icon.on = Logging is on. Click the icon to stop logging this view. > > +msg.alert.icon.off = Try Popup is off. Click the icon to start Try Notification. > +msg.alert.icon.on = Try Popup is on. Click the icon to stop Try Notification. Why is this called 'Try Popup' and 'Try Notification'? I don't know what those even mean. @@ +1457,5 @@ > # NOTE: "Bugzilla", "ChatZilla" and "mIRC" are product names. > pref.activityFlashDelay.label = Activity flash delay > pref.activityFlashDelay.help = When a tab that has had activity gets more activity, the tab is flashed. This preference is the length of the flash in milliseconds: 0 disables it. > +pref.alert.enabled.label = Enabled > +pref.alert.enabled.help = Tick this preference to allow alerts, or untick to turn off all alerts. Provides nothing more than a global toggle. Nit: I don't think this writing style suites preference labels. Users know that ticking it turns it on, so just explain what turning it on does, e.g. "When enabled, all alerts configured may be shown. When disabled, no alerts will be shown." I also think you mean to refer to alert.globEnabled and not alert.enabled here. @@ +1461,5 @@ > +pref.alert.enabled.help = Tick this preference to allow alerts, or untick to turn off all alerts. Provides nothing more than a global toggle. > +pref.alert.enabled.label = Enable Toaster Popup > +pref.alert.enabled.help = If you don't want popups for this view, just turn it off. > +pref.alert.nonFocusedOnly.label = Only When Non Focused > +pref.alert.nonFocusedOnly.help = If the chatzilla window on the top and focused, it won't show popups when this one checked. Is this preference genuinely useful? We have some similar rules for tab highlights and sound notifications; we should copy them and make sure it makes sense - but there doesn't seem to be a good reason to allow this level of control (I'd always want nonFocusedOnly = true). @@ +1463,5 @@ > +pref.alert.enabled.help = If you don't want popups for this view, just turn it off. > +pref.alert.nonFocusedOnly.label = Only When Non Focused > +pref.alert.nonFocusedOnly.help = If the chatzilla window on the top and focused, it won't show popups when this one checked. > +pref.alert.clickable.label = Toaster Popup is Clickable > +pref.alert.clickable.help = When this value is true, Toaster popup is clickable and focuses to the current view. And why is this needed? The alert should always be actionable (clickable) and go to the correct channel. @@ +1465,5 @@ > +pref.alert.nonFocusedOnly.help = If the chatzilla window on the top and focused, it won't show popups when this one checked. > +pref.alert.clickable.label = Toaster Popup is Clickable > +pref.alert.clickable.help = When this value is true, Toaster popup is clickable and focuses to the current view. > +pref.alert.channel.event.label = Alert for Channel Event > +pref.alert.channel.event.help = Shows poup for joins, parts, kicks, usermodes, and any other system messages. Suggested for channel moderators or for small traffic rooms. Nits: 'poup' => 'popup', 'rooms' => 'channels'. @@ +1471,5 @@ > +pref.alert.channel.chat.help = This one for following chat during doing anithing else around. It can be annoyed for a high traffic rooms. Suggested for channel moderators or for small traffic rooms. > +pref.alert.channel.stalk.label = Alert for Channel Stalk > +pref.alert.channel.stalk.help = Shows popups for messages contain stalk words. > +pref.alert.user.chat.label = Alert for Private Messages > +pref.alert.user.chat.help = The labels here aren't consistent with the sound notifications; please make them match up. That might mean changing the sound and other labels. @@ +1672,4 @@ > pref.group.general.connect.label = Connection > pref.group.general.ident.label = Identification > pref.group.general.log.label = Logging > +pref.group.general.palert.label = Try Notification 'palert'? ::: xul/content/handlers.js @@ +297,5 @@ > +function onAlertIconClick(e) > +{ > + if (e.button == 0) > + client.prefs["alert.globEnabled"]=!client.prefs["alert.globEnabled"]; > + updateAlertIcon() This isn't Python; you need braces around this to do what it looks like you wanted. Nit: Spaces either side of !=. ::: xul/content/prefs.js @@ +97,4 @@ > var prefs = > [ > ["activityFlashDelay", 200, "hidden"], > + ["alert.globEnabled", true, "hidden"], I don't really like this name. Either fully abbreviate it to alert.gEnabled or (preferably) fully expand it to alert.globalEnabled. @@ +100,5 @@ > + ["alert.globEnabled", true, "hidden"], > + ["alert.overlapDelay", 50, "hidden"], > + ["alert.floodDensity", 290, "hidden"], > + ["alert.floodDispersion",200, "hidden"], > + ["alert.enabled", true, ".palert"], I believe that you want alert.globEnabled to be visible and alert.enabled to be hidden here. It's meaningless to control the client's enabled status (the inheritance isn't useful for that but keep it). In global settings, the user will be wanting the global override. (You could stop alert.enabled from this list and make it default to true directly in the network prefs but it's not very useful to do so.) ::: xul/content/static.js @@ +215,5 @@ > + const nsIAlertsService = Components.interfaces.nsIAlertsService; > + client.alert = > + Components.classes["@mozilla.org/alerts-service;1"].getService(nsIAlertsService); > + client.alertList = new Object(); > + client.floodProtector = new floodProtectorClass(); I don't think the flood protector can cope with protecting multiple streams, so please put it in an alert-specific property. @@ +220,5 @@ > + } > + catch (ex) > + { > + dd("Alert service failed to initialize: " + ex); > + client.prefs["alert.globEnabled"] = false; This will cause alerts to be globally disabled *forever*. I'd rather you set client.alert to null and make everything check that. @@ +5112,5 @@ > + { > + if (isImportant) { > + showEventAlerts(this.TYPE, "stalk", message, nick, o, this); > + } else if (msgtype.match(/^(DCC-CHAT|DCC-FILE|INVITE|NOTICE|WALLOPS)$/)) { > + showEventAlerts("IRCUser", "chat", message, nick, o, this); This is, uh, a completely arbitrary list and not found anywhere else in ChatZilla. Please don't add this condition. @@ +5642,5 @@ > > return list; > } > + > +floodProtectorClass = function () Okay, everything from this point on needs adjusting to the CZ style guide: http://www.hacksrus.com/~ginda/pedant.html @@ +5647,5 @@ > +{ > + this.requestedTotal = 0; > + this.acceptedTotal = 0; > + this.firedTotal = 0; > + this.lastHit = (new Date()).valueOf(); That's an unusual way of doing that. The more usual ways are: Number(new Date()) +new Date() @@ +5653,5 @@ > + this.derivative1_cnt = 100; > + this.derivative2 = 0; > + this.derivative2_cnt = 0; > + > + this.request = function(){ This is definitely not how we do classes in ChatZilla. Please try and use prototypes like the other code. You can always ask me in #chatzilla if you're unsure. @@ +5692,5 @@ > + if(client.floodProtector.fire()){ > + setTimeout( > + "toasterPopupOverlapDelayReset('" + ev + "')", > + client.prefs['alert.overlapDelay'] > + ); It's considered bad form to use string-based setTimeouts. The better way of writing this is: setTimeout(toasterPopupOverlayDelayReset, client.prefs["alert.overlapDelay"], ev);
Attachment #8423433 - Flags: review?(bugzilla-mozilla-20020327) → review-
Attached patch patch v1.2 (obsolete) — — Splinter Review
See the next comment
Attachment #8423433 - Attachment is obsolete: true
Attachment #8428942 - Flags: review?(bugzilla-mozilla-20020327)
(In reply to James Ross from comment #25) > Comment on attachment 8423433 [details] [diff] [review] > patch v1.1 > > Review of attachment 8423433 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall, I think there are a few editing errors (like double 'enabled' > strings) and certainly some code needs to conform better to ChatZilla's > coding style but in general it looks okay. I have no idea if the flood > protector works or how it works - I have no looked at that code in detail > yet. > > Let me know in #chatzilla if anything's unclear or you'd like help with the > style guide. floodprotector works with 2 derives of event graph. Conception: the floodattack is always come from a script, and ircdserver bandwith also has bandwith limit which one makes the flood linear, 1st derive is constant and low value, 2nd derive is almost zero. By detecting this we should stop showing popups. This is more important than filtering it in the __display perhaps, because the nsIAlertsService is a window object, so flood equals to opening thousands of mozilla/firefox/seamonkey windows. So floodprotector is required for the stable working. > > ::: locales/en-US/chrome/chatzilla.properties > @@ +1032,5 @@ > > msg.logging.icon.off = Logging is off. Click the icon to start logging this view. > > msg.logging.icon.on = Logging is on. Click the icon to stop logging this view. > > > > +msg.alert.icon.off = Try Popup is off. Click the icon to start Try Notification. > > +msg.alert.icon.on = Try Popup is on. Click the icon to stop Try Notification. > > Why is this called 'Try Popup' and 'Try Notification'? I don't know what > those even mean. Renamed to Popup Notification > > @@ +1457,5 @@ > > # NOTE: "Bugzilla", "ChatZilla" and "mIRC" are product names. > > pref.activityFlashDelay.label = Activity flash delay > > pref.activityFlashDelay.help = When a tab that has had activity gets more activity, the tab is flashed. This preference is the length of the flash in milliseconds: 0 disables it. > > +pref.alert.enabled.label = Enabled > > +pref.alert.enabled.help = Tick this preference to allow alerts, or untick to turn off all alerts. Provides nothing more than a global toggle. > > Nit: I don't think this writing style suites preference labels. Users know > that ticking it turns it on, so just explain what turning it on does, e.g. > "When enabled, all alerts configured may be shown. When disabled, no alerts > will be shown." fixed > > I also think you mean to refer to alert.globEnabled and not alert.enabled > here. not. It was a dupe. But now I used it for globalEnabled > > @@ +1461,5 @@ > > +pref.alert.enabled.help = Tick this preference to allow alerts, or untick to turn off all alerts. Provides nothing more than a global toggle. > > +pref.alert.enabled.label = Enable Toaster Popup > > +pref.alert.enabled.help = If you don't want popups for this view, just turn it off. > > +pref.alert.nonFocusedOnly.label = Only When Non Focused > > +pref.alert.nonFocusedOnly.help = If the chatzilla window on the top and focused, it won't show popups when this one checked. > > Is this preference genuinely useful? We have some similar rules for tab > highlights and sound notifications; we should copy them and make sure it > makes sense - but there doesn't seem to be a good reason to allow this level > of control (I'd always want nonFocusedOnly = true). As I've said in the chat, this is the most useful stuff in the popup system. It requires some explanation: When Chatzilla is on focus, it NEVER shows popup from the current view, this is important. This feature is for following paralel the other chats while I'm on an active channel. This is the most useful thing, i've did because I don't need to click all the time to other channels I follow or moderate, because the popup shows what happens there. I go there exclusively when necessary. But true, not everybody likes getting popup when he is on focus, so this property is for handling this issue. I would like to keep this one, because useful. Default can be true, or a better name would be kewl, but I have no idea for renaming it. tooltip will explain wtf it is. > > @@ +1463,5 @@ > > +pref.alert.enabled.help = If you don't want popups for this view, just turn it off. > > +pref.alert.nonFocusedOnly.label = Only When Non Focused > > +pref.alert.nonFocusedOnly.help = If the chatzilla window on the top and focused, it won't show popups when this one checked. > > +pref.alert.clickable.label = Toaster Popup is Clickable > > +pref.alert.clickable.help = When this value is true, Toaster popup is clickable and focuses to the current view. > > And why is this needed? The alert should always be actionable (clickable) > and go to the correct channel. for backwards compatibility reasons I plan to keep this property but as a hidden. Reason: https://bug1001204.bugzilla.mozilla.org/attachment.cgi?id=8412230 When I saw this first I got stroke. o_O And this is worse before firefox26. After fixing Bug 1001204 it will get new style from firefox32: https://bug1001204.bugzilla.mozilla.org/attachment.cgi?id=8417732 which one is normally readable. Note that, I use it to follow the chats. So. Now this is true and hidden property from now, but kept as property do disable via console for the previous versions of Gecko to make it readable normally. > > @@ +1465,5 @@ > > +pref.alert.nonFocusedOnly.help = If the chatzilla window on the top and focused, it won't show popups when this one checked. > > +pref.alert.clickable.label = Toaster Popup is Clickable > > +pref.alert.clickable.help = When this value is true, Toaster popup is clickable and focuses to the current view. > > +pref.alert.channel.event.label = Alert for Channel Event > > +pref.alert.channel.event.help = Shows poup for joins, parts, kicks, usermodes, and any other system messages. Suggested for channel moderators or for small traffic rooms. > > Nits: 'poup' => 'popup', 'rooms' => 'channels'. Fixed > > @@ +1471,5 @@ > > +pref.alert.channel.chat.help = This one for following chat during doing anithing else around. It can be annoyed for a high traffic rooms. Suggested for channel moderators or for small traffic rooms. > > +pref.alert.channel.stalk.label = Alert for Channel Stalk > > +pref.alert.channel.stalk.help = Shows popups for messages contain stalk words. > > +pref.alert.user.chat.label = Alert for Private Messages > > +pref.alert.user.chat.help = > > The labels here aren't consistent with the sound notifications; please make > them match up. That might mean changing the sound and other labels. Labels are syncronised with sound labels, I mean I renamed the sound labels as we talked in the chat. > > @@ +1672,4 @@ > > pref.group.general.connect.label = Connection > > pref.group.general.ident.label = Identification > > pref.group.general.log.label = Logging > > +pref.group.general.palert.label = Try Notification > > 'palert'? Yup, palert. (like popup alert) The prefmanager works in alphabetical order and I didnot want these settings at the top of General tab, rather I wanted to see it below the logging configuration. So a < l < p means palert is the last item (try is renamed and new item added for global config) > > ::: xul/content/handlers.js > @@ +297,5 @@ > > +function onAlertIconClick(e) > > +{ > > + if (e.button == 0) > > + client.prefs["alert.globEnabled"]=!client.prefs["alert.globEnabled"]; > > + updateAlertIcon() > > This isn't Python; you need braces around this to do what it looks like you > wanted. > > Nit: Spaces either side of !=. fixed with spaces: client.prefs["alert.globEnabled"] = !client.prefs["alert.globEnabled"]; this is a switch method "a let be not a" feature. > > ::: xul/content/prefs.js > @@ +97,4 @@ > > var prefs = > > [ > > ["activityFlashDelay", 200, "hidden"], > > + ["alert.globEnabled", true, "hidden"], > > I don't really like this name. Either fully abbreviate it to alert.gEnabled > or (preferably) fully expand it to alert.globalEnabled. Fixed, renamed to globalEnabled > > @@ +100,5 @@ > > + ["alert.globEnabled", true, "hidden"], > > + ["alert.overlapDelay", 50, "hidden"], > > + ["alert.floodDensity", 290, "hidden"], > > + ["alert.floodDispersion",200, "hidden"], > > + ["alert.enabled", true, ".palert"], > > I believe that you want alert.globEnabled to be visible and alert.enabled to > be hidden here. It's meaningless to control the client's enabled status (the > inheritance isn't useful for that but keep it). In global settings, the user > will be wanting the global override. > > (You could stop alert.enabled from this list and make it default to true > directly in the network prefs but it's not very useful to do so.) I left both of them as general for a better configuration way. The enabled is based on inheritance, but globalEnable is permanent. Situtaion: what happens if somebody on 5-6 networks 50-60 channels, but wants popup only from 1 room where he is the moderator? Why he need to stop it for all of networks manually, its easier to turn it off at General, and turning on only on that 1 channel. > > ::: xul/content/static.js > @@ +215,5 @@ > > + const nsIAlertsService = Components.interfaces.nsIAlertsService; > > + client.alert = > > + Components.classes["@mozilla.org/alerts-service;1"].getService(nsIAlertsService); > > + client.alertList = new Object(); > > + client.floodProtector = new floodProtectorClass(); > > I don't think the flood protector can cope with protecting multiple streams, > so please put it in an alert-specific property. Fixed, all of alert specific stuff went under client.alert such as: client.alert.service client.alert.alertList client.alert.floodProtector > > @@ +220,5 @@ > > + } > > + catch (ex) > > + { > > + dd("Alert service failed to initialize: " + ex); > > + client.prefs["alert.globEnabled"] = false; > > This will cause alerts to be globally disabled *forever*. I'd rather you set > client.alert to null and make everything check that. > Fixed. client.alert = null, existing test at __display > @@ +5112,5 @@ > > + { > > + if (isImportant) { > > + showEventAlerts(this.TYPE, "stalk", message, nick, o, this); > > + } else if (msgtype.match(/^(DCC-CHAT|DCC-FILE|INVITE|NOTICE|WALLOPS)$/)) { > > + showEventAlerts("IRCUser", "chat", message, nick, o, this); > > This is, uh, a completely arbitrary list and not found anywhere else in > ChatZilla. Please don't add this condition. Removed (however wasnot a bad idea :) ) > > @@ +5642,5 @@ > > > > return list; > > } > > + > > +floodProtectorClass = function () > > Okay, everything from this point on needs adjusting to the CZ style guide: > http://www.hacksrus.com/~ginda/pedant.html everything is pedantly fixed > > @@ +5647,5 @@ > > +{ > > + this.requestedTotal = 0; > > + this.acceptedTotal = 0; > > + this.firedTotal = 0; > > + this.lastHit = (new Date()).valueOf(); > > That's an unusual way of doing that. The more usual ways are: > Number(new Date()) > +new Date() replaced with Number(new Date()) everywhere. > > @@ +5653,5 @@ > > + this.derivative1_cnt = 100; > > + this.derivative2 = 0; > > + this.derivative2_cnt = 0; > > + > > + this.request = function(){ > > This is definitely not how we do classes in ChatZilla. Please try and use > prototypes like the other code. You can always ask me in #chatzilla if > you're unsure. prototype way implemented as I showed in the chat. > > @@ +5692,5 @@ > > + if(client.floodProtector.fire()){ > > + setTimeout( > > + "toasterPopupOverlapDelayReset('" + ev + "')", > > + client.prefs['alert.overlapDelay'] > > + ); > > It's considered bad form to use string-based setTimeouts. The better way of > writing this is: > setTimeout(toasterPopupOverlayDelayReset, > client.prefs["alert.overlapDelay"], ev); function-pointer format added in 2 places in the code where happened. r? again at Silver for the next review. Note that my English is poor, so I adopt any correction of my English at tooltip sentences.
Comment on attachment 8428942 [details] [diff] [review] patch v1.2 Review of attachment 8428942 [details] [diff] [review]: ----------------------------------------------------------------- This is much better, thanks! There's still a few code style nits. You don't have to fix them but it'll make my life easier. :) The main this is that I've come to the conclusion this feature should be called "Message Notifications" so if you could update that everywhere that'd be great. There's a few questions and small issues with the code itself (e.g. handler.js) but not much. Nearly there. ::: locales/en-US/chrome/chatzilla.properties @@ +1032,4 @@ > msg.logging.icon.off = Logging is off. Click the icon to start logging this view. > msg.logging.icon.on = Logging is on. Click the icon to stop logging this view. > > +msg.alert.icon.off = Popup Notification is off. Click the icon to start Popup Notification. "Message notifications are off. Click the icon to start showing notifications for new messages." @@ +1032,5 @@ > msg.logging.icon.off = Logging is off. Click the icon to start logging this view. > msg.logging.icon.on = Logging is on. Click the icon to stop logging this view. > > +msg.alert.icon.off = Popup Notification is off. Click the icon to start Popup Notification. > +msg.alert.icon.on = Popup Notification is on. Click the icon to stop Popup Notification. "Message notifications are on. Click the icon to stop showing notifications for new messages." @@ +1457,4 @@ > # NOTE: "Bugzilla", "ChatZilla" and "mIRC" are product names. > pref.activityFlashDelay.label = Activity flash delay > pref.activityFlashDelay.help = When a tab that has had activity gets more activity, the tab is flashed. This preference is the length of the flash in milliseconds: 0 disables it. > +pref.alert.globalEnabled.label = Global Enable/Disable "Enabled (global)"? "Globally enabled"? @@ +1458,5 @@ > pref.activityFlashDelay.label = Activity flash delay > pref.activityFlashDelay.help = When a tab that has had activity gets more activity, the tab is flashed. This preference is the length of the flash in milliseconds: 0 disables it. > +pref.alert.globalEnabled.label = Global Enable/Disable > +pref.alert.globalEnabled.help = When enabled, all alerts configured may be shown. When disabled, no alerts will be shown. Provides nothing more than a global toggle. > +pref.alert.enabled.label = Enable Popup Notification "Enable" since we're inside a group box that tells you this is for Popup Notifications. @@ +1460,5 @@ > +pref.alert.globalEnabled.label = Global Enable/Disable > +pref.alert.globalEnabled.help = When enabled, all alerts configured may be shown. When disabled, no alerts will be shown. Provides nothing more than a global toggle. > +pref.alert.enabled.label = Enable Popup Notification > +pref.alert.enabled.help = When enabled, popups are shown for this view. > +pref.alert.nonFocusedOnly.label = Only When Non Focused "Only when window not active" @@ +1461,5 @@ > +pref.alert.globalEnabled.help = When enabled, all alerts configured may be shown. When disabled, no alerts will be shown. Provides nothing more than a global toggle. > +pref.alert.enabled.label = Enable Popup Notification > +pref.alert.enabled.help = When enabled, popups are shown for this view. > +pref.alert.nonFocusedOnly.label = Only When Non Focused > +pref.alert.nonFocusedOnly.help = If the chatzilla window on the top and focused, it won't show popups when this one checked. When it is unchecked, alerts will be shown from the other channels. Unchecking is suggested for channel moderators or for small traffic channels. "When enabled, all message notifications are supressed when the window is active. Otherwise, message notifications for non-active views will be shown." @@ +1467,5 @@ > +pref.alert.channel.event.help = Shows popup for joins, parts, kicks, usermodes, and any other system messages. Suggested for channel moderators or for small traffic channels. > +pref.alert.channel.chat.label = Alert for Channel Chat > +pref.alert.channel.chat.help = This one for following chat during doing anithing else around. It can be annoyed for a high traffic rooms. Suggested for channel moderators or for small traffic channels. > +pref.alert.channel.stalk.label = Alert for Channel Stalk > +pref.alert.channel.stalk.help = Shows popups for messages contain stalk words. "containing" @@ +1469,5 @@ > +pref.alert.channel.chat.help = This one for following chat during doing anithing else around. It can be annoyed for a high traffic rooms. Suggested for channel moderators or for small traffic channels. > +pref.alert.channel.stalk.label = Alert for Channel Stalk > +pref.alert.channel.stalk.help = Shows popups for messages contain stalk words. > +pref.alert.user.chat.label = Alert for Private Messages > +pref.alert.user.chat.help = "Shows notifications for private messages." @@ +1670,4 @@ > pref.group.general.connect.label = Connection > pref.group.general.ident.label = Identification > pref.group.general.log.label = Logging > +pref.group.general.palert.label = Popup Notification "Message notifications" ::: xul/content/handlers.js @@ +297,5 @@ > +function onAlertIconClick(e) > +{ > + if (e.button == 0) > + client.prefs["alert.globalEnabled"] = !client.prefs["alert.globalEnabled"]; > + updateAlertIcon(); You didn't add the braces around these two lines. Please put the braces in! ::: xul/content/prefs.js @@ +98,5 @@ > [ > ["activityFlashDelay", 200, "hidden"], > + ["alert.overlapDelay", 50, "hidden"], > + ["alert.floodDensity", 290, "hidden"], > + ["alert.floodDispersion",200, "hidden"], Do we have a reasonable English description of what this 290 and 200? I'd just like some idea of what they mean (beyond the precise derivatives). @@ +102,5 @@ > + ["alert.floodDispersion",200, "hidden"], > + ["alert.enabled", true, ".palert"], > + ["alert.globalEnabled",true, ".palertconfig"], > + ["alert.clickable", true, "hidden"], > + ["alert.nonFocusedOnly",true, ".palertconfig"], I'm thinking the two .palertconfig preferences might just be better going in to "global". ::: xul/content/static.js @@ +211,4 @@ > dd("Sound failed to initialize: " + ex); > } > > + try { Nit: ChatZilla style is braces on a new line. @@ +5102,5 @@ > + > + /* We want to show alerts if they're from a non-current view (optional), > + * or we don't have focus at all. > + */ > + if ( client.prefs["alert.globalEnabled"] Nit: No space after opening parenthesis. (And on following lines.) @@ +5647,5 @@ > > return list; > } > + > +FloodProtector = function () {}; Nit: The CZ style is: function FloodProtector() { } @@ +5652,5 @@ > + > +FloodProtector.prototype.requestedTotal = 0; > +FloodProtector.prototype.acceptedTotal = 0; > +FloodProtector.prototype.firedTotal = 0; > +FloodProtector.prototype.lastHit = Number(new Date()); I don't think this is what you want. If you want lastHit to be the time the FloodProtector was created, you should set it to 0 here and then set this.lastHit in the constructor above. @@ +5654,5 @@ > +FloodProtector.prototype.acceptedTotal = 0; > +FloodProtector.prototype.firedTotal = 0; > +FloodProtector.prototype.lastHit = Number(new Date()); > +FloodProtector.prototype.derivative1 = 100; > +FloodProtector.prototype.derivative1_cnt = 100; Nit: Rename 'cnt' to something. If it is short for 'count', please use 'count' and do it in this style: derivative1Count. @@ +5662,5 @@ > +FloodProtector.prototype.request = function () > +{ > + this.requestedTotal++; > + var current = Number(new Date()); > + var old_der1 = this.derivative1; Let's remove as many of the underscores and abbreviations as we can. This should be oldDerivative1. @@ +5664,5 @@ > + this.requestedTotal++; > + var current = Number(new Date()); > + var old_der1 = this.derivative1; > + this.derivative1 = current - this.lastHit; > + this.derivative1_cnt = ((this.derivative1_cnt*9) + this.derivative1) / 10; Nit: Spaces around * @@ +5680,5 @@ > +{ > + // There is no activity for 10 seconds - flood is possibly done. > + // No need more recall. In other way the first normal activity > + // overwrites it automatically earlier, if nessesary. > + if((Number(new Date()) - this.lastHit) > 10000) return false; Nit: 'return false;' on a new line, indented. @@ +5685,5 @@ > + > + // The activity is not too frequent or not massive so should not be fire. > + if((this.derivative1_cnt > client.prefs['alert.floodDensity']) > + || (this.derivative2_cnt > client.prefs['alert.floodDispersion'])) > + return false; Nit: Braces around 'return false;' because if condition is more than 1 line. @@ +5696,5 @@ > + > +toasterPopupOverlapDelayReset = function (ev) > +{ > + // it smells like a flood attack so rather wait more... > + if(client.alert.floodProtector.fire()) Nit: Space before opening parenthesis. @@ +5700,5 @@ > + if(client.alert.floodProtector.fire()) > + { > + setTimeout( > + toasterPopupOverlapDelayReset, > + client.prefs['alert.overlapDelay'],ev); Nit: setTimeout(toasterPopupOverlapDelayReset, client.prefs['alert.overlapDelay'], ev); @@ +5708,5 @@ > + delete client.alert.alertList[ev]; > + } > +} > + > +alertClickerObserver = { Nit: var @@ +5711,5 @@ > + > +alertClickerObserver = { > + observe: function(subject, topic, data) > + { > + if(topic == "alertclickcallback") Nit: Space before open parenthesis. @@ +5714,5 @@ > + { > + if(topic == "alertclickcallback") > + { > + var tb = document.getElementById(data); > + if(tb && tb.view) Nit: And again. :) @@ +5716,5 @@ > + { > + var tb = document.getElementById(data); > + if(tb && tb.view) > + { > + tb.view.dispatch("set-current-view",{view: tb.view}); Nit: And again, after comma. @@ +5733,5 @@ > + window.focus(); > + } > + }, > + > + onAlertFinished: function(data){ Nit: Open brace on new line. @@ +5739,5 @@ > +} > + > + > +// Show the alert for a particular event on a type of object. > +showEventAlerts = function(type, event, message, nick, o, thisp) function showEventAlerts(type, event, message, nick, o, thisp) @@ +5793,5 @@ > + message, clickable, tabId, listener); > + } > + catch(ex) > + { > + // yup. it is a MAC Nit: Indent level is wrong.
Attachment #8428942 - Flags: review?(bugzilla-mozilla-20020327) → review-
Attached patch patch v1.3 (obsolete) — — Splinter Review
All nits fixed in order, comment added in prefs.js what the floodprotector values are mean, and how it works based on those default settings.
Attachment #8428942 - Attachment is obsolete: true
Attachment #8432245 - Flags: review?(bugzilla-mozilla-20020327)
Attachment #8432245 - Attachment is obsolete: true
Attachment #8432245 - Flags: review?(bugzilla-mozilla-20020327)
Attached patch patch v1.3 (obsolete) — — Splinter Review
Attachment #8432255 - Flags: review?(bugzilla-mozilla-20020327)
Attachment #8432255 - Attachment is obsolete: true
Attachment #8432255 - Flags: review?(bugzilla-mozilla-20020327)
Attached patch popup.txt (obsolete) — — Splinter Review
affected files: chatzilla.properties jar.mn menus.xul chatzilla.css prefs.js static.js handlers.js newly added files: spbubble-on.png (icon) spbubble-off.png (icon) spbubble-on.png (source) spbubble-off.png (source) test ok, all nits fixed.
Attachment #8432264 - Flags: review?(bugzilla-mozilla-20020327)
Sorry for the too much replaces, but you gave me an idea to add special style for the ACTION messages to look as real /me stuff :)
Attached patch patch v1.3.1 (obsolete) — — Splinter Review
Attachment #8432264 - Attachment is obsolete: true
Attachment #8432264 - Flags: review?(bugzilla-mozilla-20020327)
Attachment #8432293 - Flags: review?(bugzilla-mozilla-20020327)
Attached patch patch v1.3.2 (obsolete) — — Splinter Review
Per discuss on chat just make the class FloodProtector independent from the popup preferences. Now preference values pass to constructor. By this modify the floodprotector will be good for Bug 280398 too for future usage.
Attachment #8432293 - Attachment is obsolete: true
Attachment #8432293 - Flags: review?(bugzilla-mozilla-20020327)
Attachment #8433263 - Flags: review?(bugzilla-mozilla-20020327)
Comment on attachment 8433263 [details] [diff] [review] patch v1.3.2 Review of attachment 8433263 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for persevering! This patch looks very good - just a few small corrections, mostly to the English text. r=silver with nits fixed If you're happy with all the nits, please attach an updated patch but just set review+ instead of review? - you only need to ask for another review if you change something other than the nits. After that, we're ready to get it committed. :) ::: locales/en-US/chrome/chatzilla.properties @@ +1458,5 @@ > pref.activityFlashDelay.label = Activity flash delay > pref.activityFlashDelay.help = When a tab that has had activity gets more activity, the tab is flashed. This preference is the length of the flash in milliseconds: 0 disables it. > +pref.alert.globalEnabled.label = Globally enabled > +pref.alert.globalEnabled.help = When enabled, all alerts configured may be shown. When disabled, no alerts will be shown. Provides nothing more than a global toggle. > +pref.alert.enabled.label = Enable Nit: "Enabled"? (Sorry, my mistake from earlier.) @@ +1463,5 @@ > +pref.alert.enabled.help = When enabled, popups are shown for this view. > +pref.alert.nonFocusedOnly.label = Only when window not active > +pref.alert.nonFocusedOnly.help = When enabled, all message notifications are supressed when the window is active. Otherwise, message notifications for non-active views will be shown. Unchecking is suggested for channel moderators or for small traffic channels. > +pref.alert.channel.event.label = Alert for Channel Event > +pref.alert.channel.event.help = Shows popup for joins, parts, kicks, usermodes, and any other system messages. Suggested for channel moderators or for small traffic channels. Nit: s/popup/message notifications/ Nit: s/small traffic/low traffic/ @@ +1465,5 @@ > +pref.alert.nonFocusedOnly.help = When enabled, all message notifications are supressed when the window is active. Otherwise, message notifications for non-active views will be shown. Unchecking is suggested for channel moderators or for small traffic channels. > +pref.alert.channel.event.label = Alert for Channel Event > +pref.alert.channel.event.help = Shows popup for joins, parts, kicks, usermodes, and any other system messages. Suggested for channel moderators or for small traffic channels. > +pref.alert.channel.chat.label = Alert for Channel Chat > +pref.alert.channel.chat.help = This one for following chat during doing anithing else around. It can be annoyed for a high traffic rooms. Suggested for channel moderators or for small traffic channels. Nit: "Show message notifications for normal chat messages. It may be annoying for high traffic channels. Suggested for moderators or for low traffic channels." @@ +1467,5 @@ > +pref.alert.channel.event.help = Shows popup for joins, parts, kicks, usermodes, and any other system messages. Suggested for channel moderators or for small traffic channels. > +pref.alert.channel.chat.label = Alert for Channel Chat > +pref.alert.channel.chat.help = This one for following chat during doing anithing else around. It can be annoyed for a high traffic rooms. Suggested for channel moderators or for small traffic channels. > +pref.alert.channel.stalk.label = Alert for Channel Stalk > +pref.alert.channel.stalk.help = Shows popups for messages containing stalk words. Nit: s/popups/message notifications/ @@ +1468,5 @@ > +pref.alert.channel.chat.label = Alert for Channel Chat > +pref.alert.channel.chat.help = This one for following chat during doing anithing else around. It can be annoyed for a high traffic rooms. Suggested for channel moderators or for small traffic channels. > +pref.alert.channel.stalk.label = Alert for Channel Stalk > +pref.alert.channel.stalk.help = Shows popups for messages containing stalk words. > +pref.alert.user.chat.label = Alert for Private Messages Nit: You used "Sound for User Chat" below so this one should be "Alert for User Chat". @@ +1469,5 @@ > +pref.alert.channel.chat.help = This one for following chat during doing anithing else around. It can be annoyed for a high traffic rooms. Suggested for channel moderators or for small traffic channels. > +pref.alert.channel.stalk.label = Alert for Channel Stalk > +pref.alert.channel.stalk.help = Shows popups for messages containing stalk words. > +pref.alert.user.chat.label = Alert for Private Messages > +pref.alert.user.chat.help = Shows notifications for private messages. Nit: "Shows message notifications for private messages." @@ +1670,5 @@ > pref.group.general.connect.label = Connection > pref.group.general.ident.label = Identification > pref.group.general.log.label = Logging > +pref.group.general.palert.label = Message notifications > +pref.group.global.palertconfig.label = Message notifications Configuration Nit: "Message notifications configuration" ::: xul/content/prefs.js @@ +98,5 @@ > [ > ["activityFlashDelay", 200, "hidden"], > + ["alert.overlapDelay", 50, "hidden"], > + ["alert.floodDensity", 290, "hidden"], > + ["alert.floodDispersion",200, "hidden"], Nit: Missing space after first comma. @@ +102,5 @@ > + ["alert.floodDispersion",200, "hidden"], > + ["alert.enabled", true, ".palert"], > + ["alert.globalEnabled",true, "global.palertconfig"], > + ["alert.clickable", true, "hidden"], > + ["alert.nonFocusedOnly",true, "global.palertconfig"], Nit: Space after first comma is missing (and will fix alignment of third item). ::: xul/content/static.js @@ +5723,5 @@ > + > +} > + > + > +toasterPopupOverlapDelayReset = function (ev) Nit: This should be "function toasterPopupOverlapDelayReset(e)". @@ +5766,5 @@ > + > + onAlertFinished: function(data) > + { > + } > +} Nit: Semi-colon after this close-brace. @@ +5831,5 @@ > + { > + client.alert.service.showAlertNotification( > + "chrome://chatzilla/skin/images/logo.png", > + "ChatZilla - " + source + " - " + event, > + message, clickable, tabId, listener); Nit: These 3 lines should be indented to show they are part of the showAlertNotification function call. @@ +5835,5 @@ > + message, clickable, tabId, listener); > + } > + catch(ex) > + { > + // yup. it is a MAC Heh, who knows why it failed. :)
Attachment #8433263 - Flags: review?(bugzilla-mozilla-20020327) → review+
Attached patch patch v1.4.0 — — Splinter Review
Nits Fixed
Attachment #8433263 - Attachment is obsolete: true
Attachment #8443712 - Flags: review+
Flags: needinfo?(bugzilla-mozilla-20000923)
http://hg.mozilla.org/chatzilla/rev/c1c6ff40a356
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [cz-0.9.91]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: