Support the old room sorting algorithm and SettingsStore watchers #2686
Conversation
This implements a dream of one day being able to listen for changes in a settings to react to them, regardless of which device actually changed the setting. The use case for this kind of thing is extremely limited, but when it is needed it should be more than powerful enough.
turt2live
force-pushed
the
travis/old-room-sorting
branch
from
8a6dc1d
to
b0cc69b
Compare
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.
| @@ -47,4 +47,12 @@ export default class ConfigSettingsHandler extends SettingsHandler { | |||
| isSupported() { | |||
| return true; // SdkConfig is always there | |||
| } | |||
|
|
|||
| watchSetting(settingName, roomId, cb) { | |||
There was a problem hiding this comment.
Add these no-ops to the common parent class (SettingsHandler)? (or better yet, see last comment, not have them at all)
There was a problem hiding this comment.
The parent class throws errors intentionally for when things aren't overridden. I would love for us to switch to TypeScript to make all this madness a bit more palatable.
| const roomId = room.roomId; | ||
|
|
||
| if (event.getType() === "org.matrix.room.preview_urls") { | ||
| let val = event.getContent()['disable']; |
There was a problem hiding this comment.
it would be nice if this mapping between account data and settings name could be shared between here, getValue and where possible setValue. Similarly in the other handlers.
There was a problem hiding this comment.
Each exception to the general rule is slightly different enough to make that not worthwhile I think. Room previews are an inverted-if-boolean-but-null-otherwise, colors are an object, and everything else is a field on the content of a specific event. Mapping this out in a generic way would result in a lot of code which ultimately doesn't do much but prevent typos.
|
|
||
| console.log(`Starting watcher for ${settingName}@${roomId || '<null room>'} at level ${level}`); | ||
| SettingsStore._watchers[watcherId][level] = localizedCallback; | ||
| handler.watchSetting(settingName, roomId, localizedCallback); |
There was a problem hiding this comment.
Hmm, so if a handler at a lower level emits an update, but a higher level handler would still override that value, the update still goes out, right? I wonder why the handlers each have their own WatchManager, as opposed to them having a reference (or use the global) to the SettingsStore to call notifyUpdate on... where you can then decide to emit an update bases on all the levels.
Not a huge issue, as most code would just cal getValue again and read the same old value, but on first sight that would also be less code, would be easier to prevent the above "issue", but might be missing something.
At least I would remove/change the newVal argument to the callback, because I suspect that as it stands it might report something different than what getValue would report.
There was a problem hiding this comment.
The idea is to maintain the high flexibility provided by the SettingsStore for the admittedly few cases which need it. There's not currently a use case for needing to know when a specific level has changed, however things like notifications (which currently use a controller) might want to make use of this granularity. I'm a bit on the fence for changing this to only emit when there's a perceivable change as in future it would result in excessive updates but might be inconsequential.
The other reason each handler has a dedicated WatchManager is to support the idea of watching a setting at a particular level. This is also a rare case, and we can probably just split it out later if we need to.
|
As per the standup: I'm going to take you up on your offer to have this be merged despite feedback. It unblocks a bugfix which needs to monitor for setting changes to be performant. I've opened element-hq/element-web#8936 to track making it better and ensured it will be the first thing I do after fixing the room list. |
Fixes element-hq/element-web#8892
Note: Reviewing commit-by-commit might be the most helpful here, given the bulk of the PR is SettingsStore stuff.
This implements a dream of one day being able to listen for changes in a settings to react to them, regardless of which device actually changed the setting. The use case for this kind of thing is extremely limited, but when it is needed it should be more than powerful enough.
Below is a GIF of it in action - pay close attention to the room list on the left. The settings have intentionally been manipulated so this is more visible.
