Change the room list algo to eagerly delete and carefully insert #2701
Conversation
Previously it made some complicated assumptions about the contexts it was called in (it generally assumed it just had to shuffle rooms between tags and didn't really handle new rooms very well). The algorithm now eagerly tries to drop rooms from tags and carefully inserts them. The actual insertion logic is mostly untouched: the only part changed is how it handles failure to insert into tag. It shouldn't be possible for this algorithm to completely skip a room unless the tag is empty, so we special case that. There are several TODO comments to be addressed here. Namely, it doesn't handle manually ordered tags (favourites, custom, etc) and doesn't check if tags are even enabled. Changes in this area are waiting for #2686 to land to take advantage of monitoring the settings flag for tags.
| @@ -52,7 +52,6 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa | |||
| } else if (event.getType() === "im.vector.web.settings") { | |||
| // We can't really discern what changed, so trigger updates for everything | |||
| for (const settingName of Object.keys(event.getContent())) { | |||
| console.log(settingName); | |||
There was a problem hiding this comment.
this was left behind in a previous PR
|
Tests for this are reserved for element-hq/element-web#8922 |
There was a problem hiding this comment.
I don't think I have the mental energy to fully grasp the changes in the PR, but you've seem to have put quite some thought into this, and we have time to fix potential issues until this lands in production, so I am OK to merge this. Sorry for not being more thorough, but it's late in the day, and as discussed before, I find the code somewhat hard to comprehend.
| @@ -64,7 +64,7 @@ class RoomListStore extends Store { | |||
| * behave. | |||
| * @param {boolean} forceRegeneration True to force a change in the algorithm | |||
| */ | |||
| updateSortingAlgorithm(forceRegeneration=false) { | |||
There was a problem hiding this comment.
Just an suggestion, but I feel like this flag could be a future source of bugs and misunderstanding. As this method doesn't seem to be on any hotpath, how about always regenerating?
There was a problem hiding this comment.
It's not really on a hot path, but it is a bit of a waste of cycles to regenerate the list from scratch whenever a setting at a lower level changes. Fixing the setting monitors to not spam changes would also fix this, which is next up on my list.
|
Sorry this is so complex :( I really want to fix it to not be difficult - I hope to do this after the release. |
Fixes element-hq/element-web#8898 (in the most complicated way possible: a rewrite)
Note to the reviewer: It may be easier to understand this by making use of github's split diff view (dropdown next to 'Review Changes' on the Files tab). Also, feel free to merge this if it is satisfactory.
Previously it made some complicated assumptions about the contexts it was called in (it generally assumed it just had to shuffle rooms between tags and didn't really handle new rooms very well).
The algorithm now eagerly tries to drop rooms from tags and carefully inserts them. The actual insertion logic is mostly untouched: the only part changed is how it handles failure to insert into tag. It shouldn't be possible for this algorithm to completely skip a room unless the tag is empty, so we special case that.
There are several TODO comments to be addressed here. Namely, it doesn't handle manually ordered tags (favourites, custom, etc) and doesn't check if tags are even enabled. Changes in this area are waiting for #2686 to land to take advantage of monitoring the settings flag for tags.