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

Timeline should follow /sync result order, not origin_server_ts #22

Closed
KitsuneRal opened this issue Sep 1, 2016 · 6 comments
Closed
Assignees
Labels
bug/fix The library doesn't work as expected

Comments

@KitsuneRal
Copy link
Member

Upon yesterday's discussion in #quaternion and #matrix-dev, the current way of forming the timeline in Quaternion turned out to be utterly wrong: origin_server_ts should not be used to order a timeline (counter-example: one homeserver with skewed clock, and you see mess in the timeline, no matter which homeserver you're looking at it through).
As there's no "correct" full order (DAG is partially ordered by definition), the best option is to follow the order in which /sync and /messages provide results. A corollary of this is that we cannot notify clients about each newMessage separately; and moreover, notifying about an arrived block of messages should include information about ordering of these message inside the timeline. The interface and rationale for that is actually a big question in itself.

@KitsuneRal KitsuneRal added the bug/fix The library doesn't work as expected label Sep 1, 2016
@KitsuneRal KitsuneRal self-assigned this Sep 1, 2016
@KitsuneRal
Copy link
Member Author

KitsuneRal commented Sep 1, 2016

For the record: I'm also experimenting with std::deque as a container for the timeline, rather than QVector. The considerations follow:

  • Qt containers don't have range insertion (but yes of course I can write a range insertion algorithm myself).
  • While std::deque is not very compact on smaller numbers of elements, it looks very strong on bigger numbers (100-1000 elemens) in performance tests (both by memory and CPU). Moreover, it's designed to store arbitrarily long sequences of elements (because it uses pages rather than contiguous storage as vectors) and is double-ended. Of Qt containers, only QList is double-ended, and it's increasingly bad (especially with respect to memory) at bigger numbers of elements - see, e.g. the results for CPU and for memory in [the article that I already referred to before](https://marcmutz.wordpress.com/effective-qt/containers/ - and those are only for appending).

@KitsuneRal
Copy link
Member Author

The current way of storing messages has one more flaw: in fact, Room::messageEvents is not really used anywhere because Quaternion (and, by copy-paste, Tensor) use own model and fill it on their own. This is, of course, inefficient; libqmatrixclient has to make messageEvents a usable timeline data source even if clients want to extend element types with other attributes. I'm still unsure how exactly to achieve that; one probable option is a template wrapper around Room, with an element type as its parameter.

@KitsuneRal
Copy link
Member Author

Upon further thinking on this: rather than keep the raw events "timeline" (which has no chance of usage in the UI) in Room, try to separate the timeline itself in a separate template class, encapsulating all the logic of adding and accessing messages into it. Parts of QuaternionRoom will transfer there, and Message might then entirely land inside the library, too.

@KitsuneRal
Copy link
Member Author

KitsuneRal commented Sep 12, 2016

Erm, using a template class doesn't quite work for Tensor that uses a bare Room class without deriving from it. Unless we officially declare Room abstract, it should keep the timeline inside of it. So there are two use-cases to support:

  • Room should be usable on its own if a client is happy with the available attributes and logic around timeline events;
  • Room should allow storing arbitrary attributes next to each timeline event, according to some model. It can be done by either of:
    • an Event proxy (that formally inherits from Event itself but actually has another Event* inside of it - this will require making all Event accessors virtual but it's not so much of a problem).
    • a template storage of message events outside of the current Room class and, respectively, management of another container, with timelines, in the client's model, next to a container of Rooms.

The option with an Event proxy looks much more flexible and less burdening both for a thinner client (such as Tensor) and a thicker client (such as Quaternion).

@KitsuneRal
Copy link
Member Author

An Event proxy is hard to create unless we, again, have a second storage for those proxy, which renders the whole idea irrelevant. Hence, a template storage.

@KitsuneRal
Copy link
Member Author

The original problem is closed by #30; the template storage for a timeline is a separate and less urgent topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/fix The library doesn't work as expected
Projects
None yet
Development

No branches or pull requests

1 participant