Lazy loading: feature toggle #2115
Conversation
| } | ||
|
|
||
| canChangeTo(level, roomId, newValue) { | ||
| return new Promise((resolve) => this._showReloadDialog(resolve)); |
There was a problem hiding this comment.
This kinda feels like an abuse of setting controllers: the warning should be hooked up elsewhere in my opinion. The setting controllers are intended to be used for things like desktop notifications where the platform may not support it at all, so the feature should not be enabled.
There's some information about their intent here: https://github.com/matrix-org/matrix-react-sdk/blob/master/docs/settings.md#setting-controllers
There was a problem hiding this comment.
Ok, I'll do it in the onChange handler for labs checks in UserSettings. As this won't stay a feature for too long hopefully, it's good not to make changes too deep 👍
src/settings/SettingsStore.js
Outdated
| @@ -260,7 +260,7 @@ export default class SettingsStore { | |||
| * @param {*} value The new value of the setting, may be null. | |||
| * @return {Promise} Resolves when the setting has been changed. | |||
| */ | |||
| static setValue(settingName, roomId, level, value) { | |||
| static async setValue(settingName, roomId, level, value) { | |||
There was a problem hiding this comment.
I'm very worried about making this async as I'm fairly confident that there are places in the project that expect this to be sync.
There was a problem hiding this comment.
But it already returned a promise before? The external API hasn't changed AFAIK.
There was a problem hiding this comment.
E.g. this is just about readability AFAIAC. The behaviour shouldn't have changed at all now canChangeTo is removed. I can still take it out if you prefer.
There was a problem hiding this comment.
Ah, I see. It should be the same, yes. Mixing up my mental iterations of this section
There was a problem hiding this comment.
I'm surprised we need to clear the e2e keys. Does https://github.com/matrix-org/matrix-react-sdk/blob/master/src/components/structures/UserSettings.js#L558 not do what you want?
|
@dbkr I thought that would also get rid of the crypto database, but it looks like that isn't the case, so yes, that would be way better! |
|
|
||
| export default class LazyLoadingController extends SettingController { | ||
| onChange(level, roomId, newValue) { | ||
| dis.dispatch({action: 'flush_storage_reload'}); | ||
| async onChange(level, roomId, newValue) { |
There was a problem hiding this comment.
copied the code from UserSettings here as I the method there is used in a different use case, to not have this break when the method changes.
|
@dbkr implemented your suggestion |
Depends on matrix-org/matrix-js-sdk#687 for detecting server support.
can't logout as we planned as that would have cleared the localstorage, clearing the setting that was just disabled. Putting it in account data also seemed hard, as that's not supported for features.
So the solution I took was to not logout, just stop the client, clear indexeddb and restart the app with the same credentials. A bit dangerous but seems to work well.
feature_lazyloadingneeds to be set to labs for the check to appear. Looks like this:Missing: