Fix RM not updating if RR event unpaginated #874

lukebarnard1 · 2017-05-09T09:05:27Z

If the RR event has been unpaginated, the logic in sendReadReceipt will now fallback on the event ID of the RM which in theory is always =< RR event ID stream-wise.

Also add some logging to debug RM bugs.

If the RR event has been unpaginated, the logic in `sendReadReceipt` will now fallback on the event ID of the RM which in theory is always =< RR event ID stream-wise.

richvdh · 2017-05-09T09:07:34Z

src/components/structures/ScrollPanel.js

@@ -575,7 +575,7 @@ module.exports = React.createClass({
        var boundingRect = node.getBoundingClientRect();
        var scrollDelta = boundingRect.bottom + pixelOffset - wrapperRect.bottom;

-        debuglog("ScrollPanel: scrolling to token '" + scrollToken + "'+" +
+        console.log("ScrollPanel: scrolling to token '" + scrollToken + "'+" +


richvdh · 2017-05-09T09:07:55Z

src/components/structures/TimelinePanel.js

@@ -523,6 +525,7 @@ var TimelinePanel = React.createClass({
        // RRs) - but that is a bit of a niche case. It will sort itself out when
        // the user eventually hits the live timeline.
        //
+        console.log(currentReadUpToEventId, currentReadUpToEventIndex, this._timelineWindow.canPaginate(EventTimeline.FORWARDS));


richvdh · 2017-05-09T09:08:13Z

src/components/structures/TimelinePanel.js

@@ -544,6 +547,11 @@ var TimelinePanel = React.createClass({
            this.last_rr_sent_event_id = lastReadEvent.getId();
            this.last_rm_sent_event_id = this.state.readMarkerEventId;

+            console.log('TimelinePanel: Sending Read Markers for ',


richvdh

I'm unconvinced this is the right thing to do. In particular there is a comment saying that we don't want to send out RRs if we don't know what we're doing. Why can't you just leave the RR alone rather than sending an old one?

richvdh · 2017-05-09T09:13:55Z

src/components/structures/TimelinePanel.js

@@ -510,8 +510,10 @@ var TimelinePanel = React.createClass({
        var currentReadUpToEventId = this._getCurrentReadReceipt(true);
        var currentReadUpToEventIndex = this._indexForEventId(currentReadUpToEventId);

+        currentReadUpToEventIndex = currentReadUpToEventIndex ||


needs a comment to explain what we're doing.

Isn't zero a valid index? why do we fall back in that case?

lukebarnard1 · 2017-05-09T09:23:15Z

Why can't you just leave the RR alone rather than sending an old one?

This is a fair point. I'll rewrite this to be a bit more stable and to accommodate RMs more sensibly.

Any suggestions as to how to log things that I'd like to include in the logs are welcome.

richvdh · 2017-05-09T09:27:41Z

Any suggestions as to how to log things that I'd like to include in the logs are welcome.

If you really want to include them in the rageshakes, then console.log it is. But do we really need to be this verbose? Could you use debuglog and set DEBUG=1 when investigating a problem?

lukebarnard1 · 2017-05-09T10:33:37Z

Could you use debuglog and set DEBUG=1 when investigating a problem?

That's largely what I have been doing but this is subtle enough to be very difficult to reproduce when someone sends a rageshake about one particular instance of the RM not updating. TBH if I rewrite sendReadReceipt the logs will hopefully become redundant. I'll remove them in the process of rewriting.

Instead of modifying the condition for updating the RR, separate the RM and RR conditions and use an OR to decide when to set both. Make some logs only log when DEBUG.

richvdh · 2017-05-10T09:30:03Z

src/components/structures/TimelinePanel.js

        if (!cli || cli.isGuest()) return;

+        let shouldSendRR = true;
+
        var currentReadUpToEventId = this._getCurrentReadReceipt(true);


it might be worth renaming these as 'read receipt' for consistency with the rest of the function - I'm finding it horribly confusing

richvdh · 2017-05-10T09:37:29Z

src/components/structures/TimelinePanel.js


        var lastReadEvent = this.state.events[lastReadEventIndex];
+        shouldSendRR = shouldSendRR &&


this could do with a comment: "only send a RR if ..."

richvdh · 2017-05-10T09:39:00Z

src/components/structures/TimelinePanel.js

-            this.last_rr_sent_event_id != lastReadEvent.getId()) ||
-                this.last_rm_sent_event_id != this.state.readMarkerEventId) {
-
+        if (shouldSendRR || shouldSendRM) {
            this.last_rr_sent_event_id = lastReadEvent.getId();


Looks like it's possible for lastReadEvent to be undefined here (eg if lastReadEventIndex was null), which would make this blow up.

This is now protected by shouldSendReadReceipt which is set to false if the index is null.

richvdh · 2017-05-10T09:40:48Z

src/components/structures/TimelinePanel.js

-            this.last_rr_sent_event_id != lastReadEvent.getId()) ||
-                this.last_rm_sent_event_id != this.state.readMarkerEventId) {
-
+        if (shouldSendRR || shouldSendRM) {


surely if we've decided not to send a RR, then we, uh, shouldn't send a RR even if shouldSendRM is true?

(in other words: pass null in the call to setRoomReadMarkers and don't update last_rr_sent_event_id)

richvdh · 2017-05-10T17:38:00Z

src/components/structures/TimelinePanel.js

@@ -177,8 +177,8 @@ var TimelinePanel = React.createClass({
    componentWillMount: function() {
        debuglog("TimelinePanel: mounting");

-        this.last_rr_sent_event_id = undefined;
-        this.last_rm_sent_event_id = undefined;
+        this.lastRRSentSentId = undefined;


s/Sent/Event
(and throughout)

richvdh · 2017-05-10T17:38:39Z

src/components/structures/TimelinePanel.js


+        const currentReadUpToEventId = this._getCurrentReadReceipt(true);


thought you were going to rename these :/

oh right, I understood something else and agree they should be consistent

`ReadUpTo` -> `RR` `ReadReceipt` -> `RR` `ReadMarker` -> `RM`

richvdh

lgtm I think

Fix RM not updating if RR event unpaginated

488fa37

If the RR event has been unpaginated, the logic in `sendReadReceipt` will now fallback on the event ID of the RM which in theory is always =< RR event ID stream-wise.

lukebarnard1 assigned richvdh May 9, 2017

richvdh reviewed May 9, 2017

View reviewed changes

richvdh suggested changes May 9, 2017

View reviewed changes

Remove log

ac25fd6

Separate predicates for RM/RR

ca79d9b

Instead of modifying the condition for updating the RR, separate the RM and RR conditions and use an OR to decide when to set both. Make some logs only log when DEBUG.

richvdh reviewed May 10, 2017

View reviewed changes

richvdh assigned lukebarnard1 and unassigned richvdh May 10, 2017

Luke Barnard added 4 commits May 10, 2017 14:42

Rename variables, more comments

7f766d8

Only send RR if we should

30e183a

Rename vars, linting

fe8ea4f

fix commen

856ef58

lukebarnard1 assigned richvdh and unassigned lukebarnard1 May 10, 2017

richvdh reviewed May 10, 2017

View reviewed changes

richvdh assigned lukebarnard1 and unassigned richvdh May 10, 2017

Luke Barnard added 2 commits May 11, 2017 09:20

Sent -> Event

3815ad6

Rename some variables

852e1eb

`ReadUpTo` -> `RR` `ReadReceipt` -> `RR` `ReadMarker` -> `RM`

lukebarnard1 assigned richvdh and unassigned lukebarnard1 May 11, 2017

richvdh approved these changes May 11, 2017

View reviewed changes

richvdh assigned lukebarnard1 and unassigned richvdh May 11, 2017

lukebarnard1 merged commit 2437eb7 into develop May 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RM not updating if RR event unpaginated #874

Fix RM not updating if RR event unpaginated #874

lukebarnard1 commented May 9, 2017 •

edited

Loading

richvdh May 9, 2017

richvdh May 9, 2017

lukebarnard1 May 9, 2017

richvdh May 9, 2017

richvdh left a comment

richvdh May 9, 2017

lukebarnard1 commented May 9, 2017

richvdh commented May 9, 2017

lukebarnard1 commented May 9, 2017

richvdh May 10, 2017

lukebarnard1 May 10, 2017

richvdh May 10, 2017

richvdh May 10, 2017

lukebarnard1 May 10, 2017

richvdh May 10, 2017

richvdh May 10, 2017

lukebarnard1 May 11, 2017

richvdh May 10, 2017

lukebarnard1 May 11, 2017

richvdh left a comment


		var lastReadEvent = this.state.events[lastReadEventIndex];
		shouldSendRR = shouldSendRR &&


		const currentReadUpToEventId = this._getCurrentReadReceipt(true);

Fix RM not updating if RR event unpaginated #874

Fix RM not updating if RR event unpaginated #874

Conversation

lukebarnard1 commented May 9, 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

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 commented May 9, 2017

richvdh commented May 9, 2017

lukebarnard1 commented May 9, 2017

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

lukebarnard1 commented May 9, 2017 •

edited

Loading