Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/components/structures/MessagePanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,8 @@ module.exports = React.createClass({
isVisibleReadMarker = visible;
}

// XXX: there should be no need for a ghost tile - we should just use a
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may be right that there is no need for a ghost tile, but I don't think the reason for its presence is to start the RM animation.

Copy link
Copy Markdown
Contributor Author

@lukebarnard1 lukebarnard1 Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can see, the ghost tile is the one that animates and the ref on the ghost is used to start the animation. This could be so much better. The non-ghost isn't even swapped out (as in there are states where both can be shown), so the client sometimes shows two markers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, but I don't understand why that means you don't need a ghost. It exists to hold the animation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could reuse the original marker and start the animation by calling Velocity on it before it moves. Instead of using a ref on the ghost, we'd use it on the original.

// a dispatch (user_activity_end) to start the RM animation.
if (eventId == this.currentGhostEventId) {
// if we're showing an animation, continue to show it.
ret.push(this._getReadMarkerGhostTile());
Expand Down
69 changes: 51 additions & 18 deletions src/components/structures/TimelinePanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,6 @@ var TimelinePanel = React.createClass({
},

statics: {
// a map from room id to read marker event ID
roomReadMarkerMap: {},

// a map from room id to read marker event timestamp
roomReadMarkerTsMap: {},
},
Expand All @@ -121,10 +118,14 @@ var TimelinePanel = React.createClass({
getInitialState: function() {
// XXX: we could track RM per TimelineSet rather than per Room.
// but for now we just do it per room for simplicity.
let initialReadMarker = null;
if (this.props.manageReadMarkers) {
var initialReadMarker =
TimelinePanel.roomReadMarkerMap[this.props.timelineSet.room.roomId]
|| this._getCurrentReadReceipt();
const readmarker = this.props.timelineSet.room.getAccountData('m.fully_read');
if (readmarker){
initialReadMarker = readmarker.getContent().event_id;
} else {
initialReadMarker = this._getCurrentReadReceipt();
}
}

return {
Expand Down Expand Up @@ -173,13 +174,15 @@ var TimelinePanel = React.createClass({
debuglog("TimelinePanel: mounting");

this.last_rr_sent_event_id = undefined;
this.last_rm_sent_event_id = undefined;

this.dispatcherRef = dis.register(this.onAction);
MatrixClientPeg.get().on("Room.timeline", this.onRoomTimeline);
MatrixClientPeg.get().on("Room.timelineReset", this.onRoomTimelineReset);
MatrixClientPeg.get().on("Room.redaction", this.onRoomRedaction);
MatrixClientPeg.get().on("Room.receipt", this.onRoomReceipt);
MatrixClientPeg.get().on("Room.localEchoUpdated", this.onLocalEchoUpdated);
MatrixClientPeg.get().on("Room.accountData", this.onAccountData);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't forget to remove it!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


this._initTimeline(this.props);
},
Expand Down Expand Up @@ -247,6 +250,7 @@ var TimelinePanel = React.createClass({
client.removeListener("Room.redaction", this.onRoomRedaction);
client.removeListener("Room.receipt", this.onRoomReceipt);
client.removeListener("Room.localEchoUpdated", this.onLocalEchoUpdated);
client.removeListener("Room.accountData", this.onAccountData);
}
},

Expand Down Expand Up @@ -414,6 +418,7 @@ var TimelinePanel = React.createClass({
} else if(lastEv && this.getReadMarkerPosition() === 0) {
// we know we're stuckAtBottom, so we can advance the RM
// immediately, to save a later render cycle

this._setReadMarker(lastEv.getId(), lastEv.getTs(), true);
updatedState.readMarkerVisible = false;
updatedState.readMarkerEventId = lastEv.getId();
Expand Down Expand Up @@ -466,6 +471,21 @@ var TimelinePanel = React.createClass({
this._reloadEvents();
},

onAccountData: function(ev, room) {
if (this.unmounted) return;

// ignore events for other rooms
if (room !== this.props.timelineSet.room) return;

if (ev.getType() !== "m.fully_read") return;

// XXX: roomReadMarkerTsMap not updated here so it is now inconsistent. Replace
// this mechanism of determining where the RM is relative to the view-port with
// one supported by the server (the client needs more than an event ID).
this.setState({
readMarkerEventId: ev.getContent().event_id,
}, this.props.onReadMarkerUpdated);
},

sendReadReceipt: function() {
if (!this.refs.messagePanel) return;
Expand Down Expand Up @@ -505,12 +525,29 @@ var TimelinePanel = React.createClass({

// 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()) {
if ((lastReadEventIndex > currentReadUpToEventIndex &&
this.last_rr_sent_event_id != lastReadEvent.getId()) ||
this.last_rm_sent_event_id != this.state.readMarkerEventId) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialise this in componentWillMount please

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


this.last_rr_sent_event_id = lastReadEvent.getId();
MatrixClientPeg.get().sendReadReceipt(lastReadEvent).catch(() => {
this.last_rm_sent_event_id = this.state.readMarkerEventId;

MatrixClientPeg.get().setRoomReadMarkers(
this.props.timelineSet.room.roomId,
this.state.readMarkerEventId,
lastReadEvent
).catch((e) => {
// /read_markers API is not implemented on this HS, fallback to just RR
if (e.errcode === 'M_UNRECOGNIZED') {
return MatrixClientPeg.get().sendReadReceipt(
lastReadEvent
).catch(() => {
this.last_rr_sent_event_id = 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;
});

// do a quick-reset of our unreadNotificationCount to avoid having
Expand Down Expand Up @@ -706,7 +743,7 @@ var TimelinePanel = React.createClass({

// the messagePanel doesn't know where the read marker is.
// if we know the timestamp of the read marker, make a guess based on that.
var rmTs = TimelinePanel.roomReadMarkerTsMap[this.props.timelineSet.roomId];
const rmTs = TimelinePanel.roomReadMarkerTsMap[this.props.timelineSet.room.roomId];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh. Has this been broken for a while?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it! This is reached if MessagePanel has not got the related event paginated in.

It doesn't look like much is done with the result of TimelinePanel.getReadMarkerPosition() other than checking that it's in the view-port when updateReadMarker is called (which might explain why we were seeing it following when scrolling down) and specifying whether it's visible to MessagePanel.

if (rmTs && this.state.events.length > 0) {
if (rmTs < this.state.events[0].getTs()) {
return -1;
Expand Down Expand Up @@ -956,16 +993,12 @@ var TimelinePanel = React.createClass({
_setReadMarker: function(eventId, eventTs, inhibitSetState) {
var roomId = this.props.timelineSet.room.roomId;

if (TimelinePanel.roomReadMarkerMap[roomId] == eventId) {
// don't update the state (and cause a re-render) if there is
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment still looks valid?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair point!

// no change to the RM.
// don't update the state (and cause a re-render) if there is
// no change to the RM.
if (eventId === this.state.readMarkerEventId) {
return;
}

// ideally we'd sync these via the server, but for now just stash them
// in a map.
TimelinePanel.roomReadMarkerMap[roomId] = eventId;

// in order to later figure out if the read marker is
// above or below the visible timeline, we stash the timestamp.
TimelinePanel.roomReadMarkerTsMap[roomId] = eventTs;
Expand All @@ -974,6 +1007,7 @@ var TimelinePanel = React.createClass({
return;
}

// Do the local echo of the RM
// run the render cycle before calling the callback, so that
// getReadMarkerPosition() returns the right thing.
this.setState({
Expand Down Expand Up @@ -1021,7 +1055,6 @@ var TimelinePanel = React.createClass({
// events when viewing historical messages, we get stuck in a loop
// of paginating our way through the entire history of the room.
var stickyBottom = !this._timelineWindow.canPaginate(EventTimeline.FORWARDS);

return (
<MessagePanel ref="messagePanel"
hidden={ this.props.hidden }
Expand Down