1649123 - Implement typing indication in the direct messaging

Status: RESOLVED FIXED

(Chat Core :: Matrix, task)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1649123_Matrix-typing-indecation-0.patch

Comment 2

[Patrick Cloke [:clokep]]

Comment on attachment 9160116 [details] [diff] [review]
Bug-1649123_Matrix-typing-indecation-0.patch

Review of attachment 9160116 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay on this. It looks reasonable overall! I'm a bit concerned about the overlap with XMPP code -- is there bits of this that could be abstracted? We'd likely need to add a new API.

::: chat/protocols/matrix/matrix.jsm
@@ +264,5 @@
> +    if (string.length) {
> +      this._typingTimer = setTimeout(this.finishedComposing.bind(this), 10000);
> +    }
> +
> +    this._setTypingState(string.length ? "composing" : "active");

Looks to me like this could just be true or false here -- no need to have a string?

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1649123_Matrix-typing-indecation-1.patch

Comment 4

[Patrick Cloke [:clokep]]

(In reply to Patrick Cloke [:clokep] from comment #2

Sorry for the delay on this. It looks reasonable overall! I'm a bit
concerned about the overlap with XMPP code -- is there bits of this that
could be abstracted? We'd likely need to add a new API.

Any thoughts on this?

Comment 5

[Khushil Mistry [:khushil324]]

This typing code is only overlapping between XMPP and Matrix only these two protocols. We can shift it to the core GenericConvIMPrototype but we need to separate _setTypingState function as it indicates to the SDKs for typing set up. Can we have a whole structure where _setTypingState lies in the protocol side and rest of the functions in GenericConvIMPrototype?

Comment 6

[Patrick Cloke [:clokep]]

(In reply to Khushil Mistry [:khushil324] from comment #5)

This typing code is only overlapping between XMPP and Matrix only these two protocols. We can shift it to the core GenericConvIMPrototype but we need to separate _setTypingState function as it indicates to the SDKs for typing set up. Can we have a whole structure where _setTypingState lies in the protocol side and rest of the functions in GenericConvIMPrototype?

Yeah that was kind of my thought -- all the timers and such could be handled via generic code, but then the protocol specific code could be implemented in a handler.

I guess this being in only two protocols isn't terrible though. Maybe we'll worry about it when it is in a third?

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 9167263 [details] [diff] [review]
Bug-1649123_Matrix-typing-indecation-1.patch

Review of attachment 9167263 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine with the one additional change!

::: chat/protocols/matrix/matrix.jsm
@@ +282,5 @@
> +    if (this._typingState == isTyping) {
> +      return;
> +    }
> +
> +    if (isTyping) {

No reason for an if statement anymore, this can be simplified to: this._account._client.sendTyping(this._roomId, isTyping)

Comment 8

[Khushil Mistry [:khushil324]]

(In reply to Patrick Cloke [:clokep] from comment #6)

I guess this being in only two protocols isn't terrible though. Maybe we'll worry about it when it is in a third?

Yes, right now, it can also mean unnecessary things for other protocols. It's not a bigger task if we take it up in the next protocol.

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: Bug-1649123_Matrix-typing-indecation-2.patch

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/438b6efe965a
Implement typing indication in the Matrix direct messaging. r=clokep

Comment 11

[Magnus Melin [:mkmelin]]

Uplift to 78?

Comment 12

[Patrick Cloke [:clokep]]

(In reply to Magnus Melin [:mkmelin] from comment #11)

Uplift to 78?

I think that's reasonable.

Comment 13

[Khushil Mistry [:khushil324]]

Comment on attachment 9167736 [details] [diff] [review]
Bug-1649123_Matrix-typing-indecation-2.patch

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Adding a typing indication feature in Matrix Chat Protocol.
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky): Low.

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9167736 [details] [diff] [review]
Bug-1649123_Matrix-typing-indecation-2.patch

[Triage Comment]
Approved for esr78

Comment 15

[Rob Lemley [:rjl]]

Thunderbird 78.2.2:
https://hg.mozilla.org/releases/comm-esr78/rev/4612a22c5592