Use singleton settings outside the Roact tree (#576)

* Use singleton settings outside the Roact tree

* Cleanup listener on unmount

* Refactor setting page components

* Fix willUnmount being added to the wrong table

* Remove bindings in favor of state

plugin/src/App/PluginSettings.lua

@@ -1,123 +0,0 @@
---[[
-	Persistent plugin settings that can be accessed via Roact context.
-]]
-
-local Rojo = script:FindFirstAncestor("Rojo")
-
-local Roact = require(Rojo.Roact)
-
-local defaultSettings = {
-	openScriptsExternally = false,
-	twoWaySync = false,
-	showNotifications = true,
-	playSounds = true,
-}
-
-local Settings = {}
-Settings.__index = Settings
-
-function Settings.fromPlugin(plugin)
-	local values = {}
-
-	for name, defaultValue in pairs(defaultSettings) do
-		local savedValue = plugin:GetSetting("Rojo_" .. name)
-
-		if savedValue == nil then
-			plugin:SetSetting("Rojo_" .. name, defaultValue)
-			values[name] = defaultValue
-		else
-			values[name] = savedValue
-		end
-	end
-
-	return setmetatable({
-		__values = values,
-		__plugin = plugin,
-		__updateListeners = {},
-	}, Settings)
-end
-
-function Settings:get(name)
-	if defaultSettings[name] == nil then
-		error("Invalid setings name " .. tostring(name), 2)
-	end
-
-	return self.__values[name]
-end
-
-function Settings:set(name, value)
-	self.__plugin:SetSetting("Rojo_" .. name, value)
-	self.__values[name] = value
-
-	for callback in pairs(self.__updateListeners) do
-		callback(name, value)
-	end
-end
-
-function Settings:onUpdate(newCallback)
-	local newListeners = {}
-	for callback in pairs(self.__updateListeners) do
-		newListeners[callback] = true
-	end
-
-	newListeners[newCallback] = true
-	self.__updateListeners = newListeners
-
-	return function()
-		local newListeners = {}
-		for callback in pairs(self.__updateListeners) do
-			if callback ~= newCallback then
-				newListeners[callback] = true
-			end
-		end
-
-		self.__updateListeners = newListeners
-	end
-end
-
-local Context = Roact.createContext(nil)
-
-local StudioProvider = Roact.Component:extend("StudioProvider")
-
-function StudioProvider:init()
-	self.settings = Settings.fromPlugin(self.props.plugin)
-end
-
-function StudioProvider:render()
-	return Roact.createElement(Context.Provider, {
-		value = self.settings,
-	}, self.props[Roact.Children])
-end
-
-local InternalConsumer = Roact.Component:extend("InternalConsumer")
-
-function InternalConsumer:render()
-	return self.props.render(self.props.settings)
-end
-
-function InternalConsumer:didMount()
-	self.disconnect = self.props.settings:onUpdate(function()
-		-- Trigger a dummy state update to update the settings consumer.
-		self:setState({})
-	end)
-end
-
-function InternalConsumer:willUnmount()
-	self.disconnect()
-end
-
-local function with(callback)
-	return Roact.createElement(Context.Consumer, {
-		render = function(settings)
-			return Roact.createElement(InternalConsumer, {
-				settings = settings,
-				render = callback,
-			})
-		end,
-	})
-end
-
-return {
-	StudioProvider = StudioProvider,
-	with = with,
-}

plugin/src/App/StatusPages/Settings.lua

@@ -1,246 +0,0 @@
-local TextService = game:GetService("TextService")
-
-local Rojo = script:FindFirstAncestor("Rojo")
-local Plugin = Rojo.Plugin
-
-local Roact = require(Rojo.Roact)
-
-local Assets = require(Plugin.Assets)
-local Theme = require(Plugin.App.Theme)
-local PluginSettings = require(Plugin.App.PluginSettings)
-
-local Checkbox = require(Plugin.App.Components.Checkbox)
-local IconButton = require(Plugin.App.Components.IconButton)
-local ScrollingFrame = require(Plugin.App.Components.ScrollingFrame)
-
-local e = Roact.createElement
-
-local DIVIDER_FADE_SIZE = 0.1
-
-local function getTextBounds(text, textSize, font, lineHeight, bounds)
-	local textBounds = TextService:GetTextSize(text, textSize, font, bounds)
-
-	local lineCount = textBounds.Y / textSize
-	local lineHeightAbsolute = textSize * lineHeight
-
-	return Vector2.new(textBounds.X, lineHeightAbsolute * lineCount - (lineHeightAbsolute - textSize))
-end
-
-local function Navbar(props)
-	return Theme.with(function(theme)
-		theme = theme.Settings.Navbar
-
-		return e("Frame", {
-			Size = UDim2.new(1, 0, 0, 46),
-			LayoutOrder = props.layoutOrder,
-			BackgroundTransparency = 1,
-		}, {
-			Back = e(IconButton, {
-				icon = Assets.Images.Icons.Back,
-				iconSize = 24,
-				color = theme.BackButtonColor,
-				transparency = props.transparency,
-
-				position = UDim2.new(0, 0, 0.5, 0),
-				anchorPoint = Vector2.new(0, 0.5),
-
-				onClick = props.onBack,
-			}),
-
-			Text = e("TextLabel", {
-				Text = "Settings",
-				Font = Enum.Font.Gotham,
-				TextSize = 18,
-				TextColor3 = theme.TextColor,
-				TextTransparency = props.transparency,
-
-				Size = UDim2.new(1, 0, 1, 0),
-
-				BackgroundTransparency = 1,
-			})
-		})
-	end)
-end
-
-local Setting = Roact.Component:extend("Setting")
-
-function Setting:init()
-	self.contentSize, self.setContentSize = Roact.createBinding(Vector2.new(0, 0))
-	self.containerSize, self.setContainerSize = Roact.createBinding(Vector2.new(0, 0))
-end
-
-function Setting:render()
-	return Theme.with(function(theme)
-		theme = theme.Settings
-
-		return PluginSettings.with(function(settings)
-			return e("Frame", {
-				Size = self.contentSize:map(function(value)
-					return UDim2.new(1, 0, 0, 20 + value.Y + 20)
-				end),
-				LayoutOrder = self.props.layoutOrder,
-				BackgroundTransparency = 1,
-
-				[Roact.Change.AbsoluteSize] = function(object)
-					self.setContainerSize(object.AbsoluteSize)
-				end,
-			}, {
-				Checkbox = e(Checkbox, {
-					active = settings:get(self.props.id),
-					transparency = self.props.transparency,
-					position = UDim2.new(1, 0, 0.5, 0),
-					anchorPoint = Vector2.new(1, 0.5),
-					onClick = function()
-						local currentValue = settings:get(self.props.id)
-						settings:set(self.props.id, not currentValue)
-					end,
-				}),
-
-				Text = e("Frame", {
-					Size = UDim2.new(1, 0, 1, 0),
-					BackgroundTransparency = 1,
-				}, {
-					Name = e("TextLabel", {
-						Text = self.props.name,
-						Font = Enum.Font.GothamBold,
-						TextSize = 17,
-						TextColor3 = theme.Setting.NameColor,
-						TextXAlignment = Enum.TextXAlignment.Left,
-						TextTransparency = self.props.transparency,
-
-						Size = UDim2.new(1, 0, 0, 17),
-
-						LayoutOrder = 1,
-						BackgroundTransparency = 1,
-					}),
-
-					Description = e("TextLabel", {
-						Text = self.props.description,
-						Font = Enum.Font.Gotham,
-						LineHeight = 1.2,
-						TextSize = 14,
-						TextColor3 = theme.Setting.DescriptionColor,
-						TextXAlignment = Enum.TextXAlignment.Left,
-						TextTransparency = self.props.transparency,
-						TextWrapped = true,
-
-						Size = self.containerSize:map(function(value)
-							local textBounds = getTextBounds(
-								self.props.description, 14, Enum.Font.Gotham, 1.2,
-								Vector2.new(value.X - 50, math.huge)
-							)
-							return UDim2.new(1, -50, 0, textBounds.Y)
-						end),
-
-						LayoutOrder = 2,
-						BackgroundTransparency = 1,
-					}),
-
-					Layout = e("UIListLayout", {
-						VerticalAlignment = Enum.VerticalAlignment.Center,
-						FillDirection = Enum.FillDirection.Vertical,
-						SortOrder = Enum.SortOrder.LayoutOrder,
-						Padding = UDim.new(0, 6),
-
-						[Roact.Change.AbsoluteContentSize] = function(object)
-							self.setContentSize(object.AbsoluteContentSize)
-						end,
-					}),
-
-					Padding = e("UIPadding", {
-						PaddingTop = UDim.new(0, 20),
-						PaddingBottom = UDim.new(0, 20),
-					}),
-				}),
-
-				Divider = e("Frame", {
-					BackgroundColor3 = theme.DividerColor,
-					BackgroundTransparency = self.props.transparency,
-					Size = UDim2.new(1, 0, 0, 1),
-					BorderSizePixel = 0,
-				}, {
-					Gradient = e("UIGradient", {
-						Transparency = NumberSequence.new({
-							NumberSequenceKeypoint.new(0, 1),
-							NumberSequenceKeypoint.new(DIVIDER_FADE_SIZE, 0),
-							NumberSequenceKeypoint.new(1 - DIVIDER_FADE_SIZE, 0),
-							NumberSequenceKeypoint.new(1, 1),
-						}),
-					}),
-				}),
-			})
-		end)
-	end)
-end
-
-local SettingsPage = Roact.Component:extend("SettingsPage")
-
-function SettingsPage:init()
-	self.contentSize, self.setContentSize = Roact.createBinding(Vector2.new(0, 0))
-end
-
-function SettingsPage:render()
-	return Theme.with(function(theme)
-		theme = theme.Settings
-
-		return e(ScrollingFrame, {
-			size = UDim2.new(1, 0, 1, 0),
-			contentSize = self.contentSize,
-			transparency = self.props.transparency,
-		}, {
-			Navbar = e(Navbar, {
-				onBack = self.props.onBack,
-				transparency = self.props.transparency,
-				layoutOrder = 0,
-			}),
-
-			OpenScriptsExternally = e(Setting, {
-				id = "openScriptsExternally",
-				name = "Open Scripts Externally",
-				description = "Attempt to open scripts in an external editor",
-				transparency = self.props.transparency,
-				layoutOrder = 1,
-			}),
-
-			ShowNotifications = e(Setting, {
-				id = "showNotifications",
-				name = "Show Notifications",
-				description = "Popup notifications in viewport",
-				transparency = self.props.transparency,
-				layoutOrder = 2,
-			}),
-
-			PlaySounds = e(Setting, {
-				id = "playSounds",
-				name = "Play Sounds",
-				description = "Toggle sound effects",
-				transparency = self.props.transparency,
-				layoutOrder = 3,
-			}),
-
-			TwoWaySync = e(Setting, {
-				id = "twoWaySync",
-				name = "Two-Way Sync",
-				description = "EXPERIMENTAL! Editing files in Studio will sync them into the filesystem",
-				transparency = self.props.transparency,
-				layoutOrder = 4,
-			}),
-
-			Layout = e("UIListLayout", {
-				FillDirection = Enum.FillDirection.Vertical,
-				SortOrder = Enum.SortOrder.LayoutOrder,
-
-				[Roact.Change.AbsoluteContentSize] = function(object)
-					self.setContentSize(object.AbsoluteContentSize)
-				end,
-			}),
-
-			Padding = e("UIPadding", {
-				PaddingLeft = UDim.new(0, 20),
-				PaddingRight = UDim.new(0, 20),
-			}),
-		})
-	end)
-end
-
-return SettingsPage

plugin/src/App/StatusPages/Settings/Setting.lua

@@ -0,0 +1,149 @@
+local TextService = game:GetService("TextService")
+
+local Rojo = script:FindFirstAncestor("Rojo")
+local Plugin = Rojo.Plugin
+
+local Roact = require(Rojo.Roact)
+
+local Settings = require(Plugin.Settings)
+local Theme = require(Plugin.App.Theme)
+
+local Checkbox = require(Plugin.App.Components.Checkbox)
+
+local e = Roact.createElement
+
+local DIVIDER_FADE_SIZE = 0.1
+
+local function getTextBounds(text, textSize, font, lineHeight, bounds)
+	local textBounds = TextService:GetTextSize(text, textSize, font, bounds)
+
+	local lineCount = textBounds.Y / textSize
+	local lineHeightAbsolute = textSize * lineHeight
+
+	return Vector2.new(textBounds.X, lineHeightAbsolute * lineCount - (lineHeightAbsolute - textSize))
+end
+
+local Setting = Roact.Component:extend("Setting")
+
+function Setting:init()
+	self.contentSize, self.setContentSize = Roact.createBinding(Vector2.new(0, 0))
+	self.containerSize, self.setContainerSize = Roact.createBinding(Vector2.new(0, 0))
+
+	self:setState({
+		setting = Settings:get(self.props.id),
+	})
+
+	self.changedCleanup = Settings:onChanged(self.props.id, function(value)
+		self:setState({
+			setting = value,
+		})
+	end)
+end
+
+function Setting:willUnmount()
+	self.changedCleanup()
+end
+
+function Setting:render()
+	return Theme.with(function(theme)
+		theme = theme.Settings
+
+		return e("Frame", {
+			Size = self.contentSize:map(function(value)
+				return UDim2.new(1, 0, 0, 20 + value.Y + 20)
+			end),
+			LayoutOrder = self.props.layoutOrder,
+			BackgroundTransparency = 1,
+
+			[Roact.Change.AbsoluteSize] = function(object)
+				self.setContainerSize(object.AbsoluteSize)
+			end,
+		}, {
+			Checkbox = e(Checkbox, {
+				active = self.state.setting,
+				transparency = self.props.transparency,
+				position = UDim2.new(1, 0, 0.5, 0),
+				anchorPoint = Vector2.new(1, 0.5),
+				onClick = function()
+					local currentValue = Settings:get(self.props.id)
+					Settings:set(self.props.id, not currentValue)
+				end,
+			}),
+
+			Text = e("Frame", {
+				Size = UDim2.new(1, 0, 1, 0),
+				BackgroundTransparency = 1,
+			}, {
+				Name = e("TextLabel", {
+					Text = self.props.name,
+					Font = Enum.Font.GothamBold,
+					TextSize = 17,
+					TextColor3 = theme.Setting.NameColor,
+					TextXAlignment = Enum.TextXAlignment.Left,
+					TextTransparency = self.props.transparency,
+
+					Size = UDim2.new(1, 0, 0, 17),
+
+					LayoutOrder = 1,
+					BackgroundTransparency = 1,
+				}),
+
+				Description = e("TextLabel", {
+					Text = self.props.description,
+					Font = Enum.Font.Gotham,
+					LineHeight = 1.2,
+					TextSize = 14,
+					TextColor3 = theme.Setting.DescriptionColor,
+					TextXAlignment = Enum.TextXAlignment.Left,
+					TextTransparency = self.props.transparency,
+					TextWrapped = true,
+
+					Size = self.containerSize:map(function(value)
+						local textBounds = getTextBounds(
+							self.props.description, 14, Enum.Font.Gotham, 1.2,
+							Vector2.new(value.X - 50, math.huge)
+						)
+						return UDim2.new(1, -50, 0, textBounds.Y)
+					end),
+
+					LayoutOrder = 2,
+					BackgroundTransparency = 1,
+				}),
+
+				Layout = e("UIListLayout", {
+					VerticalAlignment = Enum.VerticalAlignment.Center,
+					FillDirection = Enum.FillDirection.Vertical,
+					SortOrder = Enum.SortOrder.LayoutOrder,
+					Padding = UDim.new(0, 6),
+
+					[Roact.Change.AbsoluteContentSize] = function(object)
+						self.setContentSize(object.AbsoluteContentSize)
+					end,
+				}),
+
+				Padding = e("UIPadding", {
+					PaddingTop = UDim.new(0, 20),
+					PaddingBottom = UDim.new(0, 20),
+				}),
+			}),
+
+			Divider = e("Frame", {
+				BackgroundColor3 = theme.DividerColor,
+				BackgroundTransparency = self.props.transparency,
+				Size = UDim2.new(1, 0, 0, 1),
+				BorderSizePixel = 0,
+			}, {
+				Gradient = e("UIGradient", {
+					Transparency = NumberSequence.new({
+						NumberSequenceKeypoint.new(0, 1),
+						NumberSequenceKeypoint.new(DIVIDER_FADE_SIZE, 0),
+						NumberSequenceKeypoint.new(1 - DIVIDER_FADE_SIZE, 0),
+						NumberSequenceKeypoint.new(1, 1),
+					}),
+				}),
+			}),
+		})
+	end)
+end
+
+return Setting

plugin/src/App/StatusPages/Settings/init.lua

@@ -0,0 +1,121 @@
+local Rojo = script:FindFirstAncestor("Rojo")
+local Plugin = Rojo.Plugin
+
+local Roact = require(Rojo.Roact)
+
+local Assets = require(Plugin.Assets)
+local Theme = require(Plugin.App.Theme)
+
+local IconButton = require(Plugin.App.Components.IconButton)
+local ScrollingFrame = require(Plugin.App.Components.ScrollingFrame)
+local Setting = require(script.Setting)
+
+local e = Roact.createElement
+
+local function Navbar(props)
+	return Theme.with(function(theme)
+		theme = theme.Settings.Navbar
+
+		return e("Frame", {
+			Size = UDim2.new(1, 0, 0, 46),
+			LayoutOrder = props.layoutOrder,
+			BackgroundTransparency = 1,
+		}, {
+			Back = e(IconButton, {
+				icon = Assets.Images.Icons.Back,
+				iconSize = 24,
+				color = theme.BackButtonColor,
+				transparency = props.transparency,
+
+				position = UDim2.new(0, 0, 0.5, 0),
+				anchorPoint = Vector2.new(0, 0.5),
+
+				onClick = props.onBack,
+			}),
+
+			Text = e("TextLabel", {
+				Text = "Settings",
+				Font = Enum.Font.Gotham,
+				TextSize = 18,
+				TextColor3 = theme.TextColor,
+				TextTransparency = props.transparency,
+
+				Size = UDim2.new(1, 0, 1, 0),
+
+				BackgroundTransparency = 1,
+			})
+		})
+	end)
+end
+
+local SettingsPage = Roact.Component:extend("SettingsPage")
+
+function SettingsPage:init()
+	self.contentSize, self.setContentSize = Roact.createBinding(Vector2.new(0, 0))
+end
+
+function SettingsPage:render()
+	return Theme.with(function(theme)
+		theme = theme.Settings
+
+		return e(ScrollingFrame, {
+			size = UDim2.new(1, 0, 1, 0),
+			contentSize = self.contentSize,
+			transparency = self.props.transparency,
+		}, {
+			Navbar = e(Navbar, {
+				onBack = self.props.onBack,
+				transparency = self.props.transparency,
+				layoutOrder = 0,
+			}),
+
+			OpenScriptsExternally = e(Setting, {
+				id = "openScriptsExternally",
+				name = "Open Scripts Externally",
+				description = "Attempt to open scripts in an external editor",
+				transparency = self.props.transparency,
+				layoutOrder = 1,
+			}),
+
+			ShowNotifications = e(Setting, {
+				id = "showNotifications",
+				name = "Show Notifications",
+				description = "Popup notifications in viewport",
+				transparency = self.props.transparency,
+				layoutOrder = 2,
+			}),
+
+			PlaySounds = e(Setting, {
+				id = "playSounds",
+				name = "Play Sounds",
+				description = "Toggle sound effects",
+				transparency = self.props.transparency,
+				layoutOrder = 3,
+			}),
+
+			TwoWaySync = e(Setting, {
+				id = "twoWaySync",
+				name = "Two-Way Sync",
+				description = "EXPERIMENTAL! Editing files in Studio will sync them into the filesystem",
+				transparency = self.props.transparency,
+				layoutOrder = 4,
+			}),
+
+			Layout = e("UIListLayout", {
+				FillDirection = Enum.FillDirection.Vertical,
+				SortOrder = Enum.SortOrder.LayoutOrder,
+
+				[Roact.Change.AbsoluteContentSize] = function(object)
+					self.setContentSize(object.AbsoluteContentSize)
+				end,
+			}),
+
+			Padding = e("UIPadding", {
+				PaddingLeft = UDim.new(0, 20),
+				PaddingRight = UDim.new(0, 20),
+			}),
+		})
+	end)
+end
+
+return SettingsPage

plugin/src/App/init.lua

@@ -7,6 +7,7 @@ local Log = require(Rojo.Log)
 local Assets = require(Plugin.Assets)
 local Version = require(Plugin.Version)
 local Config = require(Plugin.Config)
+local Settings = require(Plugin.Settings)
 local strict = require(Plugin.strict)
 local Dictionary = require(Plugin.Dictionary)
 local ServeSession = require(Plugin.ServeSession)
@@ -14,7 +15,6 @@ local ApiContext = require(Plugin.ApiContext)
 local preloadAssets = require(Plugin.preloadAssets)
 local soundPlayer = require(Plugin.soundPlayer)
 local Theme = require(script.Theme)
-local PluginSettings = require(script.PluginSettings)
 
 local Page = require(script.Page)
 local Notifications = require(script.Notifications)
@@ -52,7 +52,7 @@ function App:init()
 end
 
 function App:addNotification(text: string, timeout: number?)
-	if not self.props.settings:get("showNotifications") then
+	if not Settings:get("showNotifications") then
 		return
 	end
 
@@ -91,8 +91,8 @@ function App:startSession()
 	local host, port = self:getHostAndPort()
 
 	local sessionOptions = {
-		openScriptsExternally = self.props.settings:get("openScriptsExternally"),
+		openScriptsExternally = Settings:get("openScriptsExternally"),
-		twoWaySync = self.props.settings:get("twoWaySync"),
+		twoWaySync = Settings:get("twoWaySync"),
 	}
 
 	local baseUrl = ("http://%s:%s"):format(host, port)
@@ -345,15 +345,9 @@ function App:render()
 end
 
 return function(props)
-	return e(PluginSettings.StudioProvider, {
+	local mergedProps = Dictionary.merge(props, {
-		plugin = props.plugin,
+		soundPlayer = soundPlayer.new(Settings),
-	}, {
-		App = PluginSettings.with(function(settings)
-			local mergedProps = Dictionary.merge(props, {
-				settings = settings,
-				soundPlayer = soundPlayer.new(settings),
-			})
-			return e(App, mergedProps)
-		end),
 	})
+
+	return e(App, mergedProps)
 end

plugin/src/Settings.lua

@@ -0,0 +1,78 @@
+--[[
+	Persistent plugin settings.
+]]
+
+local plugin = plugin or script:FindFirstAncestorWhichIsA("Plugin")
+local Rojo = script:FindFirstAncestor("Rojo")
+
+local Log = require(Rojo.Log)
+
+local defaultSettings = {
+	openScriptsExternally = false,
+	twoWaySync = false,
+	showNotifications = true,
+	playSounds = true,
+}
+
+local Settings = {}
+
+Settings._values = table.clone(defaultSettings)
+Settings._updateListeners = {}
+
+if plugin then
+	for name, defaultValue in pairs(Settings._values) do
+		local savedValue = plugin:GetSetting("Rojo_" .. name)
+
+		if savedValue == nil then
+			-- plugin:SetSetting hits disc instead of memory, so it can be slow. Spawn so we don't hang.
+			task.spawn(plugin.SetSetting, plugin, "Rojo_" .. name, defaultValue)
+			Settings._values[name] = defaultValue
+		else
+			Settings._values[name] = savedValue
+		end
+	end
+	Log.trace("Loaded settings from plugin store")
+end
+
+function Settings:get(name)
+	if defaultSettings[name] == nil then
+		error("Invalid setings name " .. tostring(name), 2)
+	end
+
+	return self._values[name]
+end
+
+function Settings:set(name, value)
+	self._values[name] = value
+
+	if plugin then
+		-- plugin:SetSetting hits disc instead of memory, so it can be slow. Spawn so we don't hang.
+		task.spawn(plugin.SetSetting, plugin, "Rojo_" .. name, value)
+	end
+
+	if self._updateListeners[name] then
+		for callback in pairs(self._updateListeners[name]) do
+			task.spawn(callback, value)
+		end
+	end
+
+	Log.trace(string.format("Set setting '%s' to '%s'", name, tostring(value)))
+end
+
+function Settings:onChanged(name, callback)
+	local listeners = self._updateListeners[name]
+	if listeners == nil then
+		listeners = {}
+		self._updateListeners[name] = listeners
+	end
+	listeners[callback] = true
+
+	Log.trace(string.format("Added listener for setting '%s' changes", name))
+
+	return function()
+		listeners[callback] = nil
+		Log.trace(string.format("Removed listener for setting '%s' changes", name))
+	end
+end
+
+return Settings