Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide pre-join UTDs #3881

Merged
merged 16 commits into from
Jan 28, 2020
88 changes: 84 additions & 4 deletions src/components/structures/TimelinePanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
Copyright 2016 OpenMarket Ltd
Copyright 2017 Vector Creations Ltd
Copyright 2019 New Vector Ltd
Copyright 2019 The Matrix.org Foundation C.I.C.
Copyright 2019-2020 The Matrix.org Foundation C.I.C.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -142,6 +142,9 @@ const TimelinePanel = createReactClass({
liveEvents: [],
timelineLoading: true, // track whether our room timeline is loading

// the index of the first event that is to be shown
firstVisibleEventIndex: 0,

// canBackPaginate == false may mean:
//
// * we haven't (successfully) loaded the timeline yet, or:
Expand Down Expand Up @@ -330,10 +333,12 @@ const TimelinePanel = createReactClass({
// We can now paginate in the unpaginated direction
const canPaginateKey = (backwards) ? 'canBackPaginate' : 'canForwardPaginate';
const { events, liveEvents } = this._getEvents();
const firstVisibleEventIndex = this._checkForPreJoinUISI(liveEvents);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It makes sense that we'd need to run this every time we get events, so I think it would be more natural to call _checkForPreJoinUISI inside _getEvents, and then firstVisibleEventIndex would be another key returned by _getEvents.

this.setState({
[canPaginateKey]: true,
events,
liveEvents,
firstVisibleEventIndex,
});
}
},
Expand Down Expand Up @@ -366,11 +371,13 @@ const TimelinePanel = createReactClass({
debuglog("TimelinePanel: paginate complete backwards:"+backwards+"; success:"+r);

const { events, liveEvents } = this._getEvents();
const firstVisibleEventIndex = this._checkForPreJoinUISI(liveEvents);
const newState = {
[paginatingKey]: false,
[canPaginateKey]: r,
events,
liveEvents,
firstVisibleEventIndex,
};

// moving the window in this direction may mean that we can now
Expand All @@ -390,7 +397,7 @@ const TimelinePanel = createReactClass({
// itself into the right place
return new Promise((resolve) => {
this.setState(newState, () => {
resolve(r);
resolve(r && (!backwards || firstVisibleEventIndex === 0));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a comment to explain this.

});
});
});
Expand Down Expand Up @@ -466,10 +473,12 @@ const TimelinePanel = createReactClass({

const { events, liveEvents } = this._getEvents();
const lastLiveEvent = liveEvents[liveEvents.length - 1];
const firstVisibleEventIndex = this._checkForPreJoinUISI(liveEvents);

const updatedState = {
events,
liveEvents,
firstVisibleEventIndex,
};

let callRMUpdated;
Expand Down Expand Up @@ -1097,7 +1106,9 @@ const TimelinePanel = createReactClass({
// the results if so.
if (this.unmounted) return;

this.setState(this._getEvents());
const newState = this._getEvents();
newState.firstVisibleEventIndex = this._checkForPreJoinUISI(newState.liveEvents);
this.setState(newState);
},

// get the list of events from the timeline window and the pending event list
Expand All @@ -1119,6 +1130,72 @@ const TimelinePanel = createReactClass({
};
},

/**
* Check for undecryptable messages that were sent while the user was not in
* the room.
*
* @param {Array<MatrixEvent>} events The timeline events to check
*
* @return {Number} The index within `events` of the event after the most recent
* undecryptable event that was sent while the user was not in the room. If no
* such events were found, then it returns 0.
*/
_checkForPreJoinUISI: function(events) {
const room = this.props.timelineSet.room;

if (events.length === 0 || !room ||
!MatrixClientPeg.get().isRoomEncrypted(room.roomId)) {
return 0;
}

const userId = MatrixClientPeg.get().credentials.userId;

let i;
let userMembership = "leave";
// find the last event that is in a timeline (events may not be in a
// timeline if they have not been sent yet) and get the user's membership
// at that point.
for (i = events.length - 1; i >= 0; i--) {
const timeline = room.getTimelineForEvent(events[i].getId());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I was hoping that moving up to TimelinePanel would also allow removing the outer loop and timeline check by only looking at liveEvents in state (instead of plain events). Is it possible to make that simplification, or does it break down somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, we don't need to use events. I've changed it to use liveEvents instead of events. (liveEvents vs events confused me.) I'm not sure if the outer loop is needed, but if it isn't, then it will exit after the first iteration of the loop.

Copy link
Collaborator

Choose a reason for hiding this comment

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

liveEvents is the set we know are already in a timeline:

const events = this._timelineWindow.getEvents();
// Hold onto the live events separately. The read receipt and read marker
// should use this list, so that they don't advance into pending events.
const liveEvents = [...events];

events is liveEvents plus the pending / unsent events as well. (Maybe not the best naming, open to suggestions...!) 😅

Given that then, let's remove the outer loop and update the comment since we know the last event of liveEvents is always the one we'll pick. (This is a somewhat complex function, so I'd like to simplify it where possible like this.)

if (timeline) {
const userMembershipEvent =
timeline.getState(EventTimeline.FORWARDS).getMember(userId);
userMembership = userMembershipEvent ? userMembershipEvent.membership : "leave";
const timelineEvents = timeline.getEvents();
for (let j = timelineEvents.length - 1; j >= 0; j--) {
const event = timelineEvents[j];
if (event.getId() === events[i].getId()) {
break;
} else if (event.getStateKey() === userId
&& event.getType() === "m.room.member") {
const prevContent = event.getPrevContent();
userMembership = prevContent.membership || "leave";
}
}
break;
}
}

// now go through the rest of the events and find the first undecryptable
// one that was sent when the user wasn't in the room
for (; i >= 0; i--) {
const event = events[i];
if (event.getStateKey() === userId
&& event.getType() === "m.room.member") {
const prevContent = event.getPrevContent();
userMembership = prevContent.membership || "leave";
} else if (userMembership === "leave" &&
(event.isDecryptionFailure() || event.isBeingDecrypted())) {
Copy link
Member Author

Choose a reason for hiding this comment

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

One result of moving this to TimelinePanel is that I had to make it also hide things that are in the process of being decrypted (which is probably a wrong thing to do), rather than just things that failed to decrypt, presumably because it hasn't had enough time to finish trying to decrypt. It would be nice if there was a better way to do this, but I can't see anything better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, not sure I follow why that's only an issue up here in TimelinePanel, I would have assumed it would apply in either component...

What is the user experience with and without including events being decrypted here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely sure why it only happens when it's in TimelinePanel and not in MessagePanel. My best guess is that it's a timing issue -- by the time MessagePanel gets called, the message decryption has completed, whereas in TimelinePanel, it's still trying to decrypt.

If I don't include isBeingDecrypted, then it will just show all the events, without dropping the pre-join UISIs.

// reached an undecryptable message when the user wasn't in
// the room -- don't try to load any more
// Note: for now, we assume that events that are being decrypted are
// not decryptable
return i + 1;
}
}
return 0;
},

_indexForEventId: function(evId) {
for (let i = 0; i < this.state.events.length; ++i) {
if (evId == this.state.events[i].getId()) {
Expand Down Expand Up @@ -1309,6 +1386,9 @@ const TimelinePanel = createReactClass({
this.state.forwardPaginating ||
['PREPARED', 'CATCHUP'].includes(this.state.clientSyncState)
);
const events = this.state.firstVisibleEventIndex
? this.state.events.slice(this.state.firstVisibleEventIndex)
: this.state.events;
return (
<MessagePanel
ref={this._messagePanel}
Expand All @@ -1317,7 +1397,7 @@ const TimelinePanel = createReactClass({
hidden={this.props.hidden}
backPaginating={this.state.backPaginating}
forwardPaginating={forwardPaginating}
events={this.state.events}
events={events}
highlightedEventId={this.props.highlightedEventId}
readMarkerEventId={this.state.readMarkerEventId}
readMarkerVisible={this.state.readMarkerVisible}
Expand Down