GNOME Bugzilla – Bug 644297
Should ack messages
Last modified: 2011-09-19 12:15:20 UTC
Once the message tray is a proper Handler (bug #643594), it should ack messages when they are displayed to user.
*** Bug 643865 has been marked as a duplicate of this bug. ***
*** Bug 648793 has been marked as a duplicate of this bug. ***
How can I know when the chat message tray is opened by the user? I didn't find any signal for that.
yeah, you'd have to add one
I have implemented this in http://cgit.collabora.com/git/user/cassidy/gnome-shell/log/?h=ack-messages-644297 but we should fix bug #643594 and bug #651227 first.
Created attachment 189539 [details] [review] messageTray.js: fire a signal when the summary item is clicked
Created attachment 189540 [details] [review] telepathyClient.js: ack pending messages (#644297)
Review of attachment 189539 [details] [review]: I've been running this branch for about two weeks. The code looks fine. My only comment is regarding the first patch. I think this should be merged pretty quickly. The Telepathy shell integration is arguably more annoying with this not merged since the handler branch got merged. Maybe that's just me though. I'm not a shell reviewer though. :-( ::: js/ui/messageTray.js @@ +2188,3 @@ } }; +Signals.addSignalMethods(MessageTray.prototype); Why is this needed? I'm not very familiar with these signals.
(In reply to comment #8) > +Signals.addSignalMethods(MessageTray.prototype); > > Why is this needed? I'm not very familiar with these signals. The method patches the prototype to add gobject-style signal methods (emit, connect, disconnect, ...), which don't exist natively in JS. In this particular case it is not needed, as the new signal is emitted on summaryItem.source (and the signal methods already have been added to Source.prototype) rather than the message tray itself.
Created attachment 189632 [details] [review] messageTray.js: fire a signal when the summary item is clicked
Review of attachment 189540 [details] [review]: ::: js/ui/telepathyClient.js @@ +226,3 @@ + // We ack messages when the message box is displayed when: + // - user clicked on it the tray + // - user expanded the notification by hovering it "by hovering it"? Do you mean hovering over the summary item? @@ +369,3 @@ return; + this._pendingMessages.push(message); It doesn't do that much harm, but doing a loop just to push things to another array seems wrong. However, I'm not sure the alternative is much better: this._pendingMessages.push.apply(this._pendingMessages, tpPendingMessages); (I wish JS was more like Python. Yay list.extend) @@ +436,3 @@ + + _pendingRemoved: function(channel, message) { + this._pendingMessages.pop(message); Array.pop doesn't take an argument. Try: this._pendingMessages.splice(this._pendingMessages.indexOf(message), 1); (again, I wish JS was more like Python. Yay list.remove)
(In reply to comment #11) > Review of attachment 189540 [details] [review]: > > ::: js/ui/telepathyClient.js > @@ +226,3 @@ > + // We ack messages when the message box is displayed when: > + // - user clicked on it the tray > + // - user expanded the notification by hovering it > > "by hovering it"? > > Do you mean hovering over the summary item? Yeah; fixed. > @@ +369,3 @@ > return; > > + this._pendingMessages.push(message); > > It doesn't do that much harm, but doing a loop just to push things to another > array seems wrong. > > However, I'm not sure the alternative is much better: > > this._pendingMessages.push.apply(this._pendingMessages, tpPendingMessages); We need the loop anyway so I'd leave it as it. > @@ +436,3 @@ > + > + _pendingRemoved: function(channel, message) { > + this._pendingMessages.pop(message); > > Array.pop doesn't take an argument. Try: > > this._pendingMessages.splice(this._pendingMessages.indexOf(message), 1); > > (again, I wish JS was more like Python. Yay list.remove) Changed (more I use it more I hate JS...)
Created attachment 189821 [details] [review] telepathyClient.js: ack pending messages (#644297)
Review of attachment 189821 [details] [review]: ::: js/ui/telepathyClient.js @@ +226,3 @@ + // We ack messages when the message box is displayed when: + // - user clicked on it the tray + // - user expanded the notification by hovering over the summary item I don't think you mean "summary item" -- that's the part in the message tray with the accordion effect. I think you mean the toaster notifications... I don't know a better term for that than "toaster notifications", unfortunately, so use that, @@ +436,3 @@ + + _pendingRemoved: function(channel, message) { + this._pendingMessages.splice(this._pendingMessages.indexOf(message), 1); Actually, this still isn't correct: in the case that it's not in the list, it will just pop the last item from the list. That should never happen, but we should be robust: splice in the case the idx >= 0, log the message otherwise. Additionally, if tp-glib doesn't intern messages, this won't work at all because you'll have two different objects. In that case, I suggest working with the "message"-style JS objects -- that is, push the makeMessageFromTpMessage'd form of the object instead, do that to the TpSignalledMessage that you get from the signal handler, and implement a custom comparator and loop. @@ +443,3 @@ + return; + + this._channel.ack_messages_async(this._pendingMessages, null); Should you clear this._pendingMessages here, or will you get a 'pending-message-removed' for each one? If it's the latter, I'd recommend putting in a comment: // Don't clear our messages here, tp-glib will send a 'pending-message-removed' for each one.
Review of attachment 189632 [details] [review]: ::: js/ui/messageTray.js @@ +1607,3 @@ this._clickedSummaryItemMouseButton = button; + + summaryItem.source.emit('summary-item-clicked'); We should send the button here as well so that we don't accidentally ack messages on RMB click.
So, you should not ack messages when you simply mouse down on the bottom thing. Instead: Only ack messages when the bottom notification bubble thing disappears. But don't ack the messages if you have clicked on it to delegate the channel to Empathy. Make sure DelegateChannels succeeds otherwise you'll not ack the messages and Empathy won't see them. Makes sense?
(In reply to comment #14) > Review of attachment 189821 [details] [review]: > > ::: js/ui/telepathyClient.js > @@ +226,3 @@ > + // We ack messages when the message box is displayed when: > + // - user clicked on it the tray > + // - user expanded the notification by hovering over the summary item > > I don't think you mean "summary item" -- that's the part in the message tray > with the accordion effect. I think you mean the toaster notifications... I > don't know a better term for that than "toaster notifications", unfortunately, > so use that, changed. > @@ +436,3 @@ > + > + _pendingRemoved: function(channel, message) { > + this._pendingMessages.splice(this._pendingMessages.indexOf(message), > 1); > > Actually, this still isn't correct: in the case that it's not in the list, it > will just pop the last item from the list. > > That should never happen, but we should be robust: splice in the case the idx > >= 0, log the message otherwise. done. > Additionally, if tp-glib doesn't intern messages, this won't work at all > because you'll have two different objects. In that case, I suggest working with > the "message"-style JS objects -- that is, push the makeMessageFromTpMessage'd > form of the object instead, do that to the TpSignalledMessage that you get from > the signal handler, and implement a custom comparator and loop. It does; I checked, the messages are removed. > @@ +443,3 @@ > + return; > + > + this._channel.ack_messages_async(this._pendingMessages, null); > > Should you clear this._pendingMessages here, or will you get a > 'pending-message-removed' for each one? If it's the latter, I'd recommend > putting in a comment: > > // Don't clear our messages here, tp-glib will send a > 'pending-message-removed' for each one. added.
(In reply to comment #15) > Review of attachment 189632 [details] [review]: > > ::: js/ui/messageTray.js > @@ +1607,3 @@ > this._clickedSummaryItemMouseButton = button; > + > + summaryItem.source.emit('summary-item-clicked'); > > We should send the button here as well so that we don't accidentally ack > messages on RMB click. RMB? Do you mean passing the summaryItem as argument to the signal?
> RMB? Do you mean passing the summaryItem as argument to the signal? RMB == "Right Mouse Button" I mean passing the 'button' argument, which tells you which mouse button it is. Also, do Johnny's thing Also, did you mean to attach a new patch?
So, I didn't implement Jonny's suggestion because gjs keeps crashing at me (bug #652753) but I do notice than having a collapsed signal on the Notification was an easier way to do things. So here is a new version of the branch. I think we shouldn't wait on the gjs bug to be fixed to merge this, we can easily improve it later.
Created attachment 190056 [details] [review] messageTray.js: add collapsed signal on Notification
Created attachment 190057 [details] [review] telepathyClient.js: ack pending messages (#644297)
Review of attachment 190056 [details] [review]: THe signal itself may be useful, but not for this purpose. This will be called when let notificationExpired = (this._notificationTimeoutId == 0 && !(this._notification && this._notification.urgency == Urgency.CRITICAL) && !this._pointerInTray && !this._locked) || this._notificationRemoved; ...welp. (A) the message timed out on its own accord, and it wasn't urgent, *and* the pointer isn't in the tray, and the screen isn't locked. (B) the notification was removed from the notification spec. It will also be called when the SummaryItem's "notification stack" is done showing -- meaning when the user clicks on the summary item and then hides the notification. I'm not sure we want both for ACKing messages.
Review of attachment 190057 [details] [review]: ::: js/ui/telepathyClient.js @@ +438,3 @@ + this._pendingMessages.splice(idx, 1); + else + global.logError('Message not in our pending list: ' + message); Don't use global.logError (nobody ever looks in the looking glass, so we should probably remove them). Instead, throwing an error might be better: throw new Error('Message not in our pending list: ' + message);
(In reply to comment #18) > (In reply to comment #14) > > Review of attachment 189821 [details] [review] [details]: > > Additionally, if tp-glib doesn't intern messages, this won't work at all > > because you'll have two different objects. In that case, I suggest working with > > the "message"-style JS objects -- that is, push the makeMessageFromTpMessage'd > > form of the object instead, do that to the TpSignalledMessage that you get from > > the signal handler, and implement a custom comparator and loop. > > It does; I checked, the messages are removed. I added some doc in tp-glib ensuring that: http://cgit.freedesktop.org/telepathy/telepathy-glib/commit/?id=0bfaa156755fbea6630a8c5e37b3d6b73c6c4452 (In reply to comment #26) > Review of attachment 190057 [details] [review]: > > ::: js/ui/telepathyClient.js > @@ +438,3 @@ > + this._pendingMessages.splice(idx, 1); > + else > + global.logError('Message not in our pending list: ' + message); > > Don't use global.logError (nobody ever looks in the looking glass, so we should > probably remove them). Instead, throwing an error might be better: > > throw new Error('Message not in our pending list: ' + message); done. (In reply to comment #25) > Review of attachment 190056 [details] [review]: > > THe signal itself may be useful, but not for this purpose. > > This will be called when > > let notificationExpired = (this._notificationTimeoutId == 0 && > !(this._notification && this._notification.urgency == Urgency.CRITICAL) && > !this._pointerInTray && !this._locked) || this._notificationRemoved; > > ...welp. > > (A) the message timed out on its own accord, and it wasn't urgent, *and* the > pointer isn't in the tray, and the screen isn't locked. > (B) the notification was removed from the notification spec. > > It will also be called when the SummaryItem's "notification stack" is done > showing -- meaning when the user clicks on the summary item and then hides the > notification. > > I'm not sure we want both for ACKing messages. You're right. We want when collapsing *but* only if user interacted with the box. I've done that.
Created attachment 190414 [details] [review] messageTray.js: fire a signal when the summary item is clicked
Created attachment 190415 [details] [review] messageTray.js: add collapsed signal on Notification
Created attachment 190416 [details] [review] telepathyClient.js: ack pending messages (#644297)
Review of attachment 190414 [details] [review]: OK.
Review of attachment 190415 [details] [review]: OK.
Review of attachment 190416 [details] [review]: OK. We've talked about it on IRC, and it looks like Tp will send the same message instance each time, so we're safe. Good to go.
Review of attachment 190416 [details] [review]: ::: js/ui/telepathyClient.js @@ +454,3 @@ + // Don't clear our messages here, tp-glib will send a + // 'pending-message-removed' for each one. + this._channel.ack_messages_async(this._pendingMessages, null); Actually, oops, I forgot. Due to some GJS changes, we're going to require a callback here. Nothing too fancy, see: http://git.gnome.org/browse/gnome-shell/commit/?id=5c6e5ef6d003e44d670cec7235c3b7522726c526 for some details.
Created attachment 190910 [details] [review] telepathyClient.js: ack pending messages (#644297)
Merged.
*** Bug 659458 has been marked as a duplicate of this bug. ***