Closed Bug 1127802 Opened 10 years ago Closed 10 years ago

Multiple directed chat messages should show as single bundled notification

Categories

(Thunderbird :: Instant Messaging, defect)

Product:
Thunderbird
Component:
Instant Messaging
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 38.0

People

(Reporter: sshagarwal, Assigned: sshagarwal)

References

Details

Attachments

(1 file, 8 obsolete files)

Patch for check-in
11.23 KB, patch
sshagarwal
: review+
sshagarwal
: ui-review+
Details | Diff | Splinter Review
Currently, if multiple chat messages from a person are arriving continuously, desktop notifications keep coming up. We should bundle the messages and show a single notification if the messages arrive within a short time interval (~1-2 secs? this can be decided).
Attached patch Patch v1 (obsolete) — — Splinter Review
This is very basic patch. Only the last message from the sender will be shown after the timeout. This is just FTR.
Attached patch Patch v2 (obsolete) — — Splinter Review
Okay, this does what I expect it to do. Notification will not be displayed before a 2 second timer expires after sender stops sending messages. An ellipsis will be prepended to the last message to show that there are messages before the one being shown in the notification. In the same timer window, if another sender sends a message, current timer will be stopped, the notification for the previous sender will be shown and this new sender's timer will start. Thanks.
Attachment #8557442 - Attachment is obsolete: true
Attachment #8557455 - Flags: review?(aleth)
Component: General → Instant Messaging
Product: Chat Core → Thunderbird
Version: trunk → Trunk
Comment on attachment 8557455 [details] [diff] [review] Patch v2 There was no ui-review option in Chat Core:General
Attachment #8557455 - Flags: ui-review?(richard.marti)
Comment on attachment 8557455 [details] [diff] [review] Patch v2 This looks already good. With sound enabled, it still plays it for every message. This should be combined with the popup. Instead of showing the last message with preceding ellipsis, could you only show something like a "5 Messages"? I think it doesn't make sense to show the last message without knowing the context of the previous messages and a the count of new messages would be better.
Attachment #8557455 - Flags: ui-review?(richard.marti) → feedback+
Doesn't it make more sense to show the *first* message (which is the one that the user will have more context about if they are in the middle of the conversation) and then display "...x more" or something at the end?
(In reply to Patrick Cloke [:clokep] from comment #5) > Doesn't it make more sense to show the *first* message Right. If what we want to do is to throttle the notifications, why can't we just remember the timestamp of the last message that caused a notification, and skip new notifications for messages from the same sender if the previous notification wasn't long enough ago? (this solution wouldn't need any timer)
Attached patch Patch v3 (obsolete) — — Splinter Review
Okay, this looks good to me :) Thanks.
Attachment #8557455 - Attachment is obsolete: true
Attachment #8557455 - Flags: review?(aleth)
Attachment #8557758 - Flags: feedback?(richard.marti)
Attachment #8557758 - Flags: feedback?(aleth)
Comment on attachment 8557758 [details] [diff] [review] Patch v3 Review of attachment 8557758 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/im/modules/chatNotifications.jsm @@ +25,5 @@ > + // Holds previous direct-message and a message counter (count of the > + // messages that haven't been a part of any notification). > + _prevMessage: null, > + > + _showMessageNotification: function () { Couldn't this function keep its aMessage parameter unmodified and just have an additional optional parameter for the counter? @@ +29,5 @@ > + _showMessageNotification: function () { > + Services.obs.notifyObservers(this._prevMessage.msgObject, "play-notification-sound", false); > + > + // If TB window has focus, there's no need to show the notification.. > + if (Services.wm.getMostRecentWindow("mail:3pane").document.hasFocus()) Are you sure of this? It seems to be that if the Chat tab is not open (or even not selected) it's easy to miss chat messages. @@ +123,5 @@ > + if (!incrementDone) { > + this._prevMessage = new Object(); > + this._prevMessage.msgObject = aSubject; > + this._prevMessage.msgCounter = 0; > + this._timer.initWithCallback( Isn't the timer needed only when receiving the second message from the same sender? @@ +125,5 @@ > + this._prevMessage.msgObject = aSubject; > + this._prevMessage.msgCounter = 0; > + this._timer.initWithCallback( > + function() { > + Notifications._showMessageNotification() We want to do this immediately when receiving the first message. The edge case of someone annoyingly sending plenty of messages at once isn't a reason to wait 2s to notify of a new IM in the general case.
Comment on attachment 8557758 [details] [diff] [review] Patch v3 Sorry to go back one step, but I think like Florian, the notification should appear immediately. This makes it not possible to show the count of the following messages but this is not really needed. When you go to the chat you can see the total number of missed messages.
Attachment #8557758 - Flags: feedback?(richard.marti) → feedback+
(In reply to Florian Quèze [:florian] [:flo] from comment #8) > Comment on attachment 8557758 [details] [diff] [review] > Patch v3 > @@ +29,5 @@ > > + _showMessageNotification: function () { > > + Services.obs.notifyObservers(this._prevMessage.msgObject, "play-notification-sound", false); > > + > > + // If TB window has focus, there's no need to show the notification.. > > + if (Services.wm.getMostRecentWindow("mail:3pane").document.hasFocus()) > > Are you sure of this? It seems to be that if the Chat tab is not open (or > even not selected) it's easy to miss chat messages. But we are already doing this. At the end, even after creating the notification, we don't show it if we are at the TB window. I just copied it up to avoid unnecessary notification generation in that case. > @@ +123,5 @@ > > + if (!incrementDone) { > > + this._prevMessage = new Object(); > > + this._prevMessage.msgObject = aSubject; > > + this._prevMessage.msgCounter = 0; > > + this._timer.initWithCallback( > > Isn't the timer needed only when receiving the second message from the same > sender? > > @@ +125,5 @@ > > + this._prevMessage.msgObject = aSubject; > > + this._prevMessage.msgCounter = 0; > > + this._timer.initWithCallback( > > + function() { > > + Notifications._showMessageNotification() > > We want to do this immediately when receiving the first message. The edge > case of someone annoyingly sending plenty of messages at once isn't a reason > to wait 2s to notify of a new IM in the general case. (In reply to Richard Marti (:Paenglab) from comment #9) > Comment on attachment 8557758 [details] [diff] [review] > Patch v3 > > Sorry to go back one step, but I think like Florian, the notification should > appear immediately. This makes it not possible to show the count of the > following messages but this is not really needed. When you go to the chat > you can see the total number of missed messages. Sorry but I don't understand the last two parts. What do we want to do immediately? And, if we are showing the messages instantly, IMHO this bug has no meaning. Sorry but I am unable to figure out what is wanted in the review? Is it that the first message should be notified immediately? And from then onwards, if any other messages arrive instantly, their notification should be deferred?
(In reply to Suyash Agarwal (:sshagarwal) from comment #10) > Sorry but I don't understand the last two parts. What do we want to do > immediately? And, if we are > showing the messages instantly, IMHO this bug has no meaning. Sorry but I am > unable to figure out what is wanted in the review? > Is it that the first message should be notified immediately? And from then > onwards, if any other messages arrive instantly, their notification should > be deferred? The notification for the first message should happen immediately, yes. For the next messages, I think comment 9 wasn't suggesting exactly the same thing as I was. I was suggesting that when the second message arrive you start your timer, and start counting the messages. I think Paenglab was suggesting you could drop completely all the notifications for messages arriving soon after the first one.
(In reply to Florian Quèze [:florian] [:flo] from comment #11) > (In reply to Suyash Agarwal (:sshagarwal) from comment #10) > > > Sorry but I don't understand the last two parts. What do we want to do > > immediately? And, if we are > > showing the messages instantly, IMHO this bug has no meaning. Sorry but I am > > unable to figure out what is wanted in the review? > > Is it that the first message should be notified immediately? And from then > > onwards, if any other messages arrive instantly, their notification should > > be deferred? > > The notification for the first message should happen immediately, yes. > > For the next messages, I think comment 9 wasn't suggesting exactly the same > thing as I was. I was suggesting that when the second message arrive you > start your timer, and start counting the messages. I think Paenglab was > suggesting you could drop completely all the notifications for messages > arriving soon after the first one. What I meant is: With your actual patch the notification comes after around 2 secs with "message text - 2 more messages". When the notification comes immediately, you can't add how many messages are skipped in the notification.
Attached patch Patch v4 (obsolete) — — Splinter Review
Okay I think this is as per the expectations. The first message is displayed as it arrives. If another message from the same sender comes before 2 seconds from this message, it is held and the timer is reset to start from now.. and in such a manner, next notification will occur only after sender has sent whatever he intends to and has waited for at least 2 seconds. If another message from another sender comes before 2 seconds from the last (held/unheld) message, the held message (if any) will be displayed and the new message from the other sender will also be displayed instantly. If, while the messages are being held, user opens the TB window, notification timer is cancelled and held notifications are discarded. Thanks.
Attachment #8557758 - Attachment is obsolete: true
Attachment #8557758 - Flags: feedback?(aleth)
Attachment #8559367 - Flags: ui-review?(richard.marti)
Attachment #8559367 - Flags: feedback?(florian)
Comment on attachment 8559367 [details] [diff] [review] Patch v4 (In reply to Suyash Agarwal (:sshagarwal) from comment #13) > Created attachment 8559367 [details] [diff] [review] > Patch v4 > > Okay I think this is as per the expectations. > > The first message is displayed as it arrives. > If another message from the same sender comes before 2 seconds from this > message, > it is held and the timer is reset to start from now.. and in such a manner, > next notification will occur only after sender has sent whatever he intends > to and has waited for at least 2 seconds. > If another message from another sender comes before 2 seconds from the last > (held/unheld) message, the held message (if any) will be displayed and the > new message from the other sender will also be displayed instantly. > If, while the messages are being held, user opens the TB window, > notification timer is cancelled and held notifications are discarded. I think we should try this.
Attachment #8559367 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 8559367 [details] [diff] [review] Patch v4 Review of attachment 8559367 [details] [diff] [review]: ----------------------------------------------------------------- Suyash asked me to change this flag to aleth.
Attachment #8559367 - Flags: feedback?(florian) → feedback?(aleth)
Comment on attachment 8559367 [details] [diff] [review] Patch v4 Review of attachment 8559367 [details] [diff] [review]: ----------------------------------------------------------------- I had actually started looking at this yesterday and got distracted. Publishing the draft I had. ::: mail/components/im/modules/chatNotifications.jsm @@ +73,5 @@ > + ((aCounter > 0) ? "" : this.ellipsis); > + > + if (aCounter > 0) { > + let moreMsgs = bundle.getString("more"); > + messageText = messageText + " " + this.ellipsis + I'm not sure what that " " is for here. The localization note for the 'more' string should be more specific about how it will be used. Probably show an example. It should at least be clear to localizers that there will be a message (or the beginning of a message) followed by an ellipsis (why isn't the ellipsis part of the localized string actually?). I also wonder if rtl locales would want to have the string constructed the other way, with the more message first and the text of the message after it; if so the whole string should be a formatted string. ::: mail/locales/en-US/chrome/messenger/chat.properties @@ +93,5 @@ > log.previousWeek=Last Week > > messagePreview=New Chat Message > + > +#LOCALIZATION NOTE The localization note needs to explicitly say this is a localized plural string. Look at other existing plural strings and copy the comment that's above one of them (it's always the same comment, and some tools grep on it). ::: mailnews/base/src/nsStatusBarBiffManager.cpp @@ +48,5 @@ > #define SYSTEM_SOUND_TYPE 0 > #define CUSTOM_SOUND_TYPE 1 > #define PREF_CHAT_ENABLED "mail.chat.enabled" > #define NEW_CHAT_MESSAGE_TOPIC "new-directed-incoming-message" > +#define PLAY_NOTIFICATION_SOUND "play-notification-sound" I'm not a mailnews reviewer, but I don't think anybody will be upset if this straightforward change goes in with my review.
I created PLAY_NOTIFICATION_SOUND but I have left NEW_CHAT_MESSAGE_TOPIC. Since, now, the latter will not be required to issue sound notifications, the former will be used (in case of chatNotifications.jsm) but /chat/components/src/imConversations.js /mailnews/base/src/nsMessengerOSXIntegration.mm still use it. So, 1) Is that okay? 2) Does that need to be changed as well? or 3) We can't do this? Thanks.
(In reply to Suyash Agarwal (:sshagarwal) from comment #17) I think it's OK.
Comment on attachment 8559367 [details] [diff] [review] Patch v4 Review of attachment 8559367 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK overall, but needs some polishing. Flo already made the key points, here are mainly coding style issues. ::: mail/components/im/modules/chatNotifications.jsm @@ +21,5 @@ > } catch (e) { } > return ellipsis; > }, > > + // Holds first direct message that needs to be notified. This isn't clear. Is this better: // Holds the first message after the last notification for _lastMessageSender, while we wait for more messages from that sender to arrive. _heldMessage... @@ +27,5 @@ > + // Number of messages to be bundled in the notification (excluding > + // _prevMessage). > + _msgCounter: 0, > + // Time the last message was received. > + _time: 0, _lastMessageTime might be clearer. @@ +29,5 @@ > + _msgCounter: 0, > + // Time the last message was received. > + _time: 0, > + // Sender of the last message. > + _sender: null, _lastMessageSender ? @@ +41,5 @@ > + }, > + > + _showMessageNotification: function (aMessage, aCounter) { > + // We are about to show the notification, so let's play the notification sound. > + Services.obs.notifyObservers(aMessage, "play-notification-sound", false); Is this a new sound you are adding? I don't see where it is being moved from in the existing code. @@ +43,5 @@ > + _showMessageNotification: function (aMessage, aCounter) { > + // We are about to show the notification, so let's play the notification sound. > + Services.obs.notifyObservers(aMessage, "play-notification-sound", false); > + > + // If TB window has focus, there's no need to show the notification.. If there's no need to show the notification, is there a need to play a sound? I'm not sure what the expected behaviour is here. @@ +72,5 @@ > + messageText = messageText.substr(0, 50) + > + ((aCounter > 0) ? "" : this.ellipsis); > + > + if (aCounter > 0) { > + let moreMsgs = bundle.getString("more"); This string needs a more descriptive name. @@ +118,5 @@ > > _notificationPrefName: "mail.chat.show_desktop_notifications", > observe: function(aSubject, aTopic, aData) { > if (aTopic == "new-directed-incoming-message") { > + if (Services.prefs.getBoolPref(this._notificationPrefName)) { Save yourself an indentation level by merging this with the previous if clause. @@ +119,5 @@ > _notificationPrefName: "mail.chat.show_desktop_notifications", > observe: function(aSubject, aTopic, aData) { > if (aTopic == "new-directed-incoming-message") { > + if (Services.prefs.getBoolPref(this._notificationPrefName)) { > + if (this._sender == null) { Please add comments to each of the three cases in this if clause. @@ +123,5 @@ > + if (this._sender == null) { > + this._sender = aSubject.who || aSubject.alias; > + this._time = aSubject.time; > + this._showMessageNotification(aSubject); > + } else if ((this._sender != (aSubject.who || aSubject.alias)) || You're duplicating (aSubject.who || aSubject.alias) a lot in this code. Stick in in a local variable above the if clause. @@ +124,5 @@ > + this._sender = aSubject.who || aSubject.alias; > + this._time = aSubject.time; > + this._showMessageNotification(aSubject); > + } else if ((this._sender != (aSubject.who || aSubject.alias)) || > + (aSubject.time >= this._time + 2)) { Replace all the '2's with a constant, this._kBundlingInterval or something like that. I also suspect it might feel even better if the value was higher, maybe 4 or 5? Not sure how frequently people want to be notified for the same person... @@ +126,5 @@ > + this._showMessageNotification(aSubject); > + } else if ((this._sender != (aSubject.who || aSubject.alias)) || > + (aSubject.time >= this._time + 2)) { > + this._time = aSubject.time; > + if (this._prevMessage) { // will not happen in case of t(i+1) > t(i) + 2 A clearer comment would say *why* ;) Also it should contain something descriptive like // Show notification for held messages now as the sender changed. @@ +136,5 @@ > + this._showMessageNotification(aSubject); > + } else if (this._sender == (aSubject.who || aSubject.alias) && > + this._time + 2 > aSubject.time) { > + this._time = aSubject.time; > + if (!this._prevMessage) { Nit: You don't need {..} around single lines. @@ +146,5 @@ > + this._timer.initWithCallback( > + function() { > + Notifications._showMessageNotification(Notifications._prevMessage, > + Notifications._msgCounter) > + }, 2000, Ci.nsITimer.TYPE_ONE_SHOT); Why are you using a timer and not setTimeout/clearTimeout? @@ +153,4 @@ > } else if (aTopic == "alertclickcallback") { > + // If there is a timer running, cancel it. > + this._timer.cancel(); > + this._clearAll(); You're only using this helper once, so inline it.
Attachment #8559367 - Flags: feedback?(aleth) → feedback+
(In reply to Florian Quèze [:florian] [:flo] from comment #16) > Comment on attachment 8559367 [details] [diff] [review] > Patch v4 > > Review of attachment 8559367 [details] [diff] [review]: > ----------------------------------------------------------------- > > I had actually started looking at this yesterday and got distracted. > Publishing the draft I had. Sorry, I thought you won't be getting back to this soon. > ::: mail/components/im/modules/chatNotifications.jsm > @@ +73,5 @@ > > + ((aCounter > 0) ? "" : this.ellipsis); > > + > > + if (aCounter > 0) { > > + let moreMsgs = bundle.getString("more"); > > + messageText = messageText + " " + this.ellipsis + > > I'm not sure what that " " is for here. It was looking quite messy, so I introduced a space :) We have ellipsis here and not in the localisation string because its possible that the previously stored message is longer than 50 chars and so we would like to append ellipsis to it. In that case, I don't think we'd want to show two ellipses. But ya, this looks odd, I'll make this a formatted string with plural form. But I am not sure how ellipsis would fit into the picture. > ::: mailnews/base/src/nsStatusBarBiffManager.cpp > @@ +48,5 @@ > > #define SYSTEM_SOUND_TYPE 0 > > #define CUSTOM_SOUND_TYPE 1 > > #define PREF_CHAT_ENABLED "mail.chat.enabled" > > #define NEW_CHAT_MESSAGE_TOPIC "new-directed-incoming-message" > > +#define PLAY_NOTIFICATION_SOUND "play-notification-sound" > (In reply to Florian Quèze [:florian] [:flo] from comment #18) > (In reply to Suyash Agarwal (:sshagarwal) from comment #17) > > I think it's OK. Okay so I'll remove "NEW_CHAT_MESSAGE_TOPIC" from this file since its only use was for sound notifications and we are going to use PLAY_NOTIFICATION_SOUND for it now. Thanks.
(In reply to aleth [:aleth] from comment #19) > Comment on attachment 8559367 [details] [diff] [review] > Patch v4 > > Review of attachment 8559367 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +41,5 @@ > > + }, > > + > > + _showMessageNotification: function (aMessage, aCounter) { > > + // We are about to show the notification, so let's play the notification sound. > > + Services.obs.notifyObservers(aMessage, "play-notification-sound", false); > > Is this a new sound you are adding? I don't see where it is being moved from > in the existing code. Ya, this is a new topic. Since the previously used topic "new-direct-message.." was being observed by both chatNotifications.jsm method and biff manager observer and it wasn't possible to handle it there in the similar fashion. So, instead of handling it in some other way, we issue this new topic from here which is now observed by the biff manager and sound can go on. > > If there's no need to show the notification, is there a need to play a > sound? I'm not sure what the expected behaviour is here. Ya, but I thought it'd be good to at least play the sound as per (Quoting Florian Quèze [:florian] [:flo] from comment #8) > Are you sure of this? It seems to be that if the Chat tab is not open (or > even not selected) it's easy to miss chat messages. But if this looks odd, please let me know. I'll remove it. > @@ +146,5 @@ > > + this._timer.initWithCallback( > > + function() { > > + Notifications._showMessageNotification(Notifications._prevMessage, > > + Notifications._msgCounter) > > + }, 2000, Ci.nsITimer.TYPE_ONE_SHOT); > > Why are you using a timer and not setTimeout/clearTimeout? Will look into this. Thanks.
(In reply to Suyash Agarwal (:sshagarwal) from comment #20) > to show two ellipses. But ya, this looks odd, I'll make this a formatted > string > with plural form. But I am not sure how ellipsis would fit into the picture. You can copy and paste the ellipsis character directly into the properties file, as we do in other places.
(In reply to Suyash Agarwal (:sshagarwal) from comment #21) > > Is this a new sound you are adding? I don't see where it is being moved from > > in the existing code. > Ya, this is a new topic. Since the previously used topic > "new-direct-message.." was being > observed by both chatNotifications.jsm method and biff manager observer and > it wasn't possible > to handle it there in the similar fashion. So, instead of handling it in > some other way, we issue > this new topic from here which is now observed by the biff manager and sound > can go on. > > > > > If there's no need to show the notification, is there a need to play a > > sound? I'm not sure what the expected behaviour is here. > Ya, but I thought it'd be good to at least play the sound as per > (Quoting Florian Quèze [:florian] [:flo] from comment #8) > > Are you sure of this? It seems to be that if the Chat tab is not open (or > > even not selected) it's easy to miss chat messages. > But if this looks odd, please let me know. I'll remove it. Does this mean that chat sound notifications are sent only from this code after your patch?
(In reply to aleth [:aleth] from comment #22) > (In reply to Suyash Agarwal (:sshagarwal) from comment #20) > > to show two ellipses. But ya, this looks odd, I'll make this a formatted > > string > > with plural form. But I am not sure how ellipsis would fit into the picture. > > You can copy and paste the ellipsis character directly into the properties > file, as we do in other places. Sorry, I forgot to mention the issue. If the previously held first message (that will be displayed along with the held messages count) is less than 50 chars, I want the ellipsis with the counter. But, if the message is more than 50 chars long, I want the ellipsis with the message so having it with the counter as well would look wrong. For instance: less than 50 chars message: |amsg ...5 more messages| more than 50 chars message: |amsg... 7 more messages| Does that sound right? Or, should we have some other decent behavior? Thanks.
(In reply to aleth [:aleth] from comment #23) > (In reply to Suyash Agarwal (:sshagarwal) from comment #21) > > > Is this a new sound you are adding? I don't see where it is being moved from > > > in the existing code. > > Ya, this is a new topic. Since the previously used topic > > "new-direct-message.." was being > > observed by both chatNotifications.jsm method and biff manager observer and > > it wasn't possible > > to handle it there in the similar fashion. So, instead of handling it in > > some other way, we issue > > this new topic from here which is now observed by the biff manager and sound > > can go on. > > > > > > > > If there's no need to show the notification, is there a need to play a > > > sound? I'm not sure what the expected behaviour is here. > > Ya, but I thought it'd be good to at least play the sound as per > > (Quoting Florian Quèze [:florian] [:flo] from comment #8) > > > Are you sure of this? It seems to be that if the Chat tab is not open (or > > > even not selected) it's easy to miss chat messages. > > But if this looks odd, please let me know. I'll remove it. > > Does this mean that chat sound notifications are sent only from this code > after your patch? For the notifications concerned by chatNotifications.jsm, yes. I was concerned about others as mentioned in comment 17 but florian says that should be okay. So, I assume its okay.
> For instance: > less than 50 chars message: > |amsg ...5 more messages| > more than 50 chars message: > |amsg... 7 more messages| That does look wrong. How about adding the ellipsis at the end of the message text whenever the notification doesn't show all the available message text(s), e.g. Hello Hello world... Hello... (and 7 more messages) Hello world... (and 5 more messages) If italics are supported in notifications, you could drop the parenthesis and use that instead. Alternatively one could simplify to Hello Hello world... (7 more messages) (5 more messages) i.e. not showing any message content in the bundled followup notifications. (In reply to Suyash Agarwal (:sshagarwal) from comment #25) > For the notifications concerned by chatNotifications.jsm, yes. > I was concerned about others as mentioned in comment 17 but florian says > that should be okay. So, I assume its okay. It would be nice if you could check after this patch is applied whether it is still possible to receive too many chat sound notifications (of any kind) per second, and if necessary file a separate bug to fix this. (By "too many" I mean "so many sounds that they start piling up because the total length of the sounds to be played exceeds the time between notifications").
Flags: needinfo?(richard.marti)
(In reply to aleth [:aleth] from comment #26) > > For instance: > > less than 50 chars message: > > |amsg ...5 more messages| > > more than 50 chars message: > > |amsg... 7 more messages| > > That does look wrong. How about adding the ellipsis at the end of the > message text whenever the notification doesn't show all the available > message text(s), e.g. > > Hello > Hello world... > Hello... (and 7 more messages) > Hello world... (and 5 more messages) > > If italics are supported in notifications, you could drop the parenthesis > and use that instead. > > Alternatively one could simplify to > > Hello > Hello world... > (7 more messages) > (5 more messages) This is what I proposed in comment 4 :). I think we should do this. With bug 760762 fixed (no pressure aleth :) ), it would be easy to know where we have to begin reading and where is no hint in the popup needed to know where we have to begin.
Flags: needinfo?(richard.marti)
Ya, the idea looks great. But I just think it sort of, kills the purpose of "message preview" particularly because we expose it in preferences.
(In reply to Suyash Agarwal (:sshagarwal) from comment #28) > Ya, the idea looks great. But I just think it sort of, kills the purpose of > "message preview" particularly because we expose it in preferences. I don't think that's true, depending on how you look at it. At the moment, you get a message preview in the notification, for the first 50 characters of the message. With the change, you get a message preview in the notification, for the first 50 characters of *a bundle of* messages.
Oh, so isn't the last suggestion about just displaying the count? Sorry if I have misunderstood this.
(In reply to Suyash Agarwal (:sshagarwal) from comment #30) > Oh, so isn't the last suggestion about just displaying the count? > Sorry if I have misunderstood this. No, my point is merely it depends on how you define the bundle, whether it's from the first message from the sender onwards or from the first message of each block of messages. Basically, the second suggestion notifies you of the beginning of a block of messages and then keeps notifying you that your buddy hasn't stopped talking ;) Anyway, decide with :paenglab what to use here :-)
Blocks: 833732
Attached patch Patch v5 (obsolete) — — Splinter Review
Made the suggested changes. Thanks.
Attachment #8559367 - Attachment is obsolete: true
Attachment #8565303 - Flags: ui-review?(richard.marti)
Attachment #8565303 - Flags: review?(aleth)
Comment on attachment 8565303 [details] [diff] [review] Patch v5 Review of attachment 8565303 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/im/modules/chatNotifications.jsm @@ +10,5 @@ > Cu.import("resource:///modules/StringBundle.js"); > +Cu.import("resource://gre/modules/PluralForm.jsm"); > +Cu.import("resource://gre/modules/Timer.jsm"); > + > +const kAllowedTimeGap = 3; Please add a comment (3 what? seconds? Time Gap between what?) @@ +24,5 @@ > } catch (e) { } > return ellipsis; > }, > > + // Holds first direct message that needs to be notified. Holds the first direct message of a bundle while we wait for further messages from the same sender to arrive. @@ +29,5 @@ > + _heldMessage: null, > + // Number of messages to be bundled in the notification (excluding > + // _heldMessage). > + _msgCounter: 0, > + // Time the last message was received. ...the last message the user was notified about... @@ +36,5 @@ > + _lastMessageSender: null, > + // timeout Id for the set timeout for showing notification. > + _timeoutId: null, > + > + _showMessageNotification: function (aMessage, aCounter) { nit: no space after function @@ +38,5 @@ > + _timeoutId: null, > + > + _showMessageNotification: function (aMessage, aCounter) { > + // We are about to show the notification, so let's play the notification sound. > + Services.obs.notifyObservers(aMessage, "play-notification-sound", false); Ask Paenglab if we really want to play a sound if the window has focus. Or does whatever receives this notification deal with whether to actually play a sound or not? @@ +41,5 @@ > + // We are about to show the notification, so let's play the notification sound. > + Services.obs.notifyObservers(aMessage, "play-notification-sound", false); > + > + // If TB window has focus, there's no need to show the notification.. > + if (Services.wm.getMostRecentWindow("mail:3pane").document.hasFocus()) { Since you return early here, why do you have to check document.hasFocus again below? @@ +52,3 @@ > let messageText, icon, name; > let notificationContent = Services.prefs.getIntPref("mail.chat.notification_info"); > + // 0 - show all the info, 1 - show only the sender not the message, 2 - show no details. nit: Is this < 80 characters or should it be on two lines? @@ +66,5 @@ > > // Crop the end of the text if needed. > + if (messageText.length > 50) { > + messageText = messageText.substr(0, 50); > + if (aCounter == 0) Add a comment that for aCounter!=0 the ellipsis is already included in the localized string. @@ +133,5 @@ > + if (this._heldMessage) { > + // if the time for the current message is greater than _lastMessageTime by > + // more than kAllowedTimeGap, this will not happen since the notification will > + // have already been dispatched. > + if (this._timeoutId) You don't need if (this._timeoutId) as clearTimeout already checks for that. @@ +153,5 @@ > + this._msgCounter++; > + > + if (this._timeoutId) > + clearTimeout(this._timeoutId); > + this._timeoutId = setTimeout( This makes no sense. You are resetting the timer each time another message arrives, when really you only want to set it once - when storing the held message. ::: mail/locales/en-US/chrome/messenger/chat.properties @@ +91,5 @@ > # the last 8-14 days. > log.currentWeek=This Week > log.previousWeek=Last Week > > messagePreview=New Chat Message While you are at it, please add a localization note comment for this too explaining where it is used. @@ +96,5 @@ > + > +#LOCALIZATION NOTE (bundledMessagePreview): Semi-colon list of plural forms. > +# Extra messages that are being bundled in the notification. > +# #1 is the number of messages being bundled. > +# Do not translate %1$S, it is the message preview to be shown in the notification. How about something like #LOCALIZATION NOTE (bundledMessagePreview): # Used when multiple incoming messages from the same sender are # bundled into a single notification. # #1 is the number of incoming messages the user is being notified about. When #1 # is greater than one, the plural form after the semicolon is used. # %1$S is the message preview to be shown in the notification, i.e. the first incoming message. @@ +97,5 @@ > +#LOCALIZATION NOTE (bundledMessagePreview): Semi-colon list of plural forms. > +# Extra messages that are being bundled in the notification. > +# #1 is the number of messages being bundled. > +# Do not translate %1$S, it is the message preview to be shown in the notification. > +bundledMessagePreview=%1$S… (#1 more message);%1$S… (#1 more messages) Why #1 and not %2$S ?
Attachment #8565303 - Flags: review?(aleth) → review-
Comment on attachment 8565303 [details] [diff] [review] Patch v5 Review of attachment 8565303 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/locales/en-US/chrome/messenger/chat.properties @@ +97,5 @@ > +#LOCALIZATION NOTE (bundledMessagePreview): Semi-colon list of plural forms. > +# Extra messages that are being bundled in the notification. > +# #1 is the number of messages being bundled. > +# Do not translate %1$S, it is the message preview to be shown in the notification. > +bundledMessagePreview=%1$S… (#1 more message);%1$S… (#1 more messages) Needs to be "(and N more messages)" to make it clear you are not counting the previewed message.
(In reply to aleth [:aleth] from comment #33) > Comment on attachment 8565303 [details] [diff] [review] > Patch v5 > @@ +29,5 @@ > > + _heldMessage: null, > > + // Number of messages to be bundled in the notification (excluding > > + // _heldMessage). > > + _msgCounter: 0, > > + // Time the last message was received. > > ...the last message the user was notified about... No, this is not what I had in mind while writing the patch. Whenever a message arrives, a kAllowedTimeGap seconds window starts from that message. So, everytime a message arrives (even in that running time gap window), lastTime is updated. And, user is notified only after 1) the sender has changed (not related to this time issue) or 2) the sender has stopped sending messages and kAllowdTimeGap seconds have passed since the last message received (but obviously not notified). > @@ +41,5 @@ > > + // If TB window has focus, there's no need to show the notification.. > > + if (Services.wm.getMostRecentWindow("mail:3pane").document.hasFocus()) { > > Since you return early here, why do you have to check document.hasFocus > again below? I am not sure about this. Is it possible that since we enter this function when user was away from the window, and reach this condition, the user is on the window? I am not sure if this runs on the main thread as the UI or not. > @@ +153,5 @@ > > + this._msgCounter++; > > + > > + if (this._timeoutId) > > + clearTimeout(this._timeoutId); > > + this._timeoutId = setTimeout( > > This makes no sense. You are resetting the timer each time another message > arrives, when really you only want to set it once - when storing the held > message. Ya, its because of our different thoughts about the implementation here. Please consider my thought mentioned above. > @@ +97,5 @@ > > +#LOCALIZATION NOTE (bundledMessagePreview): Semi-colon list of plural forms. > > +# Extra messages that are being bundled in the notification. > > +# #1 is the number of messages being bundled. > > +# Do not translate %1$S, it is the message preview to be shown in the notification. > > +bundledMessagePreview=%1$S… (#1 more message);%1$S… (#1 more messages) > > Why #1 and not %2$S ? Because #1 is a number and not a string. Its used this way everywhere.
Comment on attachment 8565303 [details] [diff] [review] Patch v5 ui-r+ with the string change proposed from aleth. (In reply to aleth [:aleth] from comment #33) > Comment on attachment 8565303 [details] [diff] [review] > Patch v5 > > Review of attachment 8565303 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +38,5 @@ > > + _timeoutId: null, > > + > > + _showMessageNotification: function (aMessage, aCounter) { > > + // We are about to show the notification, so let's play the notification sound. > > + Services.obs.notifyObservers(aMessage, "play-notification-sound", false); > > Ask Paenglab if we really want to play a sound if the window has focus. Or > does whatever receives this notification deal with whether to actually play > a sound or not? I'm okay with a sound when the window has focus but the active tab isn't the chat tab.
Attachment #8565303 - Flags: ui-review?(richard.marti) → ui-review+
(In reply to Suyash Agarwal (:sshagarwal) from comment #35) > (In reply to aleth [:aleth] from comment #33) > > > + // Time the last message was received. > > > > ...the last message the user was notified about... > No, this is not what I had in mind while writing the patch. > Whenever a message arrives, a kAllowedTimeGap seconds window starts from > that message. > So, everytime a message arrives (even in that running time gap window), > lastTime is updated. > And, user is notified only after > 1) the sender has changed (not related to this time issue) or > 2) the sender has stopped sending messages and kAllowdTimeGap seconds have > passed since the last > message received (but obviously not notified). To clarify: Your idea is that until the sender stops typing for more than N seconds, the user gets one notification for the *first* message and then *no more notifications* until the (bundled) notification N seconds *after* the last message (or when someone else sends a message)?
(In reply to aleth [:aleth] from comment #37) > (In reply to Suyash Agarwal (:sshagarwal) from comment #35) > > (In reply to aleth [:aleth] from comment #33) > > > > + // Time the last message was received. > > > > > > ...the last message the user was notified about... > > No, this is not what I had in mind while writing the patch. > > Whenever a message arrives, a kAllowedTimeGap seconds window starts from > > that message. > > So, everytime a message arrives (even in that running time gap window), > > lastTime is updated. > > And, user is notified only after > > 1) the sender has changed (not related to this time issue) or > > 2) the sender has stopped sending messages and kAllowdTimeGap seconds have > > passed since the last > > message received (but obviously not notified). > > To clarify: Your idea is that until the sender stops typing for more than N > seconds, the user gets one notification for the *first* message and then *no > more notifications* until the (bundled) notification N seconds *after* the > last message (or when someone else sends a message)? Yes.
(In reply to Suyash Agarwal (:sshagarwal) from comment #38) > (In reply to aleth [:aleth] from comment #37) > > To clarify: Your idea is that until the sender stops typing for more than N > > seconds, the user gets one notification for the *first* message and then *no > > more notifications* until the (bundled) notification N seconds *after* the > > last message (or when someone else sends a message)? > > Yes. OK, sorry for misunderstanding the expected behaviour you were aiming for! In that case, I also agree 2s is better for kAllowedTimeGap. Also, let's call it kMaxTimeBetweenBundledMessages - it's a bit long but clearer. I'm also OK with the #1 if that is general practice - I hadn't seen it before. Please update the patch with the other comments addressed, and check the sound notifications have the behaviour from comment #36 and obey the relevant prefs.
Comment on attachment 8565303 [details] [diff] [review] Patch v5 Review of attachment 8565303 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/im/modules/chatNotifications.jsm @@ +166,5 @@ > + clearTimeout(this._timeoutId); > + this._heldMessage = null; > + this._msgCounter = 0; > + this._lastMessageTime = 0; > + this._lastMessageSender = null; In this patch, this._heldMessage = null; this._msgCounter = 0; is duplicated three times, and I think in all these cases we can reset all four variables, so I think now a 'reset' helper method like the one you had before would make sense.
(In reply to aleth [:aleth] from comment #39) > In that case, I also agree 2s is better for kAllowedTimeGap. Actually, I have no idea what the best value is. You've done a lot of testing with this, so pick what you think works best, 2,3,4,5, whatever!
The delay shouldn't be to long to not have a to long wait time until the notification is shown after the last message. 3 seconds seems a good compromise.
Attached patch Patch v6 (obsolete) — — Splinter Review
Made the suggested changes. Thanks.
Attachment #8565303 - Attachment is obsolete: true
Attachment #8566112 - Flags: ui-review+
Attachment #8566112 - Flags: review?(aleth)
Comment on attachment 8566112 [details] [diff] [review] Patch v6 Review of attachment 8566112 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this, and sorry for the misunderstandings along the way! You can carry the r+ forward after fixing the nit. ::: mail/components/im/modules/chatNotifications.jsm @@ +159,5 @@ > + this._msgCounter++; > + > + clearTimeout(this._timeoutId); > + this._timeoutId = setTimeout( > + function() { nit: We generally move the "function() {" directly behind setTimeout to save a line.
Attachment #8566112 - Flags: review?(aleth) → review+
Attached patch Patch v6.1 (obsolete) — — Splinter Review
Thanks :)
Attachment #8566112 - Attachment is obsolete: true
Attachment #8566126 - Flags: ui-review+
Attachment #8566126 - Flags: review+
Keywords: checkin-needed
Comment on attachment 8566126 [details] [diff] [review] Patch v6.1 Review of attachment 8566126 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/src/nsStatusBarBiffManager.cpp @@ +47,5 @@ > #define PREF_SOUND_TYPE "play_sound.type" > #define SYSTEM_SOUND_TYPE 0 > #define CUSTOM_SOUND_TYPE 1 > #define PREF_CHAT_ENABLED "mail.chat.enabled" > +#define PLAY_NOTIFICATION_SOUND "play-notification-sound" A last minute request: can we change play-notification-sound to play-chat-notification-sound to avoid confusion with mail? Thanks!
Keywords: checkin-needed
Attached patch Patch v7 (obsolete) — — Splinter Review
Okay, done. Thanks.
Attachment #8566126 - Attachment is obsolete: true
Attachment #8566471 - Flags: ui-review+
Attachment #8566471 - Flags: review+
Keywords: checkin-needed
(In reply to Suyash Agarwal (:sshagarwal) from comment #47) > Created attachment 8566471 [details] [diff] [review] > Patch v7 > > Okay, done. I think you attached the wrong attachment?
Keywords: checkin-needed
Whoops, sorry!
Attachment #8566471 - Attachment is obsolete: true
Attachment #8566474 - Flags: ui-review+
Attachment #8566474 - Flags: review+
Keywords: checkin-needed
Comment on attachment 8566474 [details] [diff] [review] Patch for check-in Review of attachment 8566474 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/src/nsStatusBarBiffManager.cpp @@ +47,5 @@ > #define PREF_SOUND_TYPE "play_sound.type" > #define SYSTEM_SOUND_TYPE 0 > #define CUSTOM_SOUND_TYPE 1 > #define PREF_CHAT_ENABLED "mail.chat.enabled" > +#define PLAY_NOTIFICATION_SOUND "play-chat-notification-sound" umm... also change to PLAY_CHAT_NOTIFICATION_SOUND of course!
Keywords: checkin-needed
(In reply to aleth [:aleth] from comment #50) > umm... also change to PLAY_CHAT_NOTIFICATION_SOUND of course! Never mind, I'll fix this up before landing.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: