Open Bug 1348038 Opened 3 years ago Updated 9 days ago

Handle Topic Events.

Categories

(Chat Core :: Matrix, enhancement)

enhancement
Not set

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: matrixisreal, Assigned: khushil324)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 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
Blocks: 1572636
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)
You need to log in before you can comment on or make changes to this bug.