This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
Fix various issues surrounding granular settings to date #1613
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Travis Ralston <travpc@gmail.com>
Otherwise `!null` ends up being "true", therefore forcing URL previews on for everyone. Fixes element-hq/element-web#5607 Signed-off-by: Travis Ralston <travpc@gmail.com>
This shouldn't currently be causing problems, but will in teh future. The bug can be exposed by having a setting where the level order is completely reversed, therefore causing LEVEL_ORDER[0] to actually be the most generic, not the most specific. Instead, we'll pull in the setting's level order and fallback to LEVEL_ORDER, therefore requesting the most specific value. Signed-off-by: Travis Ralston <travpc@gmail.com>
Fixes element-hq/element-web#5611 Signed-off-by: Travis Ralston <travpc@gmail.com>
Otherwise we end up lying and saying notifications are disabled, despite the push rules saying otherwise. Part 1 of the fix for: * element-hq/element-web#5603 * element-hq/element-web#5606 Signed-off-by: Travis Ralston <travpc@gmail.com>
Previously the push rule was ignored, leading to all kinds of interesting issues regarding notifications. This fixes those issues by giving the master push rule the authority it deserves for reasonable defaults. Part 2 of the fix for: * element-hq/element-web#5603 * element-hq/element-web#5606 Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
turt2live
force-pushed
the
travis/granular_bugs
branch
from
e37bf6b
to
d0a0a9c
Compare
turt2live
commented
Nov 16, 2017
| @@ -613,8 +613,7 @@ module.exports = React.createClass({ | |||
|
|
|||
| onLanguageChange: function(newLang) { | |||
| if(this.state.language !== newLang) { | |||
| // We intentionally promote this to the account level at this point | |||
| SettingsStore.setValue("language", null, SettingLevel.ACCOUNT, newLang); | |||
| SettingsStore.setValue("language", null, SettingLevel.DEVICE, newLang); | |||
There was a problem hiding this comment.
for the inevitable question: I have no idea why this got promoted to an account level setting. It's always been a local setting.
There was a problem hiding this comment.
hah, i love the idea of changing language on one device magically synchronising over all your accounts :D
There was a problem hiding this comment.
'love' in a "aaaaaargh no" kinda way
ara4n
reviewed
Nov 16, 2017
| @@ -891,7 +891,7 @@ module.exports = React.createClass({ | |||
| */ | |||
| _onSetTheme: function(theme) { | |||
| if (!theme) { | |||
| theme = this.props.config.default_theme || 'light'; | |||
| theme = SettingsStore.getValueAt(SettingLevel.DEFAULT, "theme"); | |||
ara4n
reviewed
Nov 16, 2017
| return false; | ||
| } | ||
|
|
||
| // Why enabled == false means "enabled" is beyond me. |
There was a problem hiding this comment.
Because the masterRule acts to disable push, apparently.
ara4n
reviewed
Nov 16, 2017
| if (!Notifier.isPossible()) return false; | ||
|
|
||
| if (calculatedValue === null) { | ||
| console.log(isMasterRuleEnabled()); |
|
lgtm! |
turt2live
commented
Nov 16, 2017
| @@ -40,7 +40,6 @@ export class NotificationsEnabledController extends SettingController { | |||
| if (!Notifier.isPossible()) return false; | |||
|
|
|||
| if (calculatedValue === null) { | |||
| console.log(isMasterRuleEnabled()); | |||
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partner PRs
Fixes
Description
All of the issues reported are edge cases to the granular settings system in one way or another. Each fix is described in the commit body.
This PR additionally includes a missed case where the default theme should be loaded through
SettingsStoreso that the override procedure is not defined in several places. This is the reason for the riot-web PR.