Closed Bug 1649123 Opened 1 year ago Closed 1 year ago

Implement typing indication in the direct messaging

Categories

(Chat Core :: Matrix, task)

Tracking

(thunderbird_esr78 fixed)

RESOLVED FIXED
81 Branch
Tracking Status
thunderbird_esr78 --- fixed

People

(Reporter: khushil324, Assigned: khushil324)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Blocks: 1572636
Assignee: nobody → khushil324
Status: NEW → ASSIGNED
Attachment #9160116 - Flags: review?(clokep)
Attachment #9160116 - Attachment is patch: true
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?
Attachment #9160116 - Flags: review?(clokep) → feedback+
Attachment #9160116 - Attachment is obsolete: true
Attachment #9167263 - Flags: review?(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?

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?

(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 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)
Attachment #9167263 - Flags: review?(clokep) → review+

(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.

Attachment #9167263 - Attachment is obsolete: true
Attachment #9167736 - Flags: review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/438b6efe965a
Implement typing indication in the Matrix direct messaging. r=clokep

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

Uplift to 78?

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

Uplift to 78?

I think that's reasonable.

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.

Attachment #9167736 - Flags: approval-comm-esr78?

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

[Triage Comment]
Approved for esr78

Attachment #9167736 - Flags: approval-comm-esr78? → approval-comm-esr78+
You need to log in before you can comment on or make changes to this bug.