From 488fa3745b567cdff57041b1f81352c484aef0ec Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 9 May 2017 10:03:40 +0100 Subject: [PATCH 1/9] Fix RM not updating if RR event unpaginated 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. --- src/components/structures/ScrollPanel.js | 2 +- src/components/structures/TimelinePanel.js | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index a652bcc827..22eb2240ed 100644 --- a/src/components/structures/ScrollPanel.js +++ b/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 + "'+" + pixelOffset + " (delta: "+scrollDelta+")"); if(scrollDelta != 0) { diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 7c89694a29..d85282b5dc 100644 --- a/src/components/structures/TimelinePanel.js +++ b/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 || + this._indexForEventId(this.state.readMarkerEventId); // We want to avoid sending out read receipts when we are looking at - // events in the past which are before the latest RR. + // events in the past which are before the latest RR/RM. // // For now, let's apply a heuristic: if (a) the event corresponding to // the latest RR (either from the server, or sent by ourselves) doesn't @@ -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)); if (currentReadUpToEventId && currentReadUpToEventIndex === null && this._timelineWindow.canPaginate(EventTimeline.FORWARDS)) { return; @@ -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 ', + this.props.timelineSet.room.roomId, + 'rm', this.state.readMarkerEventId, + 'rr', lastReadEvent.getId(), + ); MatrixClientPeg.get().setRoomReadMarkers( this.props.timelineSet.room.roomId, this.state.readMarkerEventId, From ac25fd6d87214e962897667d7abdd8b766cef2b9 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 9 May 2017 10:16:37 +0100 Subject: [PATCH 2/9] Remove log --- src/components/structures/TimelinePanel.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index d85282b5dc..ba233780ec 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -525,7 +525,6 @@ 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)); if (currentReadUpToEventId && currentReadUpToEventIndex === null && this._timelineWindow.canPaginate(EventTimeline.FORWARDS)) { return; From ca79d9bb6eed092b7b6ca7c7f89ae345624f4047 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Tue, 9 May 2017 17:36:19 +0100 Subject: [PATCH 3/9] Separate predicates for RM/RR 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. --- src/components/structures/ScrollPanel.js | 2 +- src/components/structures/TimelinePanel.js | 27 ++++++++++++---------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 22eb2240ed..a652bcc827 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -575,7 +575,7 @@ module.exports = React.createClass({ var boundingRect = node.getBoundingClientRect(); var scrollDelta = boundingRect.bottom + pixelOffset - wrapperRect.bottom; - console.log("ScrollPanel: scrolling to token '" + scrollToken + "'+" + + debuglog("ScrollPanel: scrolling to token '" + scrollToken + "'+" + pixelOffset + " (delta: "+scrollDelta+")"); if(scrollDelta != 0) { diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index ba233780ec..973c619904 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -504,16 +504,15 @@ var TimelinePanel = React.createClass({ // very possible have logged out within that timeframe, so check // we still have a client. const cli = MatrixClientPeg.get(); - // if no client or client is guest don't send RR + // if no client or client is guest don't send RR or RM if (!cli || cli.isGuest()) return; + let shouldSendRR = true; + var currentReadUpToEventId = this._getCurrentReadReceipt(true); var currentReadUpToEventIndex = this._indexForEventId(currentReadUpToEventId); - - currentReadUpToEventIndex = currentReadUpToEventIndex || - this._indexForEventId(this.state.readMarkerEventId); // We want to avoid sending out read receipts when we are looking at - // events in the past which are before the latest RR/RM. + // events in the past which are before the latest RR. // // For now, let's apply a heuristic: if (a) the event corresponding to // the latest RR (either from the server, or sent by ourselves) doesn't @@ -527,26 +526,30 @@ var TimelinePanel = React.createClass({ // if (currentReadUpToEventId && currentReadUpToEventIndex === null && this._timelineWindow.canPaginate(EventTimeline.FORWARDS)) { - return; + shouldSendRR = false; } var lastReadEventIndex = this._getLastDisplayedEventIndex({ ignoreOwn: true }); - if (lastReadEventIndex === null) return; + if (lastReadEventIndex === null) { + shouldSendRR = false; + } var lastReadEvent = this.state.events[lastReadEventIndex]; + shouldSendRR = shouldSendRR && + (lastReadEventIndex > currentReadUpToEventIndex && + this.last_rr_sent_event_id != lastReadEvent.getId()); + + const shouldSendRM = this.last_rm_sent_event_id != this.state.readMarkerEventId; // we also remember the last read receipt we sent to avoid spamming the // same one at the server repeatedly - if ((lastReadEventIndex > currentReadUpToEventIndex && - 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(); this.last_rm_sent_event_id = this.state.readMarkerEventId; - console.log('TimelinePanel: Sending Read Markers for ', + debuglog('TimelinePanel: Sending Read Markers for ', this.props.timelineSet.room.roomId, 'rm', this.state.readMarkerEventId, 'rr', lastReadEvent.getId(), From 7f766d89c32feba745c3d3000c64e94be5431226 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 10 May 2017 14:42:06 +0100 Subject: [PATCH 4/9] Rename variables, more comments --- src/components/structures/TimelinePanel.js | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 973c619904..874c6b1ac0 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -507,7 +507,7 @@ var TimelinePanel = React.createClass({ // if no client or client is guest don't send RR or RM if (!cli || cli.isGuest()) return; - let shouldSendRR = true; + let shouldSendReadReceipt = true; var currentReadUpToEventId = this._getCurrentReadReceipt(true); var currentReadUpToEventIndex = this._indexForEventId(currentReadUpToEventId); @@ -526,26 +526,30 @@ var TimelinePanel = React.createClass({ // if (currentReadUpToEventId && currentReadUpToEventIndex === null && this._timelineWindow.canPaginate(EventTimeline.FORWARDS)) { - shouldSendRR = false; + shouldSendReadReceipt = false; } var lastReadEventIndex = this._getLastDisplayedEventIndex({ ignoreOwn: true }); if (lastReadEventIndex === null) { - shouldSendRR = false; + shouldSendReadReceipt = false; } var lastReadEvent = this.state.events[lastReadEventIndex]; - shouldSendRR = shouldSendRR && - (lastReadEventIndex > currentReadUpToEventIndex && - this.last_rr_sent_event_id != lastReadEvent.getId()); + shouldSendReadReceipt = shouldSendReadReceipt && + // Only send a RR if the last read Event is ahead in the timeline relative to + // the current RR event. + lastReadEventIndex > currentReadUpToEventIndex && + // Only send a RR if the last RR set != the one we would send + this.last_rr_sent_event_id != lastReadEvent.getId(); - const shouldSendRM = this.last_rm_sent_event_id != this.state.readMarkerEventId; + // Only send a RM if the last RM sent != the one we would send + const shouldSendReadMarker = this.last_rm_sent_event_id != this.state.readMarkerEventId; // we also remember the last read receipt we sent to avoid spamming the // same one at the server repeatedly - if (shouldSendRR || shouldSendRM) { + if (shouldSendReadReceipt || shouldSendReadMarker) { this.last_rr_sent_event_id = lastReadEvent.getId(); this.last_rm_sent_event_id = this.state.readMarkerEventId; From 30e183a7f1e0c5378d837f3ab45dfc8abbe6ade0 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 10 May 2017 14:48:01 +0100 Subject: [PATCH 5/9] Only send RR if we should --- src/components/structures/TimelinePanel.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 874c6b1ac0..26bf9dd400 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -550,23 +550,27 @@ var TimelinePanel = React.createClass({ // we also remember the last read receipt we sent to avoid spamming the // same one at the server repeatedly if (shouldSendReadReceipt || shouldSendReadMarker) { - this.last_rr_sent_event_id = lastReadEvent.getId(); + if (shouldSendReadReceipt) { + this.last_rr_sent_event_id = lastReadEvent.getId(); + } else { + lastReadEvent = null; + } this.last_rm_sent_event_id = this.state.readMarkerEventId; debuglog('TimelinePanel: Sending Read Markers for ', this.props.timelineSet.room.roomId, 'rm', this.state.readMarkerEventId, - 'rr', lastReadEvent.getId(), + lastReadEvent ? 'rr ' + lastReadEvent.getId() : '', ); MatrixClientPeg.get().setRoomReadMarkers( this.props.timelineSet.room.roomId, this.state.readMarkerEventId, - lastReadEvent + lastReadEvent, // Could be null, in which case no RR is sent ).catch((e) => { // /read_markers API is not implemented on this HS, fallback to just RR - if (e.errcode === 'M_UNRECOGNIZED') { + if (e.errcode === 'M_UNRECOGNIZED' && lastReadEvent) { return MatrixClientPeg.get().sendReadReceipt( - lastReadEvent + lastReadEvent, ).catch(() => { this.last_rr_sent_event_id = undefined; }); From fe8ea4ffe7dcc5844c52a3baf96563838cac8a90 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 10 May 2017 14:51:47 +0100 Subject: [PATCH 6/9] Rename vars, linting --- src/components/structures/TimelinePanel.js | 30 +++++++++++----------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 26bf9dd400..8a6c80980e 100644 --- a/src/components/structures/TimelinePanel.js +++ b/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; + this.lastRMSentEventId = undefined; this.dispatcherRef = dis.register(this.onAction); MatrixClientPeg.get().on("Room.timeline", this.onRoomTimeline); @@ -509,8 +509,8 @@ var TimelinePanel = React.createClass({ let shouldSendReadReceipt = true; - var currentReadUpToEventId = this._getCurrentReadReceipt(true); - var currentReadUpToEventIndex = this._indexForEventId(currentReadUpToEventId); + const currentReadUpToEventId = this._getCurrentReadReceipt(true); + const currentReadUpToEventIndex = this._indexForEventId(currentReadUpToEventId); // We want to avoid sending out read receipts when we are looking at // events in the past which are before the latest RR. // @@ -529,33 +529,33 @@ var TimelinePanel = React.createClass({ shouldSendReadReceipt = false; } - var lastReadEventIndex = this._getLastDisplayedEventIndex({ - ignoreOwn: true + const lastReadEventIndex = this._getLastDisplayedEventIndex({ + ignoreOwn: true, }); if (lastReadEventIndex === null) { shouldSendReadReceipt = false; } - - var lastReadEvent = this.state.events[lastReadEventIndex]; + let lastReadEvent = this.state.events[lastReadEventIndex]; shouldSendReadReceipt = shouldSendReadReceipt && // Only send a RR if the last read Event is ahead in the timeline relative to // the current RR event. lastReadEventIndex > currentReadUpToEventIndex && // Only send a RR if the last RR set != the one we would send - this.last_rr_sent_event_id != lastReadEvent.getId(); + this.lastRRSentSentId != lastReadEvent.getId(); // Only send a RM if the last RM sent != the one we would send - const shouldSendReadMarker = this.last_rm_sent_event_id != this.state.readMarkerEventId; + const shouldSendReadMarker = + this.lastRMSentEventId != this.state.readMarkerEventId; // we also remember the last read receipt we sent to avoid spamming the // same one at the server repeatedly if (shouldSendReadReceipt || shouldSendReadMarker) { if (shouldSendReadReceipt) { - this.last_rr_sent_event_id = lastReadEvent.getId(); + this.lastRRSentSentId = lastReadEvent.getId(); } else { lastReadEvent = null; } - this.last_rm_sent_event_id = this.state.readMarkerEventId; + this.lastRMSentEventId = this.state.readMarkerEventId; debuglog('TimelinePanel: Sending Read Markers for ', this.props.timelineSet.room.roomId, @@ -572,12 +572,12 @@ var TimelinePanel = React.createClass({ return MatrixClientPeg.get().sendReadReceipt( lastReadEvent, ).catch(() => { - this.last_rr_sent_event_id = undefined; + this.lastRRSentSentId = undefined; }); } // 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; + this.lastRRSentSentId = undefined; + this.lastRMSentEventId = undefined; }); // do a quick-reset of our unreadNotificationCount to avoid having From 856ef58d4678e4b243f4df94bdc20bb3b6e70efb Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 10 May 2017 14:55:58 +0100 Subject: [PATCH 7/9] fix commen --- src/components/structures/TimelinePanel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 8a6c80980e..08b94fbdbf 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -537,7 +537,7 @@ var TimelinePanel = React.createClass({ } let lastReadEvent = this.state.events[lastReadEventIndex]; shouldSendReadReceipt = shouldSendReadReceipt && - // Only send a RR if the last read Event is ahead in the timeline relative to + // Only send a RR if the last read event is ahead in the timeline relative to // the current RR event. lastReadEventIndex > currentReadUpToEventIndex && // Only send a RR if the last RR set != the one we would send From 3815ad6cd031a9cd14081a18ba6c6c0448fcbc6c Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 11 May 2017 09:20:34 +0100 Subject: [PATCH 8/9] Sent -> Event --- src/components/structures/TimelinePanel.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 08b94fbdbf..5a5abc6257 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -177,7 +177,7 @@ var TimelinePanel = React.createClass({ componentWillMount: function() { debuglog("TimelinePanel: mounting"); - this.lastRRSentSentId = undefined; + this.lastRRSentEventId = undefined; this.lastRMSentEventId = undefined; this.dispatcherRef = dis.register(this.onAction); @@ -541,7 +541,7 @@ var TimelinePanel = React.createClass({ // the current RR event. lastReadEventIndex > currentReadUpToEventIndex && // Only send a RR if the last RR set != the one we would send - this.lastRRSentSentId != lastReadEvent.getId(); + this.lastRRSentEventId != lastReadEvent.getId(); // Only send a RM if the last RM sent != the one we would send const shouldSendReadMarker = @@ -551,7 +551,7 @@ var TimelinePanel = React.createClass({ // same one at the server repeatedly if (shouldSendReadReceipt || shouldSendReadMarker) { if (shouldSendReadReceipt) { - this.lastRRSentSentId = lastReadEvent.getId(); + this.lastRRSentEventId = lastReadEvent.getId(); } else { lastReadEvent = null; } @@ -572,11 +572,11 @@ var TimelinePanel = React.createClass({ return MatrixClientPeg.get().sendReadReceipt( lastReadEvent, ).catch(() => { - this.lastRRSentSentId = undefined; + this.lastRRSentEventId = undefined; }); } // it failed, so allow retries next time the user is active - this.lastRRSentSentId = undefined; + this.lastRRSentEventId = undefined; this.lastRMSentEventId = undefined; }); From 852e1eb72016d71c22b817515547c1d448bdc67a Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Thu, 11 May 2017 09:31:59 +0100 Subject: [PATCH 9/9] Rename some variables `ReadUpTo` -> `RR` `ReadReceipt` -> `RR` `ReadMarker` -> `RM` --- src/components/structures/TimelinePanel.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 5a5abc6257..de43bd1c19 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -507,10 +507,10 @@ var TimelinePanel = React.createClass({ // if no client or client is guest don't send RR or RM if (!cli || cli.isGuest()) return; - let shouldSendReadReceipt = true; + let shouldSendRR = true; - const currentReadUpToEventId = this._getCurrentReadReceipt(true); - const currentReadUpToEventIndex = this._indexForEventId(currentReadUpToEventId); + const currentRREventId = this._getCurrentReadReceipt(true); + const currentRREventIndex = this._indexForEventId(currentRREventId); // We want to avoid sending out read receipts when we are looking at // events in the past which are before the latest RR. // @@ -524,33 +524,33 @@ 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. // - if (currentReadUpToEventId && currentReadUpToEventIndex === null && + if (currentRREventId && currentRREventIndex === null && this._timelineWindow.canPaginate(EventTimeline.FORWARDS)) { - shouldSendReadReceipt = false; + shouldSendRR = false; } const lastReadEventIndex = this._getLastDisplayedEventIndex({ ignoreOwn: true, }); if (lastReadEventIndex === null) { - shouldSendReadReceipt = false; + shouldSendRR = false; } let lastReadEvent = this.state.events[lastReadEventIndex]; - shouldSendReadReceipt = shouldSendReadReceipt && + shouldSendRR = shouldSendRR && // Only send a RR if the last read event is ahead in the timeline relative to // the current RR event. - lastReadEventIndex > currentReadUpToEventIndex && + lastReadEventIndex > currentRREventIndex && // Only send a RR if the last RR set != the one we would send this.lastRRSentEventId != lastReadEvent.getId(); // Only send a RM if the last RM sent != the one we would send - const shouldSendReadMarker = + const shouldSendRM = this.lastRMSentEventId != this.state.readMarkerEventId; // we also remember the last read receipt we sent to avoid spamming the // same one at the server repeatedly - if (shouldSendReadReceipt || shouldSendReadMarker) { - if (shouldSendReadReceipt) { + if (shouldSendRR || shouldSendRM) { + if (shouldSendRR) { this.lastRRSentEventId = lastReadEvent.getId(); } else { lastReadEvent = null;