From 022e40a12751728347daef46829990c8f3573c56 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 15 Nov 2017 19:04:49 -0700 Subject: [PATCH 1/8] Use SettingsStore for default theme Signed-off-by: Travis Ralston --- src/components/structures/MatrixChat.js | 2 +- src/settings/SettingsStore.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index 37005b0d69..05ed9c95ed 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -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"); } // look for the stylesheet elements. diff --git a/src/settings/SettingsStore.js b/src/settings/SettingsStore.js index b6343c4a96..dde1d3ca15 100644 --- a/src/settings/SettingsStore.js +++ b/src/settings/SettingsStore.js @@ -181,8 +181,8 @@ export default class SettingsStore { /** * Gets a setting's value at a particular level, ignoring all levels that are more specific. - * @param {"device"|"room-device"|"room-account"|"account"|"room"} level The level to - * look at. + * @param {"device"|"room-device"|"room-account"|"account"|"room"|"config"|"default"} level The + * level to look at. * @param {string} settingName The name of the setting to read. * @param {String} roomId The room ID to read the setting value in, may be null. * @param {boolean} explicit If true, this method will not consider other levels, just the one From cf8ff6aed3ea690731f0f05f45357948524a9db0 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 15 Nov 2017 19:22:30 -0700 Subject: [PATCH 2/8] Validate that URL previews are explicitly enabled/disabled Otherwise `!null` ends up being "true", therefore forcing URL previews on for everyone. Fixes https://github.com/vector-im/riot-web/issues/5607 Signed-off-by: Travis Ralston --- src/settings/handlers/AccountSettingsHandler.js | 3 +++ src/settings/handlers/RoomAccountSettingsHandler.js | 3 +++ src/settings/handlers/RoomSettingsHandler.js | 3 +++ 3 files changed, 9 insertions(+) diff --git a/src/settings/handlers/AccountSettingsHandler.js b/src/settings/handlers/AccountSettingsHandler.js index fe0ad2a500..e50358a728 100644 --- a/src/settings/handlers/AccountSettingsHandler.js +++ b/src/settings/handlers/AccountSettingsHandler.js @@ -26,6 +26,9 @@ export default class AccountSettingHandler extends SettingsHandler { // Special case URL previews if (settingName === "urlPreviewsEnabled") { const content = this._getSettings("org.matrix.preview_urls"); + + // Check to make sure that we actually got a boolean + if (typeof(content['disable']) !== "boolean") return null; return !content['disable']; } diff --git a/src/settings/handlers/RoomAccountSettingsHandler.js b/src/settings/handlers/RoomAccountSettingsHandler.js index 503d5de6c4..e946581807 100644 --- a/src/settings/handlers/RoomAccountSettingsHandler.js +++ b/src/settings/handlers/RoomAccountSettingsHandler.js @@ -25,6 +25,9 @@ export default class RoomAccountSettingsHandler extends SettingsHandler { // Special case URL previews if (settingName === "urlPreviewsEnabled") { const content = this._getSettings(roomId, "org.matrix.room.preview_urls"); + + // Check to make sure that we actually got a boolean + if (typeof(content['disable']) !== "boolean") return null; return !content['disable']; } diff --git a/src/settings/handlers/RoomSettingsHandler.js b/src/settings/handlers/RoomSettingsHandler.js index 3aee0dd6eb..cb3e836c7f 100644 --- a/src/settings/handlers/RoomSettingsHandler.js +++ b/src/settings/handlers/RoomSettingsHandler.js @@ -25,6 +25,9 @@ export default class RoomSettingsHandler extends SettingsHandler { // Special case URL previews if (settingName === "urlPreviewsEnabled") { const content = this._getSettings(roomId, "org.matrix.room.preview_urls"); + + // Check to make sure that we actually got a boolean + if (typeof(content['disable']) !== "boolean") return null; return !content['disable']; } From f141ee1944893b1a8bbc23d6d53fe583fa7dc60a Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 15 Nov 2017 19:24:32 -0700 Subject: [PATCH 3/8] Use the correct level order when getting arbitrary settings 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 --- src/settings/SettingsStore.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/settings/SettingsStore.js b/src/settings/SettingsStore.js index dde1d3ca15..d93a48005d 100644 --- a/src/settings/SettingsStore.js +++ b/src/settings/SettingsStore.js @@ -176,7 +176,15 @@ export default class SettingsStore { * @return {*} The value, or null if not found */ static getValue(settingName, roomId = null, excludeDefault = false) { - return SettingsStore.getValueAt(LEVEL_ORDER[0], settingName, roomId, false, excludeDefault); + // Verify that the setting is actually a setting + if (!SETTINGS[settingName]) { + throw new Error("Setting '" + settingName + "' does not appear to be a setting."); + } + + const setting = SETTINGS[settingName]; + const levelOrder = (setting.supportedLevelsAreOrdered ? setting.supportedLevels : LEVEL_ORDER); + + return SettingsStore.getValueAt(levelOrder[0], settingName, roomId, false, excludeDefault); } /** From 10a1d9cb29e45bb90b5454ad67a472777ca53030 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 15 Nov 2017 21:16:12 -0700 Subject: [PATCH 4/8] Language is a local setting Fixes https://github.com/vector-im/riot-web/issues/5611 Signed-off-by: Travis Ralston --- src/components/structures/UserSettings.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/components/structures/UserSettings.js b/src/components/structures/UserSettings.js index 692dd4e01d..794c0d5d4d 100644 --- a/src/components/structures/UserSettings.js +++ b/src/components/structures/UserSettings.js @@ -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); this.setState({ language: newLang, }); From 5976fb2eed668f556ed98ee1a94a141933e9ea45 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 15 Nov 2017 22:08:13 -0700 Subject: [PATCH 5/8] Treat null/undefined notification settings as "not set" Otherwise we end up lying and saying notifications are disabled, despite the push rules saying otherwise. Part 1 of the fix for: * https://github.com/vector-im/riot-web/issues/5603 * https://github.com/vector-im/riot-web/issues/5606 Signed-off-by: Travis Ralston --- src/settings/handlers/DeviceSettingsHandler.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/settings/handlers/DeviceSettingsHandler.js b/src/settings/handlers/DeviceSettingsHandler.js index 7b5ec6a5dd..22f6140a80 100644 --- a/src/settings/handlers/DeviceSettingsHandler.js +++ b/src/settings/handlers/DeviceSettingsHandler.js @@ -40,11 +40,17 @@ export default class DeviceSettingsHandler extends SettingsHandler { // Special case notifications if (settingName === "notificationsEnabled") { - return localStorage.getItem("notifications_enabled") === "true"; + const value = localStorage.getItem("notifications_enabled"); + if (typeof(value) === "string") return value === "true"; + return null; // wrong type or otherwise not set } else if (settingName === "notificationBodyEnabled") { - return localStorage.getItem("notifications_body_enabled") === "true"; + const value = localStorage.getItem("notifications_body_enabled"); + if (typeof(value) === "string") return value === "true"; + return null; // wrong type or otherwise not set } else if (settingName === "audioNotificationsEnabled") { - return localStorage.getItem("audio_notifications_enabled") === "true"; + const value = localStorage.getItem("audio_notifications_enabled"); + if (typeof(value) === "string") return value === "true"; + return null; // wrong type or otherwise not set } return this._getSettings()[settingName]; From fb1f20b7d4d2c856b39e6370a6dad748b6c6e2e3 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 15 Nov 2017 22:09:40 -0700 Subject: [PATCH 6/8] Treat the master push rule as authoritative 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: * https://github.com/vector-im/riot-web/issues/5603 * https://github.com/vector-im/riot-web/issues/5606 Signed-off-by: Travis Ralston --- .../controllers/NotificationControllers.js | 37 +++++++++++++++++-- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/src/settings/controllers/NotificationControllers.js b/src/settings/controllers/NotificationControllers.js index 261e194d74..3831cd6891 100644 --- a/src/settings/controllers/NotificationControllers.js +++ b/src/settings/controllers/NotificationControllers.js @@ -15,12 +15,36 @@ limitations under the License. */ import SettingController from "./SettingController"; +import MatrixClientPeg from '../../MatrixClientPeg'; + +// XXX: This feels wrong. +import PushProcessor from "matrix-js-sdk/lib/pushprocessor"; + +function isMasterRuleEnabled() { + // Return the value of the master push rule as a default + const processor = new PushProcessor(MatrixClientPeg.get()); + const masterRule = processor.getPushRuleById(".m.rule.master"); + + if (!masterRule) { + console.warn("No master push rule! Notifications are disabled for this user."); + return false; + } + + // Why enabled == false means "enabled" is beyond me. + return !masterRule.enabled; +} export class NotificationsEnabledController extends SettingController { getValueOverride(level, roomId, calculatedValue) { const Notifier = require('../../Notifier'); // avoids cyclical references + if (!Notifier.isPossible()) return false; - return calculatedValue && Notifier.isPossible(); + if (calculatedValue === null) { + console.log(isMasterRuleEnabled()); + return isMasterRuleEnabled(); + } + + return calculatedValue; } onChange(level, roomId, newValue) { @@ -35,15 +59,22 @@ export class NotificationsEnabledController extends SettingController { export class NotificationBodyEnabledController extends SettingController { getValueOverride(level, roomId, calculatedValue) { const Notifier = require('../../Notifier'); // avoids cyclical references + if (!Notifier.isPossible()) return false; - return calculatedValue && Notifier.isEnabled(); + if (calculatedValue === null) { + return isMasterRuleEnabled(); + } + + return calculatedValue; } } export class AudioNotificationsEnabledController extends SettingController { getValueOverride(level, roomId, calculatedValue) { const Notifier = require('../../Notifier'); // avoids cyclical references + if (!Notifier.isPossible()) return false; - return calculatedValue && Notifier.isEnabled(); + // Note: Audio notifications are *not* enabled by default. + return calculatedValue; } } From d0a0a9ce7fe7264f08586d7280c100bfc250ceed Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 15 Nov 2017 22:31:16 -0700 Subject: [PATCH 7/8] Fix URL preview string not being translated Signed-off-by: Travis Ralston --- src/components/views/room_settings/UrlPreviewSettings.js | 6 +++--- src/i18n/strings/en_EN.json | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/components/views/room_settings/UrlPreviewSettings.js b/src/components/views/room_settings/UrlPreviewSettings.js index 6fb04f3378..7fe4472017 100644 --- a/src/components/views/room_settings/UrlPreviewSettings.js +++ b/src/components/views/room_settings/UrlPreviewSettings.js @@ -17,7 +17,7 @@ limitations under the License. const React = require('react'); const sdk = require("../../../index"); -import { _t, _tJsx } from '../../../languageHandler'; +import {_t, _td, _tJsx} from '../../../languageHandler'; import SettingsStore, {SettingLevel} from "../../../settings/SettingsStore"; @@ -63,9 +63,9 @@ module.exports = React.createClass({ ); } else { - let str = "URL previews are enabled by default for participants in this room."; + let str = _td("URL previews are enabled by default for participants in this room."); if (!SettingsStore.getValueAt(SettingLevel.ROOM, "urlPreviewsEnabled")) { - str = "URL previews are disabled by default for participants in this room."; + str = _td("URL previews are disabled by default for participants in this room."); } previewsForRoom = (); } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 51f23c0968..ee299eee58 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -210,6 +210,8 @@ "Enable Notifications": "Enable Notifications", "You have enabled URL previews by default.": "You have enabled URL previews by default.", "You have disabled URL previews by default.": "You have disabled URL previews by default.", + "URL previews are enabled by default for participants in this room.": "URL previews are enabled by default for participants in this room.", + "URL previews are disabled by default for participants in this room.": "URL previews are disabled by default for participants in this room.", "URL Previews": "URL Previews", "Cannot add any more widgets": "Cannot add any more widgets", "The maximum permitted number of widgets have already been added to this room.": "The maximum permitted number of widgets have already been added to this room.", From 3e13a919edcd85ef0a7656bcdf1a78eb310a5639 Mon Sep 17 00:00:00 2001 From: Matthew Hodgson Date: Thu, 16 Nov 2017 13:09:53 +0000 Subject: [PATCH 8/8] remove rogue debug --- src/settings/controllers/NotificationControllers.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/settings/controllers/NotificationControllers.js b/src/settings/controllers/NotificationControllers.js index 3831cd6891..9dcf78e26b 100644 --- a/src/settings/controllers/NotificationControllers.js +++ b/src/settings/controllers/NotificationControllers.js @@ -40,7 +40,6 @@ export class NotificationsEnabledController extends SettingController { if (!Notifier.isPossible()) return false; if (calculatedValue === null) { - console.log(isMasterRuleEnabled()); return isMasterRuleEnabled(); }