Closed Bug 1348038 Opened 4 years ago Closed 7 months ago

Handle Topic Events.

Categories

(Chat Core :: Matrix, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 77

People

(Reporter: matrixisreal, Assigned: khushil324)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

No description provided.
Attached patch topic (obsolete) — Splinter Review
Assignee: nobody → pavankarthikboddeda
Attachment #8879589 - Flags: review?(clokep)
Depends on: 1394397
Summary: Implement SetTopic from MatrixClient in MatrixConversations → Handle Topic Events.
Comment on attachment 8879589 [details] [diff] [review]
topic

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

Clearing review. The Matrix SDK needs to be updated and re-integrated before we do more work on it.
Attachment #8879589 - Flags: review?(clokep)
Assignee: pavankarthikboddeda → nobody
Attached patch WIP v1 (obsolete) — Splinter Review

Should we do it all the commands/events in one go?

Assignee: nobody → khushil324

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

Should we do it all the commands/events in one go?

I think there's a lot of different events in Matrix though, so I'd keep this one separate unless there's obvious cross-over between it and another type of event.

Apply after patch from Bug 1611444 and Bug 1347533.

Attachment #8879589 - Attachment is obsolete: true
Attachment #9124708 - Attachment is obsolete: true
Attachment #9133839 - Flags: review?(clokep)
Status: NEW → ASSIGNED
Attachment #9124708 - Attachment is patch: true
Comment on attachment 9133839 [details] [diff] [review]
Bug-1348038_Handle-Topic-Event-0.patch

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

Looks good overall, but I think there's a few details that are missing! Let me know if you have questions.

::: chat/protocols/matrix/matrix.jsm
@@ +146,5 @@
> +  },
> +
> +  set topic(aTopic) {
> +    // Check if our user has the permissions to set the topic.
> +    // Give a warning if he/she doesn't have the permission.

Shouldn't the UI not be active if the user cannot set it? I'm confused at how we would end up in here. Do we do similar checks in IRC/XMPP?

@@ +153,5 @@
> +    ) {
> +      this._account._client.setRoomTopic(this._roomId, aTopic);
> +    } else {
> +      // what should be the first argument to writeMessage?
> +      this.writeMessage("", _("room.cannotSetTopic"), { system: true });

This comes from jsProtoHelper.jsm, the first parameter is "who".

After setting the topic does this get sent back to us? If so, we shouldn't write any message here and just tell the client to update the topic. The server should then send us back to new topic and the code from below will handle it already!

@@ +157,5 @@
> +      this.writeMessage("", _("room.cannotSetTopic"), { system: true });
> +    }
> +  },
> +
> +  get topicSettable() {

No reason for the if-statement here, we can just return the result of the boolean:

```
return (
  this.room &&
  this.room.currentState.maySendEvent("m.room.topic", this._account.userId)
)
```

Do we need to observe some sort of power level events or anything to update when the topic is settable?
Attachment #9133839 - Flags: review?(clokep) → review-

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

Shouldn't the UI not be active if the user cannot set it? I'm confused at
how we would end up in here. Do we do similar checks in IRC/XMPP?

topicSettable takes care of the UI part. If topicSettable is false then user can not edit the topic through UI. I thought Matrix has an implementation of editing topic through command line. So I kept it here, showing an error when use doesn't have permission to edit the topic.

This comes from jsProtoHelper.jsm, the first parameter is "who".

After setting the topic does this get sent back to us? If so, we shouldn't
write any message here and just tell the client to update the topic. The
server should then send us back to new topic and the code from below will
handle it already!

I have rmeoved this part now.

No reason for the if-statement here, we can just return the result of the
boolean:

return (
  this.room &&
  this.room.currentState.maySendEvent("m.room.topic", this._account.userId)
)

Yes, agree.

Do we need to observe some sort of power level events or anything to update
when the topic is settable?

this.room.currentState.maySendEvent("m.room.topic", this._account.userId) takes care of this part i.e. we don't need to decide, SDK does this from it's side only.

Attachment #9133839 - Attachment is obsolete: true
Attachment #9135390 - Flags: review?(clokep)

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

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

Shouldn't the UI not be active if the user cannot set it? I'm confused at
how we would end up in here. Do we do similar checks in IRC/XMPP?

topicSettable takes care of the UI part. If topicSettable is false then user can not edit the topic through UI. I thought Matrix has an implementation of editing topic through command line. So I kept it here, showing an error when use doesn't have permission to edit the topic.

This should live in the command code (once that's implemented), not here.

Do we need to observe some sort of power level events or anything to update
when the topic is settable?

this.room.currentState.maySendEvent("m.room.topic", this._account.userId) takes care of this part i.e. we don't need to decide, SDK does this from it's side only.

Does this only check that once or does it observe changes to permissions and continually update whether the topic is settable?

Does this only check that once or does it observe changes to permissions and continually update whether the topic is settable?

The current state gets updated every time an event gets fired. Following is from the documentation: http://matrix-org.github.io/matrix-js-sdk/0.10.1/module-models_room.html
The state of the room at the time of the newest event in the timeline. Present for backwards compatibility - prefer getLiveTimeline().getState(false).

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

Does this only check that once or does it observe changes to permissions and continually update whether the topic is settable?

The current state gets updated every time an event gets fired. Following is from the documentation: http://matrix-org.github.io/matrix-js-sdk/0.10.1/module-models_room.html
The state of the room at the time of the newest event in the timeline. Present for backwards compatibility - prefer getLiveTimeline().getState(false).

That makes sense, but something has to notify the UI when the permissions actually change or else topicSettable won't be checked again, see some of this code in IRC: https://searchfox.org/comm-central/rev/0c6472bec30280a0648e5101ffacaf72907e4cc2/chat/protocols/irc/irc.jsm#619-624

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

That makes sense, but something has to notify the UI when the permissions actually change or else topicSettable won't be checked again, see some of this code in IRC: https://searchfox.org/comm-central/rev/0c6472bec30280a0648e5101ffacaf72907e4cc2/chat/protocols/irc/irc.jsm#619-624

conv.notifyObservers(this, "chat-update-topic"); call on "RoomMember.powerLevel" will do for us, right?

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

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

That makes sense, but something has to notify the UI when the permissions actually change or else topicSettable won't be checked again, see some of this code in IRC: https://searchfox.org/comm-central/rev/0c6472bec30280a0648e5101ffacaf72907e4cc2/chat/protocols/irc/irc.jsm#619-624

conv.notifyObservers(this, "chat-update-topic"); call on "RoomMember.powerLevel" will do for us, right?

I think so, you might also need to call it from the Matrix event that changes how much power is necessary to set the topic.

Attachment #9135390 - Attachment is obsolete: true
Attachment #9135390 - Flags: review?(clokep)
Attachment #9135464 - Flags: review?(clokep)
Comment on attachment 9135464 [details] [diff] [review]
Bug-1348038_Handle-Topic-Event-2.patch

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

::: chat/protocols/matrix/matrix.jsm
@@ +147,5 @@
> +
> +  set topic(aTopic) {
> +    // Check if our user has the permissions to set the topic.
> +    if (
> +      this.room.currentState.maySendEvent("m.room.topic", this._account.userId)

Maybe just `if (this.topicSettable)` instead of repeating the logic?

@@ +319,5 @@
>        // has only been invited, but has not yet joined).
>        if (participant) {
>          participant._roomMember = member;
>          conv.notifyObservers(participant, "chat-buddy-update");
> +        conv.notifyObservers(null, "chat-update-topic");

Why do we need to notify observers here?

@@ +372,5 @@
>          this._roomList[room.roomId] = conv;
>          conv.initRoom(room);
>          conv.joining = false;
> +        conv.setTopic(
> +          room.currentState.getStateEvents("m.room.topic", "").getContent()

Does this break if there is no topic event?

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

Maybe just if (this.topicSettable) instead of repeating the logic?

Right.

Why do we need to notify observers here?

This will be called when RoomMember.powerLevel is fired. So if power-level of a user changes and his/her power to edit the topic will depend upon it.

Does this break if there is no topic event?

room.currentState.getStateEvents("m.room.topic", "").getContent() will be null and conv will take it as default a topic "No topic message for this room."

Attachment #9135464 - Attachment is obsolete: true
Attachment #9135464 - Flags: review?(clokep)
Attachment #9137966 - Flags: review?(mkmelin+mozilla)
Attachment #9137966 - Flags: review?(mkmelin+mozilla) → review?(clokep)
Comment on attachment 9137966 [details] [diff] [review]
Bug-1348038_Handle-Topic-Event-3.patch

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

I'm not sure why but this patch doesn't work for me. (I did a little debugging but couldn't figure it out.) I'm in the #test:matrix.org room, which has a topic and it isn't showing up.
Attachment #9137966 - Flags: review?(clokep) → review-

Apply patch for "Persist data over multiple restarts". I have added one part there. I don't know why, but one part is there.

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

Apply patch for "Persist data over multiple restarts". I have added one part there. I don't know why, but one part is there.

Can we get that part added to this bug if it is necessary to fix this?

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

Can we get that part added to this bug if it is necessary to fix this?

Sure.

Attachment #9137966 - Attachment is obsolete: true
Attachment #9140524 - Flags: review?(clokep)
Comment on attachment 9140524 [details] [diff] [review]
Bug-1348038_Handle-Topic-Event-4.patch

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

Per our discussion on Matrix this has some hunks in it that are incorrect.
Attachment #9140524 - Flags: review?(clokep) → review-
Attachment #9140524 - Attachment is obsolete: true
Attachment #9140571 - Flags: review?(clokep)
Comment on attachment 9140571 [details] [diff] [review]
Bug-1348038_Handle-Topic-Event-5.patch

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

Yay! Thanks for fixing this!
Attachment #9140571 - Flags: review?(clokep) → review+
Target Milestone: --- → Instandbird 77

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/7e6998a6a0e7
Handle Topic Events in Matrix. r=clokep DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.