1348038 - Handle Topic Events.

Status: RESOLVED FIXED

(Chat Core :: Matrix, enhancement)

Comment 1

[Pavan Karthik [:matrixisreal]]

Use setRoomTopic(roomId, topic, callback) API call on MatrixClient.

See: http://matrix-org.github.io/matrix-js-sdk/0.7.5/module-client-MatrixClient.html

I don't see any event occuring when a topic is changed.
So handling topic Updates might be tricky?

Comment 2

[Pavan Karthik [:matrixisreal]]

Attachment: topic

Comment 3

[Patrick Cloke [:clokep]]

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.

Comment 4

[Patrick Cloke [:clokep]]

Attachment: WIP v1

Comment 5

[Khushil Mistry [:khushil324]]

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

Comment 6

[Patrick Cloke [:clokep]]

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

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348038_Handle-Topic-Event-0.patch

Apply after patch from Bug 1611444 and Bug 1347533.

Comment 8

[Patrick Cloke [:clokep]]

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?

Comment 9

[Khushil Mistry [:khushil324]]

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

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348038_Handle-Topic-Event-1.patch

Comment 11

[Patrick Cloke [: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?

Comment 12

[Khushil Mistry [:khushil324]]

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

Comment 13

[Patrick Cloke [:clokep]]

(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

Comment 14

[Khushil Mistry [:khushil324]]

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

Comment 15

[Patrick Cloke [:clokep]]

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

Comment 16

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348038_Handle-Topic-Event-2.patch

Comment 17

[Patrick Cloke [: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?

Comment 18

[Khushil Mistry [:khushil324]]

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

Comment 19

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348038_Handle-Topic-Event-3.patch

Comment 20

[Patrick Cloke [: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.

Comment 21

[Khushil Mistry [:khushil324]]

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

Comment 22

[Patrick Cloke [:clokep]]

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

Comment 23

[Khushil Mistry [:khushil324]]

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

Comment 24

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348038_Handle-Topic-Event-4.patch

Comment 25

[Patrick Cloke [: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.

Comment 26

[Khushil Mistry [:khushil324]]

Attachment: Bug-1348038_Handle-Topic-Event-5.patch

Comment 27

[Patrick Cloke [: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!

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7e6998a6a0e7
Handle Topic Events in Matrix. r=clokep DONTBUILD