From 8ac54661bef571b2f0ff167d6213dc848fbcf416 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 12 Jul 2019 17:40:51 +0200 Subject: [PATCH 1/3] take adjacent no-tile events in combination with ignored events into account when determining the last displayed event --- src/components/structures/TimelinePanel.js | 92 +++++++++++++--------- 1 file changed, 54 insertions(+), 38 deletions(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 354d52a6f1..379939154e 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -36,6 +36,7 @@ const Modal = require("../../Modal"); const UserActivity = require("../../UserActivity"); import { KeyCode } from '../../Keyboard'; import Timer from '../../utils/Timer'; +import shouldHideEvent from '../../shouldHideEvent'; import EditorStateTransfer from '../../utils/EditorStateTransfer'; const PAGINATE_SIZE = 20; @@ -1140,55 +1141,70 @@ const TimelinePanel = React.createClass({ const wrapperRect = ReactDOM.findDOMNode(messagePanel).getBoundingClientRect(); const myUserId = MatrixClientPeg.get().credentials.userId; - let lastDisplayedIndex = null; + const isNodeInView = (node) => { + if (node) { + const boundingRect = node.getBoundingClientRect(); + if ((allowPartial && boundingRect.top < wrapperRect.bottom) || + (!allowPartial && boundingRect.bottom < wrapperRect.bottom)) { + return true; + } + } + return false; + }; + + // if allowEventsWithoutTiles is enabled, we keep track + // of how many of the adjacent events were invisible (because no tile) + // but should have the read receipt moved past, so + // we can include those once we find the last displayed (visible) event. + // The counter is cleared when we ignore an event we don't want + // to send a read receipt for (our own events, local echos) + let adjacentInvisibleEventCount = 0; // Use `liveEvents` here because we don't want the read marker or read // receipt to advance into pending events. for (let i = this.state.liveEvents.length - 1; i >= 0; --i) { const ev = this.state.liveEvents[i]; - if (ignoreOwn && ev.sender && ev.sender.userId == myUserId) { - continue; - } - - // local echoes have a fake event ID - if (ignoreEchoes && ev.status) { - continue; - } - const node = messagePanel.getNodeForEventId(ev.getId()); - if (!node) continue; + const isInView = isNodeInView(node); - const boundingRect = node.getBoundingClientRect(); - if ((allowPartial && boundingRect.top < wrapperRect.bottom) || - (!allowPartial && boundingRect.bottom < wrapperRect.bottom)) { - lastDisplayedIndex = i; - break; - } - } - - if (lastDisplayedIndex === null) { - return null; - } - - // If events without tiles are allowed, then we walk forward from the - // the last displayed event and advance the index for any events without - // tiles that immediately follow it. - // XXX: We could track the last event without a tile after the last - // displayed event in the loop above so that we only do a single pass - // through the loop, which would be more efficient. Using two passes is - // easier to reason about, so let's start there and optimise later if - // needed. - if (allowEventsWithoutTiles) { - for (let i = lastDisplayedIndex + 1; i < this.state.liveEvents.length; i++) { - const ev = this.state.liveEvents[i]; - if (EventTile.haveTileForEvent(ev)) { - break; + // the event at i + adjacentInvisibleEventCount should + // not be ignored, and it considered the first in view (at the bottom) + // because i is the first one in view and the adjacent ones are invisible, + // so return this without further ado. + if (isInView && adjacentInvisibleEventCount !== 0) { + return i + adjacentInvisibleEventCount; + } else { + if (node && !isInView) { + // has node but not in view, so adjacent invisible events don't count + adjacentInvisibleEventCount = 0; + } + + const shouldIgnore = (ignoreEchoes && ev.status) || // local echo + (ignoreOwn && ev.sender && ev.sender.userId == myUserId); // own message + const isWithoutTile = !EventTile.haveTileForEvent(ev) || shouldHideEvent(ev); + + if (allowEventsWithoutTiles && (isWithoutTile || !node)) { + // don't start counting if the event should be ignored, + // but continue counting if we were already so the offset + // to the previous invisble event that didn't need to be ignored + // doesn't get messed up + if (!shouldIgnore || (shouldIgnore && adjacentInvisibleEventCount !== 0)) { + ++adjacentInvisibleEventCount; + } + continue; + } + + if (shouldIgnore) { + continue; + } + + if (isInView) { + return i; } - lastDisplayedIndex = i; } } - return lastDisplayedIndex; + return null; }, /** From 034883dc7e8d5277514d690c4578e8a65317f508 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 15 Jul 2019 14:01:28 +0200 Subject: [PATCH 2/3] improve comments --- src/components/structures/TimelinePanel.js | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 379939154e..5acc55b4d3 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -1153,11 +1153,11 @@ const TimelinePanel = React.createClass({ }; // if allowEventsWithoutTiles is enabled, we keep track - // of how many of the adjacent events were invisible (because no tile) - // but should have the read receipt moved past, so + // of how many of the adjacent events didn't have a tile + // but should have the read receipt moved past them, so // we can include those once we find the last displayed (visible) event. - // The counter is cleared when we ignore an event we don't want - // to send a read receipt for (our own events, local echos) + // The counter is not started for events we don't want + // to send a read receipt for (our own events, local echos). let adjacentInvisibleEventCount = 0; // Use `liveEvents` here because we don't want the read marker or read // receipt to advance into pending events. @@ -1167,15 +1167,14 @@ const TimelinePanel = React.createClass({ const node = messagePanel.getNodeForEventId(ev.getId()); const isInView = isNodeInView(node); - // the event at i + adjacentInvisibleEventCount should - // not be ignored, and it considered the first in view (at the bottom) - // because i is the first one in view and the adjacent ones are invisible, - // so return this without further ado. + // when we've reached the first visible event, and the previous + // events were all invisible (with the first one not being ignored), + // return the index of the first invisible event. if (isInView && adjacentInvisibleEventCount !== 0) { return i + adjacentInvisibleEventCount; } else { if (node && !isInView) { - // has node but not in view, so adjacent invisible events don't count + // has node but not in view, so reset adjacent invisible events adjacentInvisibleEventCount = 0; } From 7e25e1b2fca0d430411e626c574d0ee4aa9c8351 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 15 Jul 2019 14:02:41 +0200 Subject: [PATCH 3/3] remove unneeded else --- src/components/structures/TimelinePanel.js | 45 +++++++++++----------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 5acc55b4d3..6eb1599147 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -1172,34 +1172,33 @@ const TimelinePanel = React.createClass({ // return the index of the first invisible event. if (isInView && adjacentInvisibleEventCount !== 0) { return i + adjacentInvisibleEventCount; - } else { - if (node && !isInView) { - // has node but not in view, so reset adjacent invisible events - adjacentInvisibleEventCount = 0; - } + } + if (node && !isInView) { + // has node but not in view, so reset adjacent invisible events + adjacentInvisibleEventCount = 0; + } - const shouldIgnore = (ignoreEchoes && ev.status) || // local echo - (ignoreOwn && ev.sender && ev.sender.userId == myUserId); // own message - const isWithoutTile = !EventTile.haveTileForEvent(ev) || shouldHideEvent(ev); + const shouldIgnore = (ignoreEchoes && ev.status) || // local echo + (ignoreOwn && ev.sender && ev.sender.userId == myUserId); // own message + const isWithoutTile = !EventTile.haveTileForEvent(ev) || shouldHideEvent(ev); - if (allowEventsWithoutTiles && (isWithoutTile || !node)) { - // don't start counting if the event should be ignored, - // but continue counting if we were already so the offset - // to the previous invisble event that didn't need to be ignored - // doesn't get messed up - if (!shouldIgnore || (shouldIgnore && adjacentInvisibleEventCount !== 0)) { - ++adjacentInvisibleEventCount; - } - continue; + if (allowEventsWithoutTiles && (isWithoutTile || !node)) { + // don't start counting if the event should be ignored, + // but continue counting if we were already so the offset + // to the previous invisble event that didn't need to be ignored + // doesn't get messed up + if (!shouldIgnore || (shouldIgnore && adjacentInvisibleEventCount !== 0)) { + ++adjacentInvisibleEventCount; } + continue; + } - if (shouldIgnore) { - continue; - } + if (shouldIgnore) { + continue; + } - if (isInView) { - return i; - } + if (isInView) { + return i; } }