Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix a number of proliferation issues in the new room list #4828

Merged
merged 5 commits into from
Jun 30, 2020

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Jun 25, 2020

Fixes element-hq/element-web#14051
Fixes element-hq/element-web#14164
Fixes element-hq/element-web#14197
Fixes element-hq/element-web#14132
Fixes element-hq/element-web#14251
For element-hq/element-web#13635

This is reviewable commit-by-commit (recommended).

Cases fixed:

  • Upgrading your own room (sticky)
  • Upgrading a focused room (other person; sticky)
  • Upgrading an unfocused room (other person; non-sticky)
  • Creating a new room
  • Receiving an invite
  • Accepting an invite

@turt2live turt2live force-pushed the travis/room-list/proliferation branch from 9ad0299 to 3521c55 Compare June 26, 2020 02:41
We have to do a bit of a dance to return the sticky room to the list so we can remove it, if needed, and ensure that we generally swap the rooms out of the list.
When a new room is added there's a fairly good chance that the other events being dispatched will happen in the middle of (for example) the room list being re-sorted. This commit wraps the entire handleRoomUpdate() function for the underlying algorithms in a lock so that if we're unlucky enough to get an update while we're sorting (as the ImportanceAlgorithm splices out what it is sorting) we won't scream about invalid index errors.
We wouldn't have seen them before, so might as well treat them as new instead of tag changes.
This small check just ensures that we aren't about to blindly accept that the calling code knows what it is doing. There are some unknown cases where NewRoom gets fired for rooms we already know about, so in those cases we just change it to a PossibleTagChange which is what the caller likely intended. 

Many of the edge cases are unknown, though this can happen for an invite being accepted (for example). It's easier to handle it here instead of tracking down every single possibility and fixing it higher up.
`setKnownRooms` is called to regenerate the room list, and if we don't take the sticky room out of the equation we end up with the room being duplicated. So, to make this easy, we simply remove the sticky room and handle it after the fact.
@turt2live turt2live force-pushed the travis/room-list/proliferation branch from 3521c55 to 9de4251 Compare June 30, 2020 21:05
@turt2live turt2live marked this pull request as ready for review June 30, 2020 21:08
@turt2live turt2live requested a review from a team June 30, 2020 21:09
@t3chguy t3chguy requested review from t3chguy and removed request for a team June 30, 2020 21:59
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Looks sane. I definitely don't comprehend the intricacies without spending a lot too long reading the entire room list algorithm but looks like it does things which may improve the situation for dogfooding :D

@turt2live turt2live merged commit 2eaaf6a into develop Jun 30, 2020
@turt2live turt2live deleted the travis/room-list/proliferation branch June 30, 2020 22:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.