From f8eb449dd7d0aa7f77abbc379efa302a328dd1bd Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Fri, 31 May 2019 17:26:09 +0100 Subject: [PATCH 1/5] Show read receipts for hidden events on last shown event This changes how we determine read receipts for the entire message panel. We now calculate read receipts for all events up front, which makes it easier to handle hidden events by moving their read receipts up to the last shown event for display purposes. Part of https://github.com/vector-im/riot-web/issues/9745 --- src/components/structures/MessagePanel.js | 46 +++++++++++++++++++---- 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 657b01c663..5e2414f37a 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -325,6 +325,11 @@ module.exports = React.createClass({ this.currentGhostEventId = null; } + this._readReceiptsByEvent = {}; + if (this.props.showReadReceipts) { + this._readReceiptsByEvent = this._getReadReceiptsByShownEvent(); + } + const isMembershipChange = (e) => e.getType() === 'm.room.member'; for (i = 0; i < this.props.events.length; i++) { @@ -525,10 +530,8 @@ module.exports = React.createClass({ // Local echos have a send "status". const scrollToken = mxEv.status ? undefined : eventId; - let readReceipts; - if (this.props.showReadReceipts) { - readReceipts = this._getReadReceiptsForEvent(mxEv); - } + const readReceipts = this._readReceiptsByEvent[eventId]; + ret.push(
  • { - return r2.ts - r1.ts; - }); + // Get an object that maps from event ID to a list of read receipts that + // should be shown next to that event. If a hidden event has read receipts, + // they are folded into the receipts of the last shown event. + _getReadReceiptsByShownEvent: function() { + const receiptsByEvent = {}; + + let lastShownEventId; + for (const event of this.props.events) { + if (this._shouldShowEvent(event)) { + lastShownEventId = event.getId(); + } + if (!lastShownEventId) { + continue; + } + const existingReceipts = receiptsByEvent[lastShownEventId] || []; + const newReceipts = this._getReadReceiptsForEvent(event); + receiptsByEvent[lastShownEventId] = existingReceipts.concat(newReceipts); + } + + // After grouping receipts by shown events, do another pass to sort each + // receipt list. + for (const eventId in receiptsByEvent) { + receiptsByEvent[eventId].sort((r1, r2) => { + return r2.ts - r1.ts; + }); + } + + return receiptsByEvent; }, _getReadMarkerTile: function(visible) { From 4cea2f0af119ac1cba1d7a427b891af6512275d0 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Mon, 3 Jun 2019 13:28:31 +0100 Subject: [PATCH 2/5] Ensure we always have receipts for users we've seen This adds additional receipt storage to so that we can handle cases where the receipts and events lists get out of sync. If we ever find a user who previously had a receipt but momentarily no longer does, we recover their previous receipt and go with that until we hear something new. Part of https://github.com/vector-im/riot-web/issues/9745 --- src/components/structures/MessagePanel.js | 40 +++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 5e2414f37a..1e7e8f3a6e 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -115,6 +115,18 @@ module.exports = React.createClass({ // to manage its animations this._readReceiptMap = {}; + // Track read receipts by user ID. For each user ID we've ever shown a + // a read receipt for, we store an object: + // { + // lastShownEventId, + // receipt, + // } + // so that we can always keep receipts displayed by reverting back to + // the last shown event for that user ID when needed. This may feel like + // it duplicates the receipt storage in the room, but at this layer, we + // are tracking _shown_ event IDs, which the JS SDK knows nothing about. + this._readReceiptsByUserId = {}; + // Remember the read marker ghost node so we can do the cleanup that // Velocity requires this._readMarkerGhostNode = null; @@ -604,6 +616,7 @@ module.exports = React.createClass({ // they are folded into the receipts of the last shown event. _getReadReceiptsByShownEvent: function() { const receiptsByEvent = {}; + const receiptsByUserId = {}; let lastShownEventId; for (const event of this.props.events) { @@ -613,11 +626,38 @@ module.exports = React.createClass({ if (!lastShownEventId) { continue; } + const existingReceipts = receiptsByEvent[lastShownEventId] || []; const newReceipts = this._getReadReceiptsForEvent(event); receiptsByEvent[lastShownEventId] = existingReceipts.concat(newReceipts); + + // Record these receipts along with their last shown event ID for + // each associated user ID. + for (const receipt of newReceipts) { + receiptsByUserId[receipt.userId] = { + lastShownEventId, + receipt, + }; + } } + // It's possible in some cases (for example, when a read receipt + // advances before we have paginated in the new event that it's marking + // received) that we can temporarily not have a matching event for + // someone which had one in the last. By looking through our previous + // mapping of receipts by user ID, we can cover recover any receipts + // that would have been lost by using the same event ID from last time. + for (const userId in this._readReceiptsByUserId) { + if (receiptsByUserId[userId]) { + continue; + } + const { lastShownEventId, receipt } = this._readReceiptsByUserId[userId]; + const existingReceipts = receiptsByEvent[lastShownEventId] || []; + receiptsByEvent[lastShownEventId] = existingReceipts.concat(receipt); + receiptsByUserId[userId] = { lastShownEventId, receipt }; + } + this._readReceiptsByUserId = receiptsByUserId; + // After grouping receipts by shown events, do another pass to sort each // receipt list. for (const eventId in receiptsByEvent) { From e5abb2e0898306c537b9e02bb8e606ce2ef6be0e Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 4 Jun 2019 11:12:52 +0100 Subject: [PATCH 3/5] Add more read receipt comments --- src/components/structures/MessagePanel.js | 25 +++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index 1e7e8f3a6e..d290efb1a4 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -115,16 +115,37 @@ module.exports = React.createClass({ // to manage its animations this._readReceiptMap = {}; + // Track read receipts by event ID. For each _shown_ event ID, we store + // the list of read receipts to display: + // [ + // { + // userId: string, + // member: RoomMember, + // ts: number, + // }, + // ] + // This is recomputed on each render. It's only stored on the component + // for ease of passing the data around since it's computed in one pass + // over all events. + this._readReceiptsByEvent = {}; + // Track read receipts by user ID. For each user ID we've ever shown a // a read receipt for, we store an object: // { - // lastShownEventId, - // receipt, + // lastShownEventId: string, + // receipt: { + // userId: string, + // member: RoomMember, + // ts: number, + // }, // } // so that we can always keep receipts displayed by reverting back to // the last shown event for that user ID when needed. This may feel like // it duplicates the receipt storage in the room, but at this layer, we // are tracking _shown_ event IDs, which the JS SDK knows nothing about. + // This is recomputed on each render, using the data from the previous + // render as our fallback for any user IDs we can't match a receipt to a + // displayed event in the current render cycle. this._readReceiptsByUserId = {}; // Remember the read marker ghost node so we can do the cleanup that From 06547ef5b308d545919613919ac7c42afeaaaa37 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 4 Jun 2019 15:07:23 +0100 Subject: [PATCH 4/5] Cache hidden events setting Settings is too expensive to query in a hot code path, so this caches the value on the MessagePanel component instead. --- src/components/structures/MessagePanel.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index d290efb1a4..bc9795ff2f 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -152,6 +152,11 @@ module.exports = React.createClass({ // Velocity requires this._readMarkerGhostNode = null; + // Cache hidden events setting on mount since Settings is expensive to + // query, and we check this in a hot code path. + this._showHiddenEventsInTimeline = + SettingsStore.getValue("showHiddenEventsInTimeline"); + this._isMounted = true; }, @@ -292,7 +297,7 @@ module.exports = React.createClass({ return false; // ignored = no show (only happens if the ignore happens after an event was received) } - if (SettingsStore.getValue("showHiddenEventsInTimeline")) { + if (this._showHiddenEventsInTimeline) { return true; } From 8e811fc78e90aaa60d1a1216ada1d84f0e45f90e Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 4 Jun 2019 15:25:54 +0100 Subject: [PATCH 5/5] Use the existing room object For some reason, we were getting the room object for every event during read receipt processing, even though it has been passed in as a prop already. --- src/components/structures/MessagePanel.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/components/structures/MessagePanel.js b/src/components/structures/MessagePanel.js index bc9795ff2f..0078a3fc21 100644 --- a/src/components/structures/MessagePanel.js +++ b/src/components/structures/MessagePanel.js @@ -52,6 +52,10 @@ module.exports = React.createClass({ // ID of an event to highlight. If undefined, no event will be highlighted. highlightedEventId: PropTypes.string, + // The room these events are all in together, if any. + // (The notification panel won't have a room here, for example.) + room: PropTypes.object, + // Should we show URL Previews showUrlPreview: PropTypes.bool, @@ -615,7 +619,7 @@ module.exports = React.createClass({ const myUserId = MatrixClientPeg.get().credentials.userId; // get list of read receipts, sorted most recent first - const room = MatrixClientPeg.get().getRoom(event.getRoomId()); + const { room } = this.props; if (!room) { return null; }