(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

@@ -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; }

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<any | null>;
+  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<any>[]) => {
-      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<Theme>) {
+    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.

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<Theme>,
+  }) {
+    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);

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<Theme>,
     mainPath?: string,
     baseLogger?: BaseLogger,
     rpcLogger?: IRpcLogger,
-    theme?: Computed<Theme>,
   }) {
     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<boolean>;
+  // Set once all of the plugin's onThemeChange handlers have been called.
+  protected _themeInitialized: Observable<boolean>;
 }
 
 /**
@@ -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});
   }
 }
 

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);
 

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<boolean>;
 }

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() {

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;
 }

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([], {

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;
 }
 
 /**

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<unknown>[] = [];
-
 /**
  * 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<unknown>[] = [];
  * the document that contains it.
  */
 export function onOptions(callback: (options: any, settings: InteractionOptions) => unknown) {
-  let finishInitialization: () => void;
-  if (!_readyCalled) {
-    const promise = new Promise<void>(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();

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);