From d63f83187f8a96e290f54694e5bb88359301d0e5 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Jan 2016 21:18:47 +0000 Subject: [PATCH 01/10] Only show read marker if it's somewhere other than at the bottom, make it animate away and put a short delay before the read marker advances so quickly changing to a room and then away again doesn't advance your read marker. --- src/components/structures/RoomView.js | 76 ++++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 6 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 5cffefad26..9871c54455 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -40,6 +40,7 @@ var dis = require("../../dispatcher"); var PAGINATE_SIZE = 20; var INITIAL_SIZE = 20; +var SEND_READ_RECEIPT_DELAY = 2000; var DEBUG_SCROLL = false; @@ -74,6 +75,8 @@ module.exports = React.createClass({ syncState: MatrixClientPeg.get().getSyncState(), hasUnsentMessages: this._hasUnsentMessages(room), callState: null, + readReceiptEventId: room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId), + readMarkerGhostEventId: undefined, } }, @@ -100,6 +103,12 @@ module.exports = React.createClass({ }, componentWillUnmount: function() { + // if we're waiting to send a read receipt, don't: + // message wasn't on screen for long enough + if (this.sendRRTimer) { + clearTimeout(this.sendRRTimer); + } + if (this.refs.messagePanel) { // disconnect the D&D event listeners from the message panel. This // is really just for hygiene - the messagePanel is going to be @@ -237,7 +246,16 @@ module.exports = React.createClass({ onRoomReceipt: function(receiptEvent, room) { if (room.roomId == this.props.roomId) { - this.forceUpdate(); + var readReceiptEventId = this.state.room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId); + var readMarkerGhostEventId = this.state.readMarkerGhostEventId; + if (this.state.readReceiptEventId !== undefined && this.state.readReceiptEventId != readReceiptEventId) { + var newReadEventIndex = this._indexForEventId(readReceiptEventId); + readMarkerGhostEventId = this.state.readReceiptEventId; + } + this.setState({ + readReceiptEventId: readReceiptEventId, + readMarkerGhostEventId: readMarkerGhostEventId, + }); } }, @@ -649,10 +667,10 @@ module.exports = React.createClass({ var EventTile = sdk.getComponent('rooms.EventTile'); - var prevEvent = null; // the last event we showed - var readReceiptEventId = this.state.room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId); var startIdx = Math.max(0, this.state.room.timeline.length - this.state.messageCap); + var readMarkerIndex; + var ghostIndex; for (var i = startIdx; i < this.state.room.timeline.length; i++) { var mxEv = this.state.room.timeline[i]; @@ -666,6 +684,24 @@ module.exports = React.createClass({ } } + // now we've decided whether or not to show this messages, + // add the read up to marker if appropriate + // doing this here means we implicitly do not show the marker + // if it's at the bottom + // NB. it would be better to decide where the read marker was going + // when the state changed rather than here in the render method, but + // this is where we decide what messages we show so it's the only + // place we know whether we're at the bottom or not. + var self = this; + if (prevEvent && prevEvent.getId() == this.state.readReceiptEventId) { + var hr; + hr = (
); + readMarkerIndex = ret.length; + ret.push(
  • {hr}
    • ); + } + // is this a continuation of the previous message? var continuation = false; if (prevEvent !== null) { @@ -702,13 +738,29 @@ module.exports = React.createClass({ ); - if (eventId == readReceiptEventId) { - ret.push(
    ); + // A read up to marker has died and returned as a ghost! + // Lives in the dom as the ghost of the previous one while it fades away + if (eventId == this.state.readMarkerGhostEventId) { + ghostIndex = i + 1; } prevEvent = mxEv; } + // splice the read marker ghost in now that we know whether the read receipt + // is the last element or not, because we only decide as we're going along. + if (readMarkerIndex === undefined && ghostIndex && ghostIndex <= ret.length) { + var hr; + hr = (
    ); + ret.splice(ghostIndex, 0, ( +
  • {hr}
    • + )); + } + return ret; }, @@ -815,6 +867,7 @@ module.exports = React.createClass({ sendReadReceipt: function() { if (!this.state.room) return; + var currentReadUpToEventId = this.state.room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId); var currentReadUpToEventIndex = this._indexForEventId(currentReadUpToEventId); @@ -822,7 +875,18 @@ module.exports = React.createClass({ if (lastReadEventIndex === null) return; if (lastReadEventIndex > currentReadUpToEventIndex) { - MatrixClientPeg.get().sendReadReceipt(this.state.room.timeline[lastReadEventIndex]); + var self = this; + + var lastReadEventId = self.state.room.timeline[lastReadEventIndex].getId(); + if (this.pendingRR != lastReadEventId) { + this.pendingRR = lastReadEventId; + if (this.sendRRTimer) clearTimeout(this.sendRRTimer); + this.sendRRTimer = setTimeout(function() { + MatrixClientPeg.get().sendReadReceipt(self.state.room.timeline[lastReadEventIndex]); + self.sendRRTimer = undefined; + self.pendingRR = undefined; + }, SEND_READ_RECEIPT_DELAY); + } } }, From 7913b0b465393659d487b2368e313c93e4858be1 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Jan 2016 21:51:14 +0000 Subject: [PATCH 02/10] Length of the returned array, not the index of the timeline event. --- src/components/structures/RoomView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 9871c54455..375ea92686 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -741,7 +741,7 @@ module.exports = React.createClass({ // A read up to marker has died and returned as a ghost! // Lives in the dom as the ghost of the previous one while it fades away if (eventId == this.state.readMarkerGhostEventId) { - ghostIndex = i + 1; + ghostIndex = ret.length; } prevEvent = mxEv; From 1c4d1d250783b6eeaf15f8479eccf02f278b0d97 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Jan 2016 22:19:31 +0000 Subject: [PATCH 03/10] Make scrolling count as user activity. --- src/UserActivity.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/UserActivity.js b/src/UserActivity.js index 3048ad4454..6b918f0729 100644 --- a/src/UserActivity.js +++ b/src/UserActivity.js @@ -31,6 +31,9 @@ class UserActivity { start() { document.onmousemove = this._onUserActivity.bind(this); document.onkeypress = this._onUserActivity.bind(this); + // can't use document.scroll here because that's only the document + // itself being scrolled. Need to use addEventListener's useCapture. + window.addEventListener('scroll', this._onUserActivity.bind(this), true); this.lastActivityAtTs = new Date().getTime(); this.lastDispatchAtTs = 0; } @@ -41,6 +44,7 @@ class UserActivity { stop() { document.onmousemove = undefined; document.onkeypress = undefined; + window.removeEventListener('scroll', this._onUserActivity.bind(this), true); } _onUserActivity(event) { From 7b2d56f6198324fc0a31ab7157396a26ffa6a409 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Jan 2016 22:19:51 +0000 Subject: [PATCH 04/10] Calculate what event we send a read receipt for after the delay so we send a receipt for the one the user actually settles on. --- src/components/structures/RoomView.js | 29 ++++++++++----------------- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 375ea92686..6f9ac51e4a 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -867,27 +867,20 @@ module.exports = React.createClass({ sendReadReceipt: function() { if (!this.state.room) return; + if (this.sendRRTimer) clearTimeout(this.sendRRTimer); + var self = this; + this.sendRRTimer = setTimeout(function() { + self.sendRRTimer = undefined; + var currentReadUpToEventId = self.state.room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId); + var currentReadUpToEventIndex = self._indexForEventId(currentReadUpToEventId); - var currentReadUpToEventId = this.state.room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId); - var currentReadUpToEventIndex = this._indexForEventId(currentReadUpToEventId); + var lastReadEventIndex = self._getLastDisplayedEventIndexIgnoringOwn(); + if (lastReadEventIndex === null) return; - var lastReadEventIndex = this._getLastDisplayedEventIndexIgnoringOwn(); - if (lastReadEventIndex === null) return; - - if (lastReadEventIndex > currentReadUpToEventIndex) { - var self = this; - - var lastReadEventId = self.state.room.timeline[lastReadEventIndex].getId(); - if (this.pendingRR != lastReadEventId) { - this.pendingRR = lastReadEventId; - if (this.sendRRTimer) clearTimeout(this.sendRRTimer); - this.sendRRTimer = setTimeout(function() { - MatrixClientPeg.get().sendReadReceipt(self.state.room.timeline[lastReadEventIndex]); - self.sendRRTimer = undefined; - self.pendingRR = undefined; - }, SEND_READ_RECEIPT_DELAY); + if (lastReadEventIndex > currentReadUpToEventIndex) { + MatrixClientPeg.get().sendReadReceipt(self.state.room.timeline[lastReadEventIndex]); } - } + }, SEND_READ_RECEIPT_DELAY); }, _getLastDisplayedEventIndexIgnoringOwn: function() { From 581111e1a72b131eda3abe5fa324eb0730db7254 Mon Sep 17 00:00:00 2001 From: David Baker Date: Sat, 9 Jan 2016 00:06:54 +0000 Subject: [PATCH 05/10] Use wheel, not scroll as we get scroll events from auto scroll down. Also only do the cursor move check for mouse move events. --- src/UserActivity.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/UserActivity.js b/src/UserActivity.js index 6b918f0729..8b136c0bcc 100644 --- a/src/UserActivity.js +++ b/src/UserActivity.js @@ -33,7 +33,9 @@ class UserActivity { document.onkeypress = this._onUserActivity.bind(this); // can't use document.scroll here because that's only the document // itself being scrolled. Need to use addEventListener's useCapture. - window.addEventListener('scroll', this._onUserActivity.bind(this), true); + // also this needs to be the wheel event, not scroll, as scroll is + // fired when the view scrolls down for a new message. + window.addEventListener('wheel', this._onUserActivity.bind(this), true); this.lastActivityAtTs = new Date().getTime(); this.lastDispatchAtTs = 0; } @@ -44,11 +46,11 @@ class UserActivity { stop() { document.onmousemove = undefined; document.onkeypress = undefined; - window.removeEventListener('scroll', this._onUserActivity.bind(this), true); + window.removeEventListener('wheel', this._onUserActivity.bind(this), true); } _onUserActivity(event) { - if (event.screenX) { + if (event.screenX && event.type == "mousemove") { if (event.screenX === this.lastScreenX && event.screenY === this.lastScreenY) { From 1507b3994052b41771fb88bcb665b41f6fd38968 Mon Sep 17 00:00:00 2001 From: David Baker Date: Sat, 9 Jan 2016 00:07:51 +0000 Subject: [PATCH 06/10] Redundant line. --- src/components/structures/RoomView.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 6f9ac51e4a..2e6ebd2933 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -249,7 +249,6 @@ module.exports = React.createClass({ var readReceiptEventId = this.state.room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId); var readMarkerGhostEventId = this.state.readMarkerGhostEventId; if (this.state.readReceiptEventId !== undefined && this.state.readReceiptEventId != readReceiptEventId) { - var newReadEventIndex = this._indexForEventId(readReceiptEventId); readMarkerGhostEventId = this.state.readReceiptEventId; } this.setState({ From 37f1b4ba8a884690e93a868655f4eac02be0cd0c Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 12 Jan 2016 14:13:42 +0000 Subject: [PATCH 07/10] Tweaked style means we can have 100% width (well 99% otherwise we gain a horizontal scrollbar) --- src/components/structures/RoomView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 2e6ebd2933..5df5a1044e 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -694,7 +694,7 @@ module.exports = React.createClass({ var self = this; if (prevEvent && prevEvent.getId() == this.state.readReceiptEventId) { var hr; - hr = (
    ); readMarkerIndex = ret.length; From 8b730c0a5d7c1aa2db7ddf0227e0d7aa3fedddfb Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 12 Jan 2016 16:38:09 +0000 Subject: [PATCH 08/10] PR feedback --- src/components/structures/RoomView.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 5df5a1044e..e1f8067146 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -75,7 +75,7 @@ module.exports = React.createClass({ syncState: MatrixClientPeg.get().getSyncState(), hasUnsentMessages: this._hasUnsentMessages(room), callState: null, - readReceiptEventId: room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId), + readMarkerEventId: room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId), readMarkerGhostEventId: undefined, } }, @@ -246,13 +246,13 @@ module.exports = React.createClass({ onRoomReceipt: function(receiptEvent, room) { if (room.roomId == this.props.roomId) { - var readReceiptEventId = this.state.room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId); + var readMarkerEventId = this.state.room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId); var readMarkerGhostEventId = this.state.readMarkerGhostEventId; - if (this.state.readReceiptEventId !== undefined && this.state.readReceiptEventId != readReceiptEventId) { - readMarkerGhostEventId = this.state.readReceiptEventId; + if (this.state.readMarkerEventId !== undefined && this.state.readMarkerEventId != readMarkerEventId) { + readMarkerGhostEventId = this.state.readMarkerEventId; } this.setState({ - readReceiptEventId: readReceiptEventId, + readMarkerEventId: readMarkerEventId, readMarkerGhostEventId: readMarkerGhostEventId, }); } @@ -683,7 +683,7 @@ module.exports = React.createClass({ } } - // now we've decided whether or not to show this messages, + // now we've decided whether or not to show this message, // add the read up to marker if appropriate // doing this here means we implicitly do not show the marker // if it's at the bottom @@ -692,7 +692,7 @@ module.exports = React.createClass({ // this is where we decide what messages we show so it's the only // place we know whether we're at the bottom or not. var self = this; - if (prevEvent && prevEvent.getId() == this.state.readReceiptEventId) { + if (prevEvent && prevEvent.getId() == this.state.readMarkerEventId) { var hr; hr = (
    ); From 4a8b5dfe3a4b9d1a663b6fb7a0a68c1fea0dc56d Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 12 Jan 2016 17:18:16 +0000 Subject: [PATCH 09/10] Don't display read markers (or ghosts) above our own messages. --- src/components/structures/RoomView.js | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index e1f8067146..04facc59fc 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -251,6 +251,24 @@ module.exports = React.createClass({ if (this.state.readMarkerEventId !== undefined && this.state.readMarkerEventId != readMarkerEventId) { readMarkerGhostEventId = this.state.readMarkerEventId; } + + + // if the event after the one referenced in the read receipt if sent by us, do nothing since + // this is a temporary period before the synthesized receipt for our own message arrives + var readMarkerGhostEventIndex; + for (var i = 0; i < room.timeline.length; ++i) { + if (room.timeline[i].getId() == readMarkerGhostEventId) { + readMarkerGhostEventIndex = i; + break; + } + } + if (readMarkerGhostEventIndex + 1 < room.timeline.length) { + var nextEvent = room.timeline[readMarkerGhostEventIndex + 1]; + if (nextEvent.sender && nextEvent.sender.userId == MatrixClientPeg.get().credentials.userId) { + readMarkerGhostEventId = undefined; + } + } + this.setState({ readMarkerEventId: readMarkerEventId, readMarkerGhostEventId: readMarkerGhostEventId, @@ -692,7 +710,8 @@ module.exports = React.createClass({ // this is where we decide what messages we show so it's the only // place we know whether we're at the bottom or not. var self = this; - if (prevEvent && prevEvent.getId() == this.state.readMarkerEventId) { + var mxEvSender = mxEv.sender ? mxEv.sender.userId : null; + if (prevEvent && prevEvent.getId() == this.state.readMarkerEventId && mxEvSender != MatrixClientPeg.get().credentials.userId) { var hr; hr = (
    Date: Tue, 12 Jan 2016 17:48:34 +0000 Subject: [PATCH 10/10] Remove ill-concieved delay before sending read receipts & instead just wait a bit before removing the ghost read marker. --- src/components/structures/RoomView.js | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 04facc59fc..b06a4e23f4 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -103,12 +103,6 @@ module.exports = React.createClass({ }, componentWillUnmount: function() { - // if we're waiting to send a read receipt, don't: - // message wasn't on screen for long enough - if (this.sendRRTimer) { - clearTimeout(this.sendRRTimer); - } - if (this.refs.messagePanel) { // disconnect the D&D event listeners from the message panel. This // is really just for hygiene - the messagePanel is going to be @@ -770,7 +764,7 @@ module.exports = React.createClass({ if (readMarkerIndex === undefined && ghostIndex && ghostIndex <= ret.length) { var hr; hr = (
    ); @@ -885,20 +879,15 @@ module.exports = React.createClass({ sendReadReceipt: function() { if (!this.state.room) return; - if (this.sendRRTimer) clearTimeout(this.sendRRTimer); - var self = this; - this.sendRRTimer = setTimeout(function() { - self.sendRRTimer = undefined; - var currentReadUpToEventId = self.state.room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId); - var currentReadUpToEventIndex = self._indexForEventId(currentReadUpToEventId); + var currentReadUpToEventId = this.state.room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId); + var currentReadUpToEventIndex = this._indexForEventId(currentReadUpToEventId); - var lastReadEventIndex = self._getLastDisplayedEventIndexIgnoringOwn(); - if (lastReadEventIndex === null) return; + var lastReadEventIndex = this._getLastDisplayedEventIndexIgnoringOwn(); + if (lastReadEventIndex === null) return; - if (lastReadEventIndex > currentReadUpToEventIndex) { - MatrixClientPeg.get().sendReadReceipt(self.state.room.timeline[lastReadEventIndex]); - } - }, SEND_READ_RECEIPT_DELAY); + if (lastReadEventIndex > currentReadUpToEventIndex) { + MatrixClientPeg.get().sendReadReceipt(this.state.room.timeline[lastReadEventIndex]); + } }, _getLastDisplayedEventIndexIgnoringOwn: function() {