(core) Guessing column widget options when transforming from

Summary: When converting changing the type of Any column, try to guess the widgetOptions. Especially important for choice and choiceList types. Test Plan: Existing Reviewers: alexmojaki Reviewed By: alexmojaki Differential Revision: https://phab.getgrist.com/D4088
2024-10-27 20:44:07 +00:00 · 2023-10-30 11:40:28 +01:00 · 2023-10-30 11:40:28 +01:00 · c7ba31eb7d
commit c7ba31eb7d
parent cc9a9ae8c5
4 changed files with 156 additions and 71 deletions
--- a/app/client/components/TypeConversion.ts
+++ b/app/client/components/TypeConversion.ts
@ -17,15 +17,16 @@ import {dateTimeWidgetOptions, guessDateFormat, timeFormatOptions} from 'app/com
 import {TableData} from 'app/common/TableData';
 import {decodeObject} from 'app/plugin/objtypes';

-interface ColInfo {
+interface PrepColInfo {
  type: string;
  isFormula: boolean;
-  formula: string;
+  formula?: string;
  visibleCol: number;
  widgetOptions?: string;
  rules: gristTypes.RefListValue
 }

+
 /**
 * Returns the suggested full type for `column` given a desired pure type to convert it to.
 * Specifically, a pure type of "DateTime" returns a full type of "DateTime:{timezone}", and "Ref"
@ -85,8 +86,14 @@ function getRefTableIdFromData(docModel: DocModel, column: ColumnRec): string|nu
 // ColInfo to use for the transform column. Note that isFormula will be set to true, and formula
 // will be set to the expression to compute the new values from the old ones.
 // @param toTypeMaybeFull: Type to convert the column to, either full ('Ref:Foo') or pure ('Ref').
-export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRec, origDisplayCol: ColumnRec,
-                                           toTypeMaybeFull: string, convertedRef: string): Promise<ColInfo> {
+export async function prepTransformColInfo(options: {
+  docModel: DocModel;
+  origCol: ColumnRec;
+  origDisplayCol: ColumnRec;
+  toTypeMaybeFull: string;
+  convertedRef?: string
+}): Promise<PrepColInfo> {
+  const {docModel, origCol, origDisplayCol, toTypeMaybeFull, convertedRef} = options;
  const toType = gristTypes.extractTypeFromColType(toTypeMaybeFull);
  const tableData: TableData = docModel.docData.getTable(origCol.table().tableId())!;

@ -115,7 +122,7 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe
    }
  }

-  const colInfo: ColInfo = {
+  const colInfo: PrepColInfo = {
    type: addColTypeSuffix(toTypeMaybeFull, origCol, docModel),
    isFormula: true,
    visibleCol: 0,
@ -123,6 +130,71 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe
    rules: origCol.rules(),
  };

+  switch (toType) {
+    case 'Ref':
+    case 'RefList':
+    {
+      // Set suggested destination table and visible column.
+      // Undefined if toTypeMaybeFull is a pure type (e.g. converting to Ref before a table is chosen).
+      const optTableId = gutil.removePrefix(toTypeMaybeFull, `${toType}:`) || undefined;
+
+      let suggestedColRef: number;
+      let suggestedTableId: string;
+      const origColTypeInfo = gristTypes.extractInfoFromColType(origCol.type.peek());
+      if (!optTableId && (origColTypeInfo.type === "Ref" || origColTypeInfo.type === "RefList")) {
+        // When converting between Ref and Reflist, initially suggest the same table and visible column.
+        // When converting, if the table is the same, it's a special case.
+        // The visible column will not affect conversion.
+        // It will simply wrap the reference (row ID) in a list or extract the one element of a reference list.
+        suggestedColRef = origCol.visibleCol.peek();
+        suggestedTableId = origColTypeInfo.tableId;
+      } else {
+        // Finds a reference suggestion column and sets it as the current reference value.
+        const columnData = tableData.getDistinctValues(origDisplayCol.colId(), 100);
+        if (!columnData) { break; }
+        columnData.delete(gristTypes.getDefaultForType(origCol.type()));
+
+        // 'findColFromValues' function requires an array since it sends the values to the sandbox.
+        const matches: number[] = await docModel.docData.findColFromValues(Array.from(columnData), 2, optTableId);
+        suggestedColRef = matches.find(match => match !== origCol.getRowId())!;
+        if (!suggestedColRef) { break; }
+        const suggestedCol = docModel.columns.getRowModel(suggestedColRef);
+        suggestedTableId = suggestedCol.table().tableId();
+        if (optTableId && suggestedTableId !== optTableId) {
+          console.warn("Inappropriate column received from findColFromValues");
+          break;
+        }
+      }
+      colInfo.type = `${toType}:${suggestedTableId}`;
+      colInfo.visibleCol = suggestedColRef;
+      break;
+    }
+    default:
+      widgetOptions = guessWidgetOptionsSync({docModel, origCol, toTypeMaybeFull, widgetOptions});
+  }
+
+  if (Object.keys(widgetOptions).length) {
+    colInfo.widgetOptions = JSON.stringify(widgetOptions);
+  }
+  return colInfo;
+}
+
+/**
+ * Tries to guess widget options for a given column, based on the type it's being converted to.
+ * It works synchronously, so it can't reason about options that require async calls to the data-engine.
+ */
+export function guessWidgetOptionsSync(options: {
+  docModel: DocModel;
+  origCol: ColumnRec;
+  toTypeMaybeFull: string;
+  widgetOptions?: any;
+}): object {
+  const {docModel, origCol, toTypeMaybeFull} = options;
+  const toType = gristTypes.extractTypeFromColType(toTypeMaybeFull);
+  let widgetOptions = {...(options.widgetOptions ?? {})};
+  const tableData: TableData = docModel.docData.getTable(origCol.table().tableId())!;
+  const visibleCol = origCol.visibleColModel();
+  const sourceCol = visibleCol.getRowId() !== 0 ? visibleCol : origCol;
  switch (toType) {
    case 'Bool':
      // Most types use a TextBox as the default widget.
@ -162,9 +234,9 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe
        // trouble than desired behavior. For many choices, recommend using a Ref to helper table.
        const columnData = tableData.getDistinctValues(sourceCol.colId(), 100);
        if (columnData) {
-          const choices = Array.from(columnData, String).filter((choice) => {
-            return choice !== null && choice.trim() !== '';
-          });
+          const choices = Array.from(columnData).filter(isNonNullish)
+                                                .map(v => String(v).trim())
+                                                .filter(Boolean);
          widgetOptions = {...widgetOptions, choices};
        }
      }
@ -181,7 +253,7 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe
          value = String(decodeObject(value)).trim();
          const tags: unknown[] = (value.startsWith('[') && gutil.safeJsonParse(value, null)) || csvDecodeRow(value);
          for (const tag of tags) {
-            const choice = String(tag).trim();
+            const choice = !tag ? '' : String(tag).trim();
            if (choice === '') { continue; }
            choices.add(choice);
            if (choices.size > 100) { break; }    // Don't suggest excessively many choices.
@ -191,52 +263,11 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe
      }
      break;
    }
-    case 'Ref':
-    case 'RefList':
-    {
-      // Set suggested destination table and visible column.
-      // Undefined if toTypeMaybeFull is a pure type (e.g. converting to Ref before a table is chosen).
-      const optTableId = gutil.removePrefix(toTypeMaybeFull, `${toType}:`) || undefined;
-
-      let suggestedColRef: number;
-      let suggestedTableId: string;
-      const origColTypeInfo = gristTypes.extractInfoFromColType(origCol.type.peek());
-      if (!optTableId && (origColTypeInfo.type === "Ref" || origColTypeInfo.type === "RefList")) {
-        // When converting between Ref and Reflist, initially suggest the same table and visible column.
-        // When converting, if the table is the same, it's a special case.
-        // The visible column will not affect conversion.
-        // It will simply wrap the reference (row ID) in a list or extract the one element of a reference list.
-        suggestedColRef = origCol.visibleCol.peek();
-        suggestedTableId = origColTypeInfo.tableId;
-      } else {
-        // Finds a reference suggestion column and sets it as the current reference value.
-        const columnData = tableData.getDistinctValues(origDisplayCol.colId(), 100);
-        if (!columnData) { break; }
-        columnData.delete(gristTypes.getDefaultForType(origCol.type()));
-
-        // 'findColFromValues' function requires an array since it sends the values to the sandbox.
-        const matches: number[] = await docModel.docData.findColFromValues(Array.from(columnData), 2, optTableId);
-        suggestedColRef = matches.find(match => match !== origCol.getRowId())!;
-        if (!suggestedColRef) { break; }
-        const suggestedCol = docModel.columns.getRowModel(suggestedColRef);
-        suggestedTableId = suggestedCol.table().tableId();
-        if (optTableId && suggestedTableId !== optTableId) {
-          console.warn("Inappropriate column received from findColFromValues");
-          break;
-        }
-      }
-      colInfo.type = `${toType}:${suggestedTableId}`;
-      colInfo.visibleCol = suggestedColRef;
-      break;
-    }
  }
-
-  if (Object.keys(widgetOptions).length) {
-    colInfo.widgetOptions = JSON.stringify(widgetOptions);
-  }
-  return colInfo;
+  return widgetOptions;
 }

+
 // Given the transformCol, calls (if needed) a user action to update its displayCol.
 export async function setDisplayFormula(
  docModel: DocModel, transformCol: ColumnRec, visibleCol?: number
--- a/app/client/components/TypeTransform.ts
+++ b/app/client/components/TypeTransform.ts
@ -100,9 +100,13 @@ export class TypeTransform extends ColumnTransform {
    const gristHelper_TransformRef = newColInfos[1].colRef;
    this.transformColumn = docModel.columns.getRowModel(gristHelper_TransformRef);
    this._convertColumn = docModel.columns.getRowModel(gristHelper_ConvertedRef);
-    const colInfo = await TypeConversion.prepTransformColInfo(
-      docModel, this.origColumn,
-      this.origDisplayCol, toType, this._convertColumn.colId.peek());
+    const colInfo = await TypeConversion.prepTransformColInfo({
+      docModel,
+      origCol: this.origColumn,
+      origDisplayCol: this.origDisplayCol,
+      toTypeMaybeFull: toType,
+      convertedRef: this._convertColumn.colId.peek()
+    });
    // NOTE: We could add rules with AddColumn action, but there are some optimizations that converts array values.
    const rules = colInfo.rules;
    delete (colInfo as any).rules;
@ -165,9 +169,13 @@ export class TypeTransform extends ColumnTransform {
   */
  public async setType(toType: string) {
    const docModel = this.gristDoc.docModel;
-    const colInfo = await TypeConversion.prepTransformColInfo(
-      docModel, this.origColumn, this.origDisplayCol,
-      toType, this._convertColumn.colId.peek());
+    const colInfo = await TypeConversion.prepTransformColInfo({
+      docModel,
+      origCol: this.origColumn,
+      origDisplayCol: this.origDisplayCol,
+      toTypeMaybeFull: toType,
+      convertedRef: this._convertColumn.colId.peek()
+    });
    const tcol = this.transformColumn;
    await tcol.updateColValues(colInfo as any);
  }
--- a/app/client/widgets/FieldBuilder.ts
+++ b/app/client/widgets/FieldBuilder.ts
@ -2,7 +2,7 @@ import { ColumnTransform } from 'app/client/components/ColumnTransform';
 import { Cursor } from 'app/client/components/Cursor';
 import { FormulaTransform } from 'app/client/components/FormulaTransform';
 import { GristDoc } from 'app/client/components/GristDoc';
-import { addColTypeSuffix } from 'app/client/components/TypeConversion';
+import { addColTypeSuffix, guessWidgetOptionsSync } from 'app/client/components/TypeConversion';
 import { TypeTransform } from 'app/client/components/TypeTransform';
 import { FloatingEditor } from 'app/client/widgets/FloatingEditor';
 import { UnsavedChange } from 'app/client/components/UnsavedChanges';
@ -36,8 +36,9 @@ import * as UserTypeImpl from 'app/client/widgets/UserTypeImpl';
 import * as gristTypes from 'app/common/gristTypes';
 import { getReferencedTableId, isFullReferencingType } from 'app/common/gristTypes';
 import { CellValue } from 'app/plugin/GristData';
-import { Computed, Disposable, fromKo, dom as grainjsDom,
-         makeTestId, MultiHolder, Observable, styled, toKo } from 'grainjs';
+import { bundleChanges, Computed, Disposable, fromKo,
+         dom as grainjsDom, makeTestId, MultiHolder, Observable, styled, toKo } from 'grainjs';
+import isEqual from 'lodash/isEqual';
 import * as ko from 'knockout';
 import * as _ from 'underscore';

@ -160,9 +161,9 @@ export class FieldBuilder extends Disposable {
      write: val => {
        const type = this.field.column().type();
        if (type.startsWith('Ref:')) {
-          void this._setType(`Ref:${val}`);
+          this._setType(`Ref:${val}`);
        } else {
-          void this._setType(`RefList:${val}`);
+          this._setType(`RefList:${val}`);
        }
      }
    }));
@ -331,28 +332,57 @@ export class FieldBuilder extends Disposable {
  }

  // Helper function to set the column type to newType.
-  public _setType(newType: string): Promise<unknown>|undefined {
+  public _setType(newType: string): void {
+    // If the original column is a formula, we won't be showing any transform UI, so we can
+    // just set the type directly. We test original column as this field might be in the middle
+    // of transformation and temporary be connected to a helper column (but formula columns are
+    // never transformed using UI).
    if (this.origColumn.isFormula()) {
      // Do not type transform a new/empty column or a formula column. Just make a best guess for
      // the full type, and set it. If multiple columns are selected (and all are formulas/empty),
      // then we will set the type for all of them using full type guessed from the first column.
-      const column = this.field.column();
+      const column = this.field.column(); // same as this.origColumn.
      const calculatedType = addColTypeSuffix(newType, column, this._docModel);
+      const fields = this.field.viewSection.peek().selectedFields.peek();
      // If we selected multiple empty/formula columns, make the change for all of them.
-      if (this.field.viewSection.peek().selectedFields.peek().length > 1 &&
-          ['formula', 'empty'].indexOf(this.field.viewSection.peek().columnsBehavior.peek())) {
-        return this.gristDoc.docData.bundleActions(t("Changing multiple column types"), () =>
+      if (
+        fields.length > 1 &&
+        fields.every(f => f.column.peek().isFormula() || f.column.peek().isEmpty())
+      ) {
+        this.gristDoc.docData.bundleActions(t("Changing multiple column types"), () =>
          Promise.all(this.field.viewSection.peek().selectedFields.peek().map(f =>
            f.column.peek().type.setAndSave(calculatedType)
        ))).catch(reportError);
+      } else if (column.pureType() === 'Any') {
+        // If this is Any column, guess the final options.
+        const guessedOptions = guessWidgetOptionsSync({
+          docModel: this._docModel,
+          origCol: this.origColumn,
+          toTypeMaybeFull: newType,
+        });
+        const existingOptions = column.widgetOptionsJson.peek();
+        const widgetOptions = JSON.stringify({...existingOptions, ...guessedOptions});
+        bundleChanges(() => {
+          this.gristDoc.docData.bundleActions(t("Changing column type"), () =>
+            Promise.all([
+              // This order is better for any other UI modifications, as first we are updating options
+              // and then saving type.
+              !isEqual(existingOptions, guessedOptions)
+                ? column.widgetOptions.setAndSave(widgetOptions)
+                : Promise.resolve(),
+                column.type.setAndSave(calculatedType),
+            ])
+          ).catch(reportError);
+        });
+      } else {
+        column.type.setAndSave(calculatedType).catch(reportError);
      }
-      column.type.setAndSave(calculatedType).catch(reportError);
    } else if (!this.columnTransform) {
      this.columnTransform = TypeTransform.create(null, this.gristDoc, this);
-      return this.columnTransform.prepare(newType);
+      this.columnTransform.prepare(newType).catch(reportError);
    } else {
      if (this.columnTransform instanceof TypeTransform) {
-        return this.columnTransform.setType(newType);
+        this.columnTransform.setType(newType).catch(reportError);
      }
    }
  }
--- a/test/nbrowser/gristUtils.ts
+++ b/test/nbrowser/gristUtils.ts
@ -12,6 +12,7 @@ import { stackWrapFunc, stackWrapOwnMethods, WebDriver } from 'mocha-webdriver';
 import * as path from 'path';
 import * as PluginApi from 'app/plugin/grist-plugin-api';

+import {CommandName} from 'app/client/components/commandList';
 import {csvDecodeRow} from 'app/common/csvFormat';
 import { AccessLevel } from 'app/common/CustomWidget';
 import { decodeUrl } from 'app/common/gristUrls';
@ -3413,6 +3414,21 @@ class Clipboard implements IClipboard {
  }
 }

+/**
+ * Runs a Grist command in the browser window.
+ */
+export async function sendCommand(name: CommandName) {
+  await driver.executeAsyncScript((name: any, done: any) => {
+    const result = (window as any).gristApp.allCommands[name].run();
+    if (result?.finally) {
+      result.finally(done);
+    } else {
+      done();
+    }
+  }, name);
+  await waitForServer();
+}
+

 } // end of namespace gristUtils