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.

Sign up for GitHub

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

De-dup code: use the initialiseState function #611

Merged
merged 9 commits into from
Feb 20, 2018
Merged

De-dup code: use the initialiseState function #611

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
275eb8d
De-dup code: use the initialiseState function
dbkr Feb 16, 2018
8c02e7b
comment
dbkr Feb 16, 2018
b11bacc
Process state events before timeline events
dbkr Feb 16, 2018
21a3245
Comments
dbkr Feb 19, 2018
c77277b
rename variable
dbkr Feb 19, 2018
9a08194
Factor out calls to getLiveTimeline
dbkr Feb 19, 2018
e810ee7
Add test for fixed state misbehaviour
dbkr Feb 20, 2018
8aa68b3
lint
dbkr Feb 20, 2018
1ddf7fb
Add XXX comment
dbkr Feb 20, 2018
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
42 changes: 18 additions & 24 deletions src/sync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1294,41 +1294,35 @@ SyncApi.prototype._resolveInvites = function(room) {
*/
SyncApi.prototype._processRoomEvents = function(room, stateEventList,
timelineEventList) {
timelineEventList = timelineEventList || [];
const client = this.client;
// "old" and "current" state are the same initially; they
// start diverging if the user paginates.
// We must deep copy otherwise membership changes in old state
// will leak through to current state!
const oldStateEvents = utils.map(
utils.deepCopy(
stateEventList.map(function(mxEvent) {
return mxEvent.event;
}),
), client.getEventMapper(),
);
const stateEvents = stateEventList;
// We'll only get state events in stateEventList if this is a limited
Copy link
Member

Choose a reason for hiding this comment

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

hrm... this is an assumption based on the way synapse behaves. Indeed it seems to be in conflict with the fact that you then do something with the state events if the timeline is not empty.

Could you rephrase this comment please.

Copy link
Member Author

Choose a reason for hiding this comment

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

tbh I think deleting it may make more sense: I think this was left in from when the code was relying on this behaviour in synapse and I failed to remove it.

// sync, in which case this should be a fresh timeline, and this is us
// initialising it.

// set the state of the room to as it was before the timeline executes
//
// XXX: what if we've already seen (some of) the events in the timeline,
// and they modify some of the state set in stateEvents? In that case we'll
// end up with the state from stateEvents, instead of the more recent state
// from the timeline.
room.oldState.setStateEvents(oldStateEvents);
room.currentState.setStateEvents(stateEvents);

// If there are no events in the timeline yet, initialise it with
// the given state events
const timelinewasEmpty = room.getLiveTimeline().getEvents().length == 0;
Copy link
Member

Choose a reason for hiding this comment

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

timelineWasEmpty ?

Copy link
Member Author

Choose a reason for hiding this comment

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

mm, yeah - fixed

Copy link
Member

Choose a reason for hiding this comment

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

can we factor out the myriad calls to room.getLiveTimeline() into a local var?

if (timelinewasEmpty) {
room.getLiveTimeline().initialiseState(stateEventList);
}

this._resolveInvites(room);
Copy link
Member

Choose a reason for hiding this comment

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

why does this (and recalculate) happen after adding the state events for an empty timeline, but before it for an existing timeline?

Copy link
Member Author

Choose a reason for hiding this comment

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

My reasoning for this was the the state events ought to be treated the same as if they'd arrived in the timeline section. That said, I opted not to chase that particular rabbit down the hole to figure out what actually happens on a room name change, etc.

Copy link
Member

Choose a reason for hiding this comment

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

well, if that's true, why isn't it true for when the timeline is empty?

"I didn't go down the rabbithole" is valid but an XXX comment saying so would be good for future reference

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, and now looking at it, it looks a bit broken. Have added an XXX to explain how I think it might be broken.


// recalculate the room name at this point as adding events to the timeline
// may make notifications appear which should have the right name.
room.recalculate(this.client.credentials.userId);

// execute the timeline events, this will begin to diverge the current state
// If the timeline wasn't empty, we process the state events here: they're
// defined as updates to the state before the start of the timeline, so this
// starts to roll the state forward.
if (!timelinewasEmpty) {
room.addLiveEvents(stateEventList || []);
}
// execute the timeline events, this will continue to diverge the current state
Copy link
Member

Choose a reason for hiding this comment

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

s/,/./

Copy link
Member Author

Choose a reason for hiding this comment

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

done

// if the timeline has any state events in it.
// This also needs to be done before running push rules on the events as they need
// to be decorated with sender etc.
room.addLiveEvents(timelineEventList);
room.addLiveEvents(timelineEventList || []);
};

/**
Expand Down