Sync RM across instances of Riot #805

lukebarnard1 · 2017-04-18T14:19:38Z

This uses the new RM syncing API:

design: https://docs.google.com/document/d/1UWqdS-e1sdwkLDUY0wA4gZyIkRp-ekjsLZ8k6g_Zvso/edit
synapse changes: Implement Read Marker API synapse#2120, m.read_marker -> m.fully_read synapse#2128
js-sdk changes: Implement API for setting RM matrix-js-sdk#419

As detailed here https://docs.google.com/document/d/1UWqdS-e1sdwkLDUY0wA4gZyIkRp-ekjsLZ8k6g_Zvso/edit, the RM state is no longer kept locally, but rather server-side. The client now uses it's locally-calculated RM to update the server and receives server updates via the per-room account data. The sending of the RR has been bundled in to reduce traffic when sending both. In effect, whenever a RR is sent the RM is sent with it but using the new API. This uses a js-sdk change which has set to be finalised and so might change.

Put a XXX to indicate that the ghost tile should be replaced with something mor e stable. As it stands, the ghost will appear, potentially at a different position to the RMs actual position

richvdh · 2017-04-18T16:48:59Z

src/components/structures/MessagePanel.js

@@ -388,6 +388,8 @@ module.exports = React.createClass({
                isVisibleReadMarker = visible;
            }

+            // XXX: there should be no need for a ghost tile - we should just use a


you may be right that there is no need for a ghost tile, but I don't think the reason for its presence is to start the RM animation.

From what I can see, the ghost tile is the one that animates and the ref on the ghost is used to start the animation. This could be so much better. The non-ghost isn't even swapped out (as in there are states where both can be shown), so the client sometimes shows two markers.

ok, but I don't understand why that means you don't need a ghost. It exists to hold the animation.

We could reuse the original marker and start the animation by calling Velocity on it before it moves. Instead of using a ref on the ghost, we'd use it on the original.

richvdh · 2017-04-18T16:50:09Z

src/components/structures/TimelinePanel.js

+            } else {
+                initialReadMarker = this._getCurrentReadReceipt();
+            }
+            console.info('Read marker initially', initialReadMarker);


do we need this?

No, not really

richvdh · 2017-04-18T17:14:52Z

src/components/structures/TimelinePanel.js

@@ -180,6 +182,7 @@ var TimelinePanel = React.createClass({
        MatrixClientPeg.get().on("Room.redaction", this.onRoomRedaction);
        MatrixClientPeg.get().on("Room.receipt", this.onRoomReceipt);
        MatrixClientPeg.get().on("Room.localEchoUpdated", this.onLocalEchoUpdated);
+        MatrixClientPeg.get().on("Room.accountData", this.onAccountData);


don't forget to remove it!

richvdh · 2017-04-18T17:26:39Z

src/components/structures/TimelinePanel.js

-                && this.last_rr_sent_event_id != lastReadEvent.getId()) {
+        if ((lastReadEventIndex > currentReadUpToEventIndex &&
+            this.last_rr_sent_event_id != lastReadEvent.getId()) ||
+                this.last_rm_sent_event_id != this.state.readMarkerEventId) {


initialise this in componentWillMount please

richvdh · 2017-04-18T17:26:58Z

src/components/structures/TimelinePanel.js

            });
+            console.log('TimelinePanel: Read marker sent to the server ', this.state.readMarkerEventId, );


do we need this? looks spammy

richvdh · 2017-04-18T17:27:38Z

src/components/structures/TimelinePanel.js

@@ -706,7 +735,7 @@ var TimelinePanel = React.createClass({

        // the messagePanel doesn't know where the read marker is.
        // if we know the timestamp of the read marker, make a guess based on that.
-        var rmTs = TimelinePanel.roomReadMarkerTsMap[this.props.timelineSet.roomId];
+        const rmTs = TimelinePanel.roomReadMarkerTsMap[this.props.timelineSet.room.roomId];


huh. Has this been broken for a while?

Looks like it! This is reached if MessagePanel has not got the related event paginated in.

It doesn't look like much is done with the result of TimelinePanel.getReadMarkerPosition() other than checking that it's in the view-port when updateReadMarker is called (which might explain why we were seeing it following when scrolling down) and specifying whether it's visible to MessagePanel.

richvdh · 2017-04-18T17:29:46Z

src/components/structures/TimelinePanel.js

@@ -414,9 +417,10 @@ var TimelinePanel = React.createClass({
                } else if(lastEv && this.getReadMarkerPosition() === 0) {
                    // we know we're stuckAtBottom, so we can advance the RM
                    // immediately, to save a later render cycle
+
+                    // This call will setState with readMarkerEventId = lastEv.getId()


... except that the true argument is called inhibitSetState - so I don't think it will.

richvdh · 2017-04-18T17:31:14Z

src/components/structures/TimelinePanel.js

+        if (ev.getType() !== "m.fully_read") return;
+
+        const markerEventId = ev.getContent().event_id;
+        console.log('TimelinePanel: Read marker received from server', markerEventId);


do we need this? looks spammy

richvdh · 2017-04-18T17:32:56Z

src/components/structures/TimelinePanel.js

+        const markerEventId = ev.getContent().event_id;
+        console.log('TimelinePanel: Read marker received from server', markerEventId);
+
+        this.setState({


it seems like a problem that we aren't updating roomReadMarkerTsMap here.

I suspect we actually no longer need roomReadMarkerTsMap, but if that's true, let's get rid of it (preferably in a separate PR) rather than leave it in an inconsistent state.

richvdh · 2017-04-18T17:33:37Z

src/components/structures/TimelinePanel.js

@@ -956,16 +985,10 @@ var TimelinePanel = React.createClass({
    _setReadMarker: function(eventId, eventTs, inhibitSetState) {
        var roomId = this.props.timelineSet.room.roomId;

-        if (TimelinePanel.roomReadMarkerMap[roomId] == eventId) {
-            // don't update the state (and cause a re-render) if there is


this comment still looks valid?

fair point!

This will become redundant when there is server support for directionality of the RM

richvdh

lgtm

richvdh · 2017-04-21T10:50:23Z

src/components/structures/TimelinePanel.js

                // it failed, so allow retries next time the user is active
-                this.last_rr_sent_event_id = undefined;
                this.last_rm_sent_event_id = undefined;


won't this mean we hit the API every time the user moves the mouse? I feel like we should leave this set if sendReadReceipt succeeds.

If sendReadReceipt is called, it'll return a promise this line is not reached.

richvdh · 2017-04-21T10:52:32Z

src/components/structures/TimelinePanel.js

-            ).catch(() => {
+            ).catch((e) => {
+                // /read_markers API is not implemented on this HS, fallback to just RR
+                if (e.errcode === 'M_UNRECOGNIZED') {


what should be the behaviour for other errors? should we not set last_rr_sent_event_id = undefined so that we retry?

Actually, yes I think that's the Right Way to do it

Indicate that setting the RR was a failure and that hitting the API should be retried (in the case where the errcode !== "M_UNRECOGNISED")

richvdh

lgtm

Luke Barnard added 9 commits April 12, 2017 15:04

Prevent the ghost and real RM tile from both appearing

424aae6

Fix bug where roomId was expected to be a property on timelineSet

249e427

Remove redundant setting of readMarkerEventId

9c9dc84

m.read_marker -> m.fully_read

28ed69b

Merge branch 'develop' into luke/feature-read-marker

4b5364e

marker -> event_id

d33afa9

Undo change in ghost RM logic

d38520d

Put a XXX to indicate that the ghost tile should be replaced with something mor e stable. As it stands, the ghost will appear, potentially at a different position to the RMs actual position

Clarify comment

c431c9a

lukebarnard1 assigned richvdh Apr 18, 2017

richvdh suggested changes Apr 18, 2017

View reviewed changes

richvdh assigned lukebarnard1 Apr 18, 2017

Luke Barnard added 8 commits April 19, 2017 10:14

Update to match renamed API

81bdfe2

Remove log

28818b8

Remove Room.accountData listener on unmount

e32f153

Revert change

00cf5b5

Remove spammy log

a787ee8

Make note of inconsistant roomReadMarkerTsMap

81bf2be

This will become redundant when there is server support for directionality of the RM

Initialise last_rm_sent_event_id

edeaef8

Remove log, reinstate comment

a4ba5f0

lukebarnard1 removed their assignment Apr 19, 2017

richvdh approved these changes Apr 19, 2017

View reviewed changes

richvdh assigned lukebarnard1 and unassigned richvdh Apr 19, 2017

If new RR-RM API not implemented, fallback to RR-only API

67089cb

richvdh reviewed Apr 21, 2017

View reviewed changes

reset last_rr_sent on error

29c2bd3

Indicate that setting the RR was a failure and that hitting the API should be retried (in the case where the errcode !== "M_UNRECOGNISED")

richvdh approved these changes Apr 21, 2017

View reviewed changes

lukebarnard1 merged commit ae8d04b into develop Apr 21, 2017

lukebarnard1 mentioned this pull request Apr 21, 2017

Self-read markers should be synchronised across clients element-hq/element-web#1159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync RM across instances of Riot #805

Sync RM across instances of Riot #805

lukebarnard1 commented Apr 18, 2017

richvdh Apr 18, 2017

lukebarnard1 Apr 19, 2017 •

edited

Loading

richvdh Apr 19, 2017

lukebarnard1 Apr 20, 2017

richvdh Apr 18, 2017

lukebarnard1 Apr 19, 2017

richvdh Apr 18, 2017

lukebarnard1 Apr 19, 2017

richvdh Apr 18, 2017

lukebarnard1 Apr 19, 2017

richvdh Apr 18, 2017

richvdh Apr 18, 2017

lukebarnard1 Apr 19, 2017

richvdh Apr 18, 2017

lukebarnard1 Apr 19, 2017

richvdh Apr 18, 2017

lukebarnard1 Apr 19, 2017

richvdh Apr 18, 2017

richvdh Apr 18, 2017

lukebarnard1 Apr 19, 2017

richvdh left a comment

richvdh Apr 21, 2017

lukebarnard1 Apr 21, 2017

richvdh Apr 21, 2017

richvdh Apr 21, 2017

lukebarnard1 Apr 21, 2017

richvdh left a comment

		});
		console.log('TimelinePanel: Read marker sent to the server ', this.state.readMarkerEventId, );

Sync RM across instances of Riot #805

Sync RM across instances of Riot #805

Conversation

lukebarnard1 commented Apr 18, 2017

Choose a reason for hiding this comment

lukebarnard1 Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

lukebarnard1 Apr 19, 2017 •

edited

Loading