Skip to content

Fix some UI sync issues due to NotifyingList bug#647

Open
Lama-Thematique wants to merge 2 commits intocommetchat:mainfrom
Lama-Thematique:notifiying-list-fix
Open

Fix some UI sync issues due to NotifyingList bug#647
Lama-Thematique wants to merge 2 commits intocommetchat:mainfrom
Lama-Thematique:notifiying-list-fix

Conversation

@Lama-Thematique
Copy link
Contributor

Problem

When creating a NotifyingList this code gets run:

    _onAdd = StreamController.broadcast(sync: sync);
    _onRemove = StreamController.broadcast(sync: sync);
    _onListUpdated = StreamController.broadcast(sync: sync);
    _onItemUpdated = StreamController.broadcast(sync: sync);

In all instance of creating a NotifyingList the params sync is always true.
We use those Streams to send the index of the deleted/add/modified element.

Here is the description of sync=true for StreamController in the docs:

If [sync] is true, events may be fired directly by the stream's subscriptions during an [add], [addError] or [close] call. The returned stream controller is a [SynchronousStreamController], and must be used with the care and attention necessary to not break the [Stream] contract. See [Completer.sync] for some explanations on when a synchronous dispatching can be used. If in doubt, keep the controller non-sync.

This cause issue when removing something in from the list:
Because either:

  • Remove first, send the index in the stream later -> Problem : We can't send the index of the element as it's not in the list anymore
  • We send the index in the stream first, remove later (what commet does) -> With sync=true the subscribers callback get an unmodified list (the item is still not removed), if we update an UI state in the callback (without async call) the UI does not update.

How do we fix this:

We don't send the index but directly the element being modified, as it turns out we never specifically need the index, we always only use the index to lookup the element.

This is what this Pull Request implements.

In practice when i leave a space I now see the update, whereas before this patch we don't have any UI update.

PS

Note that if you want I can try to rework a large portion of the matrix backend (client, clientmanager, rooms, etc) to try to make cleaner, easier to use and lower time complexity (using Sets and Hashmap instead of lists).
Please inform me if you are interested or not so that I don't loose a buch of time on this.
Note It might take a quite some time to rework all that

@Lama-Thematique
Copy link
Contributor Author

I also presume that sync=true was set to not have invalid indexes so this PR might make it safe to put it back to false (untested).

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.

1 participant

Comments