From 1be1e5f647cba4c8637318523c68d3a45db31681 Mon Sep 17 00:00:00 2001 From: George Gevoian Date: Fri, 6 Oct 2023 09:17:39 -0400 Subject: [PATCH] (core) Move theme from ConfigNotifier to ThemeNotifier Summary: This reverts the behavior of onOptions, which had unintentionally changed recently and no longer matched the API documentation. Test Plan: Existing tests. Reviewers: jarek Reviewed By: jarek Subscribers: paulfitz Differential Revision: https://phab.getgrist.com/D4064 --- app/client/components/CustomView.ts | 12 +-- app/client/components/WidgetFrame.ts | 95 ++++++++++++-------- app/client/lib/HomePluginManager.ts | 13 ++- app/client/lib/SafeBrowser.ts | 24 ++--- app/client/models/HomeModel.ts | 10 ++- app/client/models/entities/ViewSectionRec.ts | 4 +- app/client/ui/CustomSectionConfig.ts | 8 +- app/common/CustomWidget.ts | 4 +- app/plugin/CustomSectionAPI-ti.ts | 1 - app/plugin/CustomSectionAPI.ts | 10 --- app/plugin/grist-plugin-api.ts | 70 ++++++--------- test/client/lib/SafeBrowser.ts | 3 + 12 files changed, 127 insertions(+), 127 deletions(-) diff --git a/app/client/components/CustomView.ts b/app/client/components/CustomView.ts index 1668ff25..d5599cd9 100644 --- a/app/client/components/CustomView.ts +++ b/app/client/components/CustomView.ts @@ -11,6 +11,7 @@ import { MinimumLevel, RecordNotifier, TableNotifier, + ThemeNotifier, WidgetAPIImpl, WidgetFrame } from 'app/client/components/WidgetFrame'; @@ -215,6 +216,7 @@ export class CustomView extends Disposable { }) { const {baseUrl, access, showAfterReady} = options; const documentSettings = this.gristDoc.docData.docSettings(); + const readonly = this.gristDoc.isReadonly.get(); return grains.create(WidgetFrame, { url: baseUrl || this.getEmptyWidgetPage(), access, @@ -225,13 +227,8 @@ export class CustomView extends Disposable { timeZone: this.gristDoc.docInfo.timezone() ?? "UTC", currency: documentSettings.currency?? "USD", }, - readonly: this.gristDoc.isReadonly.get(), + readonly, showAfterReady, - onSettingsInitialized: async () => { - if (!this.customDef.renderAfterReady.peek()) { - await this.customDef.renderAfterReady.setAndSave(true); - } - }, configure: (frame) => { this._frame = frame; // Need to cast myself to a BaseView @@ -266,6 +263,9 @@ export class CustomView extends Disposable { theme: this.gristDoc.currentTheme, }), new MinimumLevel(AccessLevel.none)); // none access is enough + frame.useEvents( + ThemeNotifier.create(frame, this.gristDoc.currentTheme), + new MinimumLevel(AccessLevel.none)); }, onElem: (iframe) => onFrameFocus(iframe, () => { if (this.isDisposed()) { return; } diff --git a/app/client/components/WidgetFrame.ts b/app/client/components/WidgetFrame.ts index 3043514f..e225eb37 100644 --- a/app/client/components/WidgetFrame.ts +++ b/app/client/components/WidgetFrame.ts @@ -54,17 +54,10 @@ export interface WidgetFrameOptions { /** * If set, show the iframe after `grist.ready()`. * - * Currently, this is only used to defer showing a widget until it has had - * a chance to apply the Grist theme. + * This is used to defer showing a widget on initial load until it has finished + * applying the Grist theme. */ showAfterReady?: boolean; - /** - * Handler for the settings initialized message. - * - * Currently, this is only used to defer showing a widget until it has had - * a chance to apply the Grist theme. - */ - onSettingsInitialized: () => void; /** * Optional callback to configure exposed API. */ @@ -216,9 +209,12 @@ export class WidgetFrame extends DisposableWithEvents { this.trigger('ready', this); this._readyCalled.set(true); } - if (event.data.data?.settings?.status === 'initialized') { + if ( + event.data.data?.message === 'themeInitialized' || + // DEPRECATED: remove once the plugin API starts sending the message above. + event.data.data?.settings?.status === 'initialized' + ) { this._visible.set(true); - this._options.onSettingsInitialized(); } this._rpc.receiveMessage(event.data); } @@ -580,56 +576,79 @@ export interface ConfigNotifierOptions { } /** - * Notifies about options change. Exposed in the API as a onOptions handler. + * Notifies about options changes. Exposed in the API as `onOptions`. */ export class ConfigNotifier extends BaseEventSource { private _accessLevel = this._options.access; private _theme = this._options.theme; - private _currentConfig: Computed; + private _currentConfig = Computed.create(this, use => { + const options = use(this._section.activeCustomOptions); + return options; + }); + // Debounced call to let the view know linked cursor changed. - private _debounced: (fromReady?: boolean) => void; + private _debounced = debounce((options?: {fromReady?: boolean}) => this._update(options), 0); + constructor(private _section: ViewSectionRec, private _options: ConfigNotifierOptions) { super(); - this._currentConfig = Computed.create(this, use => { - const options = use(this._section.activeCustomOptions); - return options; - }); - this._debounced = debounce((fromReady?: boolean) => this._update(fromReady), 0); - const subscribe = (...observables: Observable[]) => { - for (const obs of observables) { - this.autoDispose( - obs.addListener((cur, prev) => { - if (isEqual(prev, cur)) { - return; - } - this._debounced(); - }) - ); - } - }; - subscribe(this._currentConfig, this._theme); + this.autoDispose( + this._currentConfig.addListener((newConfig, oldConfig) => { + if (isEqual(newConfig, oldConfig)) { return; } + + this._debounced(); + }) + ); } protected _ready() { // On ready, send initial configuration. - this._debounced(true); + this._debounced({fromReady: true}); } - private _update(fromReady = false) { - if (this.isDisposed()) { - return; - } + private _update({fromReady}: {fromReady?: boolean} = {}) { + if (this.isDisposed()) { return; } + this._notify({ options: this._currentConfig.get(), settings: { accessLevel: this._accessLevel, - theme: this._theme.get(), + // DEPRECATED: remove once the plugin API includes the `onThemeChange` handler. + ...(fromReady ? {theme: this._theme.get()} : undefined), }, fromReady, }); } } +/** + * Notifies about theme changes. Exposed in the API as `onThemeChange`. + */ +export class ThemeNotifier extends BaseEventSource { + constructor(private _theme: Computed) { + super(); + this.autoDispose( + this._theme.addListener((newTheme, oldTheme) => { + if (isEqual(newTheme, oldTheme)) { return; } + + this._update(); + }) + ); + } + + protected _ready() { + this._update({fromReady: true}); + } + + private _update({fromReady}: {fromReady?: boolean} = {}) { + if (this.isDisposed()) { return; } + + this._notify({ + theme: this._theme.get(), + fromReady, + }); + } +} + /** * Notifies about cursor table data or structure change. * Exposed in the API as a onRecords handler. diff --git a/app/client/lib/HomePluginManager.ts b/app/client/lib/HomePluginManager.ts index e96e4438..0f5f31b7 100644 --- a/app/client/lib/HomePluginManager.ts +++ b/app/client/lib/HomePluginManager.ts @@ -2,6 +2,8 @@ import {ClientScope} from 'app/client/components/ClientScope'; import {SafeBrowser} from 'app/client/lib/SafeBrowser'; import {LocalPlugin} from 'app/common/plugin'; import {createRpcLogger, PluginInstance} from 'app/common/PluginInstance'; +import {Theme} from 'app/common/ThemePrefs'; +import {Computed} from 'grainjs'; /** * Home plugins are all plugins that contributes to a general Grist management tasks. @@ -13,9 +15,13 @@ export class HomePluginManager { public pluginsList: PluginInstance[]; - constructor(localPlugins: LocalPlugin[], - untrustedContentOrigin: string, - clientScope: ClientScope) { + constructor(options: { + localPlugins: LocalPlugin[], + untrustedContentOrigin: string, + clientScope: ClientScope, + theme: Computed, + }) { + const {localPlugins, untrustedContentOrigin, clientScope, theme} = options; this.pluginsList = []; for (const plugin of localPlugins) { try { @@ -35,6 +41,7 @@ export class HomePluginManager { clientScope, untrustedContentOrigin, mainPath: components.safeBrowser, + theme, }); if (components.safeBrowser) { pluginInstance.rpc.registerForwarder(components.safeBrowser, safeBrowser); diff --git a/app/client/lib/SafeBrowser.ts b/app/client/lib/SafeBrowser.ts index fe50c688..35182cb9 100644 --- a/app/client/lib/SafeBrowser.ts +++ b/app/client/lib/SafeBrowser.ts @@ -73,7 +73,7 @@ export class SafeBrowser extends BaseComponent { new IframeProcess(safeBrowser, rpc, src); } - public theme? = this._options.theme; + public theme = this._options.theme; // All view processes. This is not used anymore to dispose all processes on deactivation (this is // now achieved using `this._mainProcess.autoDispose(...)`) but rather to be able to dispatch @@ -94,10 +94,10 @@ export class SafeBrowser extends BaseComponent { pluginInstance: PluginInstance, clientScope: ClientScope, untrustedContentOrigin: string, + theme: Computed, mainPath?: string, baseLogger?: BaseLogger, rpcLogger?: IRpcLogger, - theme?: Computed, }) { super( _options.pluginInstance.definition.manifest, @@ -292,8 +292,8 @@ class WorkerProcess extends ClientProcess { export class ViewProcess extends ClientProcess { public element: HTMLElement; - // Set once all of the plugin's onOptions handlers have been called. - protected _optionsInitialized: Observable; + // Set once all of the plugin's onThemeChange handlers have been called. + protected _themeInitialized: Observable; } /** @@ -302,21 +302,21 @@ export class ViewProcess extends ClientProcess { class IframeProcess extends ViewProcess { public create(safeBrowser: SafeBrowser, rpc: Rpc, src: string) { super.create(safeBrowser, rpc, src); - this._optionsInitialized = Observable.create(this, false); + this._themeInitialized = Observable.create(this, false); const iframe = this.element = this.autoDispose( grainjsDom(`iframe.safe_browser_process.clipboard_focus`, {src}, - grainjsDom.style('visibility', use => use(this._optionsInitialized) ? 'visible' : 'hidden'), + grainjsDom.style('visibility', use => use(this._themeInitialized) ? 'visible' : 'hidden'), ) as HTMLIFrameElement ); const listener = async (event: MessageEvent) => { if (event.source === iframe.contentWindow) { if (event.data.mtype === MsgType.Ready) { - await this._sendSettings({theme: safeBrowser.theme?.get()}, true); + await this._sendTheme({theme: safeBrowser.theme.get(), fromReady: true}); } - if (event.data.data?.settings?.status === 'initialized') { - this._optionsInitialized.set(true); + if (event.data.data?.message === 'themeInitialized') { + this._themeInitialized.set(true); } this.rpc.receiveMessage(event.data); @@ -333,14 +333,14 @@ class IframeProcess extends ViewProcess { safeBrowser.theme.addListener(async (newTheme, oldTheme) => { if (isEqual(newTheme, oldTheme)) { return; } - await this._sendSettings({theme: safeBrowser.theme?.get()}); + await this._sendTheme({theme: newTheme}); }) ); } } - private async _sendSettings(settings: {theme?: Theme}, fromReady = false) { - await this.rpc.postMessage({settings, fromReady}); + private async _sendTheme({theme, fromReady = false}: {theme: Theme, fromReady?: boolean}) { + await this.rpc.postMessage({theme, fromReady}); } } diff --git a/app/client/models/HomeModel.ts b/app/client/models/HomeModel.ts index 6c4f1633..0c61a54f 100644 --- a/app/client/models/HomeModel.ts +++ b/app/client/models/HomeModel.ts @@ -186,10 +186,12 @@ export class HomeModelImpl extends Disposable implements HomeModel, ViewSettings this._updateWorkspaces().catch(reportError))); // Defer home plugin initialization - const pluginManager = new HomePluginManager( - _app.topAppModel.plugins, - _app.topAppModel.getUntrustedContentOrigin()!, - clientScope); + const pluginManager = new HomePluginManager({ + localPlugins: _app.topAppModel.plugins, + untrustedContentOrigin: _app.topAppModel.getUntrustedContentOrigin()!, + clientScope, + theme: _app.currentTheme, + }); const importSources = ImportSourceElement.fromArray(pluginManager.pluginsList); this.importSources.set(importSources); diff --git a/app/client/models/entities/ViewSectionRec.ts b/app/client/models/entities/ViewSectionRec.ts index 3e6ad7d3..8feae770 100644 --- a/app/client/models/entities/ViewSectionRec.ts +++ b/app/client/models/entities/ViewSectionRec.ts @@ -272,8 +272,8 @@ export interface CustomViewSectionDef { /** * If set, render the widget after `grist.ready()`. * - * Currently, this is only used to defer rendering a widget until it has had - * a chance to apply the Grist theme. + * This is used to defer showing a widget on initial load until it has finished + * applying the Grist theme. */ renderAfterReady: modelUtil.KoSaveableObservable; } diff --git a/app/client/ui/CustomSectionConfig.ts b/app/client/ui/CustomSectionConfig.ts index 46aa9b80..538562f5 100644 --- a/app/client/ui/CustomSectionConfig.ts +++ b/app/client/ui/CustomSectionConfig.ts @@ -459,7 +459,7 @@ export class CustomSectionConfig extends Disposable { } bundleChanges(() => { // Reset whether widget should render after `grist.ready()`. - _section.customDef.renderAfterReady(false); + _section.customDef.renderAfterReady(selectedWidget.renderAfterReady ?? false); // Clear access level _section.customDef.access(AccessLevel.none); // When widget wants some access, set desired access level. @@ -627,16 +627,16 @@ export class CustomSectionConfig extends Disposable { protected async _getWidgets() { const api = this._gristDoc.app.topAppModel.api; - const wigets = await api.getWidgets(); + const widgets = await api.getWidgets(); // Request for rest of the widgets. if (this._canSelect) { // From the start we will provide single widget definition // that was chosen previously. if (this._section.customDef.widgetDef.peek()) { - wigets.push(this._section.customDef.widgetDef.peek()!); + widgets.push(this._section.customDef.widgetDef.peek()!); } } - this._widgets.set(wigets); + this._widgets.set(widgets); } private _accept() { diff --git a/app/common/CustomWidget.ts b/app/common/CustomWidget.ts index 115e0a2b..89324e19 100644 --- a/app/common/CustomWidget.ts +++ b/app/common/CustomWidget.ts @@ -21,8 +21,8 @@ export interface ICustomWidget { /** * If set, Grist will render the widget after `grist.ready()`. * - * Currently, this is only used to defer rendering a widget until it has had - * a chance to apply the Grist theme. + * This is used to defer showing a widget on initial load until it has finished + * applying the Grist theme. */ renderAfterReady?: boolean; } diff --git a/app/plugin/CustomSectionAPI-ti.ts b/app/plugin/CustomSectionAPI-ti.ts index aea8a325..fe49f7d9 100644 --- a/app/plugin/CustomSectionAPI-ti.ts +++ b/app/plugin/CustomSectionAPI-ti.ts @@ -25,7 +25,6 @@ export const InteractionOptionsRequest = t.iface([], { export const InteractionOptions = t.iface([], { "accessLevel": "string", - "theme": "any", }); export const WidgetColumnMap = t.iface([], { diff --git a/app/plugin/CustomSectionAPI.ts b/app/plugin/CustomSectionAPI.ts index 8f87769e..6fe30bf3 100644 --- a/app/plugin/CustomSectionAPI.ts +++ b/app/plugin/CustomSectionAPI.ts @@ -69,16 +69,6 @@ export interface InteractionOptions{ * Granted access level. */ accessLevel: string, - /** - * Information about the current Grist theme. - * - * Includes the theme appearance ("light" or "dark"), and a mapping of UI elements to - * CSS color values. The CSS values are also accessible within a widget via CSS variables - * prefixed with "--grist-theme-" (e.g. `var(--grist-theme-text)`). - * - * NOTE: the variables aren't yet finalized and may change in the future. - */ - theme: any; } /** diff --git a/app/plugin/grist-plugin-api.ts b/app/plugin/grist-plugin-api.ts index 001947eb..d9585881 100644 --- a/app/plugin/grist-plugin-api.ts +++ b/app/plugin/grist-plugin-api.ts @@ -380,14 +380,6 @@ export function onRecords(callback: (data: RowRecord[], mappings: WidgetColumnMa }); } -/* Keep track of the completion status of all initial onOptions calls made prior to sending - * the ready message. The `ready` function will wait for this list of calls to settle after - * it receives the initial options message from Grist. - * - * Note that this includes all internal calls to `onOptions`, such as the one made by - * `syncThemeWithGrist`. */ -const _pendingInitializeOptionsCalls: Promise[] = []; - /** * For custom widgets, add a handler that will be called whenever the * widget options change (and on initial ready message). Handler will be @@ -396,16 +388,26 @@ const _pendingInitializeOptionsCalls: Promise[] = []; * the document that contains it. */ export function onOptions(callback: (options: any, settings: InteractionOptions) => unknown) { - let finishInitialization: () => void; - if (!_readyCalled) { - const promise = new Promise(resolve => { finishInitialization = resolve; }); - _pendingInitializeOptionsCalls.push(promise); - } - - on('message', async function(msg) { + on('message', function(msg) { if (msg.settings) { - await callback(msg.options || null, msg.settings); - finishInitialization?.(); + callback(msg.options || null, msg.settings); + } + }); +} + +/** + * Called whenever the Grist theme changes (and on initial ready message). + */ +function onThemeChange(callback: (theme: any) => unknown) { + on('message', function(msg) { + if (msg.theme) { + callback(msg.theme); + + if (msg.fromReady) { + void (async function() { + await rpc.postMessage({message: 'themeInitialized'}); + })(); + } } }); } @@ -468,24 +470,6 @@ export function ready(settings?: ReadyPayload): void { rpc.registerFunc('editOptions', settings.onEditOptions); } on('message', async function(msg) { - if (msg.settings && msg.fromReady) { - /* Grist sends an options message immediately after receiving a ready message, containing - * some initial settings (such as the Grist theme). Grist may decide to wait until all - * initial onOptions calls have completed before making the custom widget's iframe visible - * to the client. This is the case when a widget's manifest explicitly declares a widget - * must be rendered after the ready, or in subsequent renders of a custom widget that - * previously sent Grist a ready message. The reason for this behavior is to make the - * experience of embedding iframes within a Grist document feel more seamless and polished. - * - * Here, we check for that initial options message, waiting for all onOptions calls to - * settle before notifying Grist that all settings have been initialized. */ - await Promise.all(_pendingInitializeOptionsCalls); - - void (async function() { - await rpc.postMessage({settings: {status: 'initialized'}}); - })(); - } - if (msg.tableId && msg.tableId !== _tableId) { if (!_tableId) { _setInitialized(); } _tableId = msg.tableId; @@ -566,14 +550,12 @@ function createRpcLogger(): IRpcLogger { let _theme: any; -function syncThemeWithGrist() { - onOptions((_options, settings) => { - if (settings.theme && !isEqual(settings.theme, _theme)) { - _theme = settings.theme; - attachCssThemeVars(_theme); - } - }); -} +onThemeChange((newTheme) => { + if (isEqual(newTheme, _theme)) { return; } + + _theme = newTheme; + attachCssThemeVars(_theme); +}); function attachCssThemeVars({appearance, colors}: any) { // Prepare the custom properties needed for applying the theme. @@ -613,5 +595,3 @@ function getOrCreateStyleElement(id: string) { document.head.append(style); return style; } - -syncThemeWithGrist(); diff --git a/test/client/lib/SafeBrowser.ts b/test/client/lib/SafeBrowser.ts index 5d7b21ba..0ff8b5a2 100644 --- a/test/client/lib/SafeBrowser.ts +++ b/test/client/lib/SafeBrowser.ts @@ -3,11 +3,13 @@ import { Disposable } from 'app/client/lib/dispose'; import { ClientProcess, SafeBrowser } from 'app/client/lib/SafeBrowser'; import { LocalPlugin } from 'app/common/plugin'; import { PluginInstance } from 'app/common/PluginInstance'; +import { GristLight } from 'app/common/themes/GristLight'; import { GristAPI, RPC_GRISTAPI_INTERFACE } from 'app/plugin/GristAPI'; import { Storage } from 'app/plugin/StorageAPI'; import { checkers } from 'app/plugin/TypeCheckers'; import { assert } from 'chai'; import { Rpc } from 'grain-rpc'; +import { Computed } from 'grainjs'; import { noop } from 'lodash'; import { basename } from 'path'; import * as sinon from 'sinon'; @@ -186,6 +188,7 @@ describe('SafeBrowser', function() { untrustedContentOrigin: '', mainPath, baseLogger: {}, + theme: Computed.create(null, () => ({appearance: 'light', colors: GristLight})), }); cleanup.push(() => safeBrowser.deactivate()); pluginInstance.rpc.registerForwarder(mainPath, safeBrowser);