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

De-dup code: use the initialiseState function #611

Merged
merged 9 commits into from Feb 20, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 24 additions & 0 deletions spec/integ/matrix-client-syncing.spec.js
Expand Up @@ -437,6 +437,30 @@ describe("MatrixClient syncing", function() {
});
});

it("should correctly interpret state in incremental sync.", function() {
httpBackend.when("GET", "/sync").respond(200, syncData);
httpBackend.when("GET", "/sync").respond(200, nextSyncData);

client.startClient();
return Promise.all([
httpBackend.flushAllExpected(),
awaitSyncEvent(2),
]).then(function() {
const room = client.getRoom(roomOne);
const stateAtStart = room.getLiveTimeline().getState(
EventTimeline.BACKWARDS,
);
const startRoomNameEvent = stateAtStart.getStateEvents('m.room.name', '');
expect(startRoomNameEvent.getContent().name).toEqual('Old room name');

const stateAtEnd = room.getLiveTimeline().getState(
EventTimeline.FORWARDS,
);
const endRoomNameEvent = stateAtEnd.getStateEvents('m.room.name', '');
expect(endRoomNameEvent.getContent().name).toEqual('A new room name');
});
});

xit("should update power levels for users in a room", function() {

});
Expand Down
40 changes: 15 additions & 25 deletions src/sync.js
Expand Up @@ -1294,41 +1294,31 @@ 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;

// 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 liveTimeline = room.getLiveTimeline();
const timelineWasEmpty = liveTimeline.getEvents().length == 0;
if (timelineWasEmpty) {
liveTimeline.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
// 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