Created attachment 223577 [details] [review]
TelepathyClient: don't emit an error if an unknown message is acknowledged

pending-messages-removed is emitted for sent messages too, but we don't
include those in the _pendingMessages list. Avoid useless spew in the session
logs in that case.

Created attachment 223578 [details] [review]
TelepathyClient: ignore invalid message id errors

Those errors can happen if the pending queue message is not drained fast
enough and we attempt another ack_all_messages. They are harmless (they
just indicate the message is not pending), so ignore them.

Created attachment 223579 [details] [review]
GrabHelper: always navigate focus when grabbing

Users of GrabHelper.grab() espect that the actor parameter (or one of its
children) will receive focus, irrespective of the previous focus location.
This fixes the key focus on the chat entry when expanding the notification.

Created attachment 223580 [details] [review]
MessageTray: fix keybinding for fixed GrabHelper

With the new GrabHelper, grabbing focus ourselves is not necessary.

(In reply to comment #4)
> Created an attachment (id=223580) [details] [review]
> MessageTray: fix keybinding for fixed GrabHelper
> 
> With the new GrabHelper, grabbing focus ourselves is not necessary.

Uhm, this is still broken after you exit the overview... Needs further investigation.

Created attachment 223582 [details] [review]
GrabHelper: always navigate focus when grabbing

Users of GrabHelper.grab() espect that the actor parameter (or one of its
children) will receive focus, irrespective of the previous focus location.
This fixes the key focus on the chat entry when expanding the notification.

This one is even better, and keeps the right focus when entering the overview.

Review of attachment 223582 [details] [review]:

Commit message still says "always". Also, "espect".

And I remember having issues with this before, but whatever. If it works, it works.

Review of attachment 223580 [details] [review]:

Did you obsolete this one by mistake?

Review of attachment 223577 [details] [review]:

We really should include sent messages in the _pendingMessages list.

Review of attachment 223578 [details] [review]:

When will the "pending queue message" not be drained fast enough?

(In reply to comment #9)
> Review of attachment 223577 [details] [review]:
> 
> We really should include sent messages in the _pendingMessages list.

That means having the source counter include the number of pending sent messages, which is wrong.

(In reply to comment #10)
> Review of attachment 223578 [details] [review]:
> 
> When will the "pending queue message" not be drained fast enough?

I'm not sure, I think it's when the previous acking is currently in flight, the pending-message-removed was not emitted and you open the notification, thus acking all messages again.

(In reply to comment #8)
> Review of attachment 223580 [details] [review]:
> 
> Did you obsolete this one by mistake?

No, it was meant for the previous iteration of the GrabHelper patch. With the new one, it's wrong.

Comment on attachment 223582 [details] [review]
GrabHelper: always navigate focus when grabbing

Attachment 223582 [details] pushed as 5a259dd - GrabHelper: always navigate focus when grabbing

Update on this one? At least patch 223577 is safe, and I don't want to wait after hard code freeze for it.

(In reply to comment #14)
> Update on this one? At least patch 223577 [details] is safe, and I don't want to wait
> after hard code freeze for it.

I really don't think it's safe. We shouldn't be ignoring errors like this. I'd prefer it if we tracked sent messages mid-flight for various reasons.

The various reasons being?

Note that if you really want all pending messages in the telepathy sense, rather than all message the user hasn't seen yet, you can use textChannel.get_pending_messages()

Review of attachment 223577 [details] [review]:

Whoops, I completely misread the Telepathy code last time. This is fine.

Comment on attachment 223577 [details] [review]
TelepathyClient: don't emit an error if an unknown message is acknowledged

Attachment 223577 [details] pushed as 488820d - TelepathyClient: don't emit an error if an unknown message is acknowledged

And what about the other one?
It is really annoying to have the logs spammed with dozens of
JS ERROR: !!!   Exception was: TelepathyGLib.Error: invalid message id 68
    JS ERROR: !!!     message = '"invalid message id 68"'
    JS ERROR: !!!     fileName = '"/opt/gnome/share/gnome-shell/js/ui/components/telepathyClient.js"'
    JS ERROR: !!!     lineNumber = '748'
    JS ERROR: !!!     stack = '"0 anonymous("result" = [object GObject_Object], "src" = [object GObject_Object])@/opt/gnome/share/gnome-shell/js/ui/components/telepathyClient.js:748

Btw, my new theory is that the shell is acking messages while acting as a Telepathy Observer, which conflicts with the acking done by empathy.
I am now testing it, and I'll post a patch if it works.

Nope, I'm still getting the errors, but the shell is the handler for the channel.

Created attachment 229948 [details] [review]
Telepathy: ignore errors from ack_all_pending_messages()

Invalid ID errors from that function are normal, because the set
of IDs to acknowledge may not match the set in the connection manager
due to race conditions.
This is also what empathy does.

Created attachment 229949 [details] [review]
Telepathy: ignore errors from ack_all_pending_messages()

Invalid ID errors from that function are normal, because the set
of IDs to acknowledge may not match the set in the connection manager
due to race conditions.
This is also what empathy does.

Oops...

Review of attachment 229949 [details] [review]:

Looks good to me.

Attachment 229949 [details] pushed as d517c13 - Telepathy: ignore errors from ack_all_pending_messages()