Don't re-sort the room-list based on profile/status changes

[SimonBrandner]

Fixes element-hq/element-meta#862
Type: defect

---

Here's what your changelog entry will look like:

🐛 Bug Fixes

• Don't re-sort the room-list based on profile/status changes (#6595). Fixes Don't re-sort the room list (or mark rooms as unread) based on nick or profile changes element-hq/element-meta#862.

Preview: https://6125eb4783c37e7091182453--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

[github-actions]

Here's what your changelog entry will look like:

🐛 Bug Fixes

• Don't re-sort the room-list based on profile/status changes (#6595). Fixes Don't re-sort the room list (or mark rooms as unread) based on nick or profile changes element-hq/element-meta#862. Contributed by SimonBrandner.

[turt2live]

For a more overarching review: this should be applied before we get to the ordering algorithm. Once we're at the ordering algorithm we're doing the sort and updating which means wasted CPU cycles spent calculating a zero change.

Instead, we can no-op a whole lot earlier during the onDispatchAsync function in the RoomListStore - here we'd be checking event type and changes on the RoomUpdateCause.Timeline calls to Algorithm#handleRoomUpdate() (worst class name ever). This would mean that we don't even send the event down for processing or re-sorting, saving the CPU cycles and still fixing the principal complaint.

[SimonBrandner]

For a more overarching review: this should be applied before we get to the ordering algorithm. Once we're at the ordering algorithm we're doing the sort and updating which means wasted CPU cycles spent calculating a zero change.

Instead, we can no-op a whole lot earlier during the onDispatchAsync function in the RoomListStore - here we'd be checking event type and changes on the RoomUpdateCause.Timeline calls to Algorithm#handleRoomUpdate() (worst class name ever). This would mean that we don't even send the event down for processing or re-sorting, saving the CPU cycles and still fixing the principal complaint.

Sorry if I am missing something. This doesn't seem to work since sometimes something else triggers the re-ordering, so the room-list still re-orders. I guess we could use that function both in the RecentAlgorithm and in the RoomListStore. Is that what you had in mind or am I doing something wrong?

---

X-Blocked as the PR doesn't work in the current state

[turt2live]

Clearing review request while this is blocked.

[SimonBrandner]

@turt2live, sorry, I think I might have misused the label again 😅 This is blocked on figuring out the correct way to do this and I am not sure how to proceed with that as I've explained in #6595 (comment). Does what I said in #6595 (comment) make sense?

[turt2live]

Ah, for this it's not "blocked" but rather just part of iterative review :)

Blocked means that unless something is done, there is zero way to make progress on it.

[turt2live]

Sorry if I am missing something. This doesn't seem to work since sometimes something else triggers the re-ordering, so the room-list still re-orders. I guess we could use that function both in the RecentAlgorithm and in the RoomListStore. Is that what you had in mind or am I doing something wrong?

The room list is complicated to say the least, so no worries there. It'd be good to track down what causes the random reorderings, but that's also a bit out of scope. For this, I'm mostly worried about redundant renders caused by the reordering (as the room list store assumes it will be pushing an update after reordering happens).

From a brief look, it seems as though we ignore the handleRoomUpdate return value from the OrderingAlgorithm during Algorithm#handleRoomUpdate(), though that may be because we're recalculating filtered room sets and sticky rooms regardless. It's not immediately clear to me why we do that, but it'd be the place where we'd no-op an update that caused no change to the tag set.

So... with all that being said, this is probably fine. element-hq/element-web#14457 in particular is likely to fix some of the issues with performance, but for now let's see how bad it actually gets. Can we go back to the other implementation you had, with the added comment of naming the function in a positive way? (eg: ignoreSelfEvent -> shouldCauseReorder)

[SimonBrandner]

I've reverted back to the old solution with the function renamed and avatar changes handling added

[turt2live]

thanks :)

Let's see what breaks :D