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

Lazy loading: Lazy load members while backpaginating #676

Merged
merged 15 commits into from
Aug 3, 2018

Conversation

bwindels
Copy link
Contributor

@bwindels bwindels commented Aug 1, 2018

Based on #677

  • pass LL filter to server (combining with timeline filters?)
  • add received state to correct timeline
  • add LL flag in client instead of just passing in filter? (done, but needs cleanup)
  • Add RoomState to EventContext
  • Fix avatars not always appearing in history
  • Fix MessagePanel::_getReadMarkerGhostTile generating double keys

@bwindels bwindels changed the base branch from master to bwindels/feature_lazyloading August 1, 2018 13:48
This works with rooms which haven't had their members
loaded yet.
we need more than just a filter, which is what is passed in now,
so have an explicit option. For now still take the filter but later on
this could be created inside MatrixClient
so we only need to add LL filter once
As RoomMember contains the event in a nested object (events.member),
a shallow copy was not enough to be immutable.

This solution won't copy OOB flags but that's not neccesary
for sentinels.
applying itself all the way till the next member event
when back paginating
@bwindels
Copy link
Contributor Author

bwindels commented Aug 2, 2018

MatrixClient::scrollback doesn't seem to be used anywhere (actually scrolling back triggers MatrixClient::paginateEventTimeline) apart from an example and specs. Leaving it in (and added support for LL state) but we could consider removing the scrollback method altogether.

As we need an option to turn lazy loading on (we can't just accept a filter,
as /messages has an incompatible filter), better only pass the option
and create the filter inside startClient
@bwindels bwindels requested a review from a team August 2, 2018 17:53
@bwindels bwindels changed the title Lazy load members while backpaginating Lazy loading: Lazy load members while backpaginating Aug 2, 2018
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

otherwise I think this looks good!

* of the chunk can be set with this method.
* @param {MatrixEvent[]} events state events to prepend
*/
RoomState.prototype.prependStateEvents = function(events) {
Copy link
Member

Choose a reason for hiding this comment

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

isn't this just setUnknownStateEvents? I'm not sure what is actually 'prepend'-y about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was that it inserts the state events as if they came before the state already present, hence prepend. But you're right, setUnknownStateEvents makes more sense.

*/
MatrixClient.prototype.startClient = function(opts) {
MatrixClient.prototype.startClient = async function(opts) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm - in terms of backwards compat, what will happen if a client doesn't wait on this promise and starts calling API methods immediately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. MatrixClient::_syncApi wouldn't be set yet, which could have some side-effects:

  • getSyncState returning null
  • startClient twice could leave you with two running SyncApis
  • stopClient wouldn't stop the sync
  • retryImmediately would crash

Hmm, I guess this could be a breaking change then, and hence if we want to do it carry a major version number change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I just keep the filter creation inside MatrixClientPeg then?

Copy link
Member

Choose a reason for hiding this comment

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

(out of band: let's do a version bump)

@bwindels bwindels merged commit adbf5fb into bwindels/feature_lazyloading Aug 3, 2018
@bwindels bwindels deleted the bwindels/backpaginate_ll branch August 3, 2018 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants