Conversation
Lama-Thematique
force-pushed
the
rework
branch
from
fb02fcc to
62fc2d0
Compare
Lama-Thematique
force-pushed
the
rework
branch
from
62fc2d0 to
e1f73db
Compare
| Stream<int> get onChanged; | ||
| Stream<int> get onRemoved; | ||
| Stream<Photo> get onAdded; | ||
| Stream<Photo> get onChanged; |
There was a problem hiding this comment.
A potential issue with not being able to get the index of where the change happened, means the UI always has to rebuild the entire list, and can't just update a single widget. It would make it harder to use things like AnimatedList
There was a problem hiding this comment.
Rebuilding an entire list is the same time complexity as removing a list element as an index, updating a list using indexes is extremely slow already. At worst (animated list) it's the same complexity as before, at best it's one order faster.
And with this change there is many things that can be made faster by reworking ligthly the components.
There was a problem hiding this comment.
I also would recommend talking about the notifying list change in here : #647
They are the same commits.
It make it easy to only include that change, as it is more of a bugfix on synchronization that "code quality" change.
There was a problem hiding this comment.
I'm not sure I agree that making ClientManager a singleton is necessarily beneficial. with it being a top level variable, it's already globally scoped but has the addition of being nullable. while in most cases its safe to assume the ClientManager has been initialized, in the instances where it isn't being able to use the ? or ! operators is much nicer than having to check something like ClientManager.isInitialized and I would prefer to hit a null operator error, as opposed to having it silently fail when I've assumed ClientManager is initialized, but it hasn't actually been.
There are conditions where ClientManager should not be null and uninitialized, for example updating notifications in the background
There was a problem hiding this comment.
There are conditions where ClientManager should not be null and uninitialized, for example updating notifications in the background
Oups, i expected it was always initialized, my bad on this one, tho i still think it's cleaner as a static element maybe have it as static nullable then.
|
|
||
| final NotifyingList<Peer> _peers = NotifyingList.empty(growable: true); | ||
| late final NotifyingMap<String, Space> _spaces = NotifyingSubMap( | ||
| clientManager.motifyingMapSpace, (e) => e?.clientId == _id); |
There was a problem hiding this comment.
to me it seems weird to have the client depend on clientManager to access it's own spaces and rooms. personally I think it makes more sense for each client to be its own source of truth, and the clientManager just combines them together.
There was a problem hiding this comment.
In a way I get what you mean here.
I think this solution can still be advantageous as there is a single real parent hashmap it remove all possibility for sync issue between your data structures.
There was a problem hiding this comment.
the other way to remove duplication if you prefer it is to have the client manager search into all clients without storing by itself this full list/map of rooms.
| /// The Room object should only be used for rooms which the user is a member of. | ||
| /// Rooms which the user has not joined should be represented with a RoomPreview | ||
| abstract class Room { | ||
| String get identifier; |
There was a problem hiding this comment.
changing identifier to roomId is unnecessary
There was a problem hiding this comment.
I felt it's better to differential between localid, clientid, and especially localid so that you always know which id we are talking about even when using generic type.
I base myself on database design where we prefer to name and always reference comparable ids by names to not loose track
|
|
||
| enum PushRule { notify, mentionsOnly, dontNotify } | ||
|
|
||
| abstract class BaseRoom { |
There was a problem hiding this comment.
While I understand the rationale behind wanting spaces, rooms and previews to extend from the same base class as in Matrix they are all the same, the API for interacting with Commet's clients aims to be agnostic to the underlying protocol. This is future proofing in case we want to support protocols other than Matrix in the future, where these concepts may not be able to be composed like this.
So while for Matrix specifically, it does make some parts of the code more verbose, I think its worth having the distinction between these different concepts
There was a problem hiding this comment.
I feel like it's a very small assumption to say that we have a way to identify a discussion in any protocol we might have (even in protocols like SMS the roomid would likely be the phone number you are sending the message to).
And we will always need a way identify the client we use to send the message.
I think this pair (roomId, ClientId) => LocalId would always be buildable.
|
I added my reasoning behind all changes that you commented on, if you still want some of them reverted say so, I will update the PR based on that. |
2 participants
I think this is slightly better code in term of backend without changing functionality, though some improvement are still possibles.
I recommend going through the patches in commit order as all of them at once might not be readable.
The mains changes are around:
The app seems to run without issues with thoses changes.
Rooms now all inherit Baseroom for Spaces, normal Rooms, and PreviewRooms to better handle IDs.
ClientManager and MatrixClient now use the new maps for O(1) removal/addition.
This code includes the changes of #647 the fixes the NotifyingList.
NotifyingMap and NotifyingSet are pretty obvious, with the note that I consider a replacement in NotifyingMap as a removal followed by a addition.
Notifying Set is currently unused but could be used later, for example for optimizing AlertManager. (remove on list is O(n), O(1) for sets).
NotifyingSubMap allow have a map that specialize any other NotifyingMap without any copy, only aver showing element that repsonds to an condition. all operations on NotifyingSubMap is applied to the underlying NotifyingMap. This allow for zero copies of all Rooms in ClientManager and MatrixClient.