From c7ba31eb7d9a0c9ff3327f3e80392062f091f26b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Mon, 30 Oct 2023 11:40:28 +0100 Subject: [PATCH] (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 --- app/client/components/TypeConversion.ts | 135 +++++++++++++++--------- app/client/components/TypeTransform.ts | 20 ++-- app/client/widgets/FieldBuilder.ts | 56 +++++++--- test/nbrowser/gristUtils.ts | 16 +++ 4 files changed, 156 insertions(+), 71 deletions(-) diff --git a/app/client/components/TypeConversion.ts b/app/client/components/TypeConversion.ts index eb893d34..099cbb3f 100644 --- 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 { +export async function prepTransformColInfo(options: { + docModel: DocModel; + origCol: ColumnRec; + origDisplayCol: ColumnRec; + toTypeMaybeFull: string; + convertedRef?: string +}): Promise { + 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 diff --git a/app/client/components/TypeTransform.ts b/app/client/components/TypeTransform.ts index 3fd5e217..1e1d04b8 100644 --- 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); } diff --git a/app/client/widgets/FieldBuilder.ts b/app/client/widgets/FieldBuilder.ts index 00dff0f3..eab4b46b 100644 --- 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|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); } } } diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index e1d684a1..1f22cc48 100644 --- 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