From 77775401fc8cf5fbb779fa551f72ae940ee4a9a2 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Thu, 7 Jul 2022 14:58:37 +0200 Subject: [PATCH] (core) Don't clear widget options when changing column type Summary: Previously, changing the type of a column would clear its widget options and conditional style rules by default, with a few exceptions to explicitly keep them. This diff reverses that behaviour, keeping the options by default. Test Plan: Updated several existing tests, plus lots of manual testing. Reviewers: cyprien Reviewed By: cyprien Subscribers: dsagal Differential Revision: https://phab.getgrist.com/D3491 --- app/client/components/TypeConversion.ts | 77 +++++++++++++++---------- sandbox/grist/test_useractions.py | 1 - sandbox/grist/useractions.py | 9 +-- 3 files changed, 51 insertions(+), 36 deletions(-) diff --git a/app/client/components/TypeConversion.ts b/app/client/components/TypeConversion.ts index 16a4a68d..a268fff7 100644 --- a/app/client/components/TypeConversion.ts +++ b/app/client/components/TypeConversion.ts @@ -11,7 +11,7 @@ import * as gristTypes from 'app/common/gristTypes'; import {isFullReferencingType} from 'app/common/gristTypes'; import * as gutil from 'app/common/gutil'; import NumberParse from 'app/common/NumberParse'; -import {dateTimeWidgetOptions, guessDateFormat} from 'app/common/parseDate'; +import {dateTimeWidgetOptions, guessDateFormat, timeFormatOptions} from 'app/common/parseDate'; import {TableData} from 'app/common/TableData'; import {decodeObject} from 'app/plugin/objtypes'; @@ -87,31 +87,60 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe toTypeMaybeFull: string): Promise { const toType = gristTypes.extractTypeFromColType(toTypeMaybeFull); const tableData: TableData = docModel.docData.getTable(origCol.table().tableId())!; - let widgetOptions: any = null; - let rules: gristTypes.RefListValue = null; + + const visibleCol = origCol.visibleColModel(); + // Column used to derive previous widget options and sample values for guessing + const sourceCol = visibleCol.getRowId() !== 0 ? visibleCol : origCol; + let widgetOptions = {...(sourceCol.widgetOptionsJson() || {})}; + + if (isReferenceCol(origCol)) { + // While reference columns copy most options from their visible column, conditional style rules are kept + // from the original reference column for a few reasons: + // 1. The rule formula of the visible column is less likely to make sense after conversion, + // especially if the reference points to a different table. + // 2. Overwriting the conditional styles of the original reference column could be annoying, whereas + // most other widget options in reference columns aren't particularly valuable. + // 3. The `rules` column (i.e. a reflist to other formula columns) can't simply be copied because those rule columns + // can't currently be shared by multiple columns. + // So in general we keep `rules: origCol.rules()` (further below) and the corresponding + // `widgetOptions.rulesOptions`. + // A quirk of this is that the default (non-conditional) cell style can still be copied from the visible column, + // so a subset of the overall cell styling can change. + delete widgetOptions.rulesOptions; + const {rulesOptions} = origCol.widgetOptionsJson() || {}; + if (rulesOptions) { + widgetOptions.rulesOptions = rulesOptions; + } + } const colInfo: ColInfo = { type: addColTypeSuffix(toTypeMaybeFull, origCol, docModel), isFormula: true, visibleCol: 0, formula: "CURRENT_CONVERSION(rec)", - rules, + rules: origCol.rules(), }; - const visibleCol = origCol.visibleColModel(); - // Column used to derive previous widget options and sample values for guessing - const sourceCol = visibleCol.getRowId() !== 0 ? visibleCol : origCol; - const prevOptions = sourceCol.widgetOptionsJson.peek() || {}; - const prevRules = sourceCol.rules.peek(); switch (toType) { + case 'Bool': + // Most types use a TextBox as the default widget. + // We don't want to reuse that for Toggle columns, which should be a CheckBox by default. + delete widgetOptions.widget; + break; case 'Date': case 'DateTime': { - let {dateFormat} = prevOptions; + let {dateFormat} = widgetOptions; if (!dateFormat) { + // Guess date and time format if there aren't any already const colValues = tableData.getColValues(sourceCol.colId()) || []; dateFormat = guessDateFormat(colValues.map(String)); + widgetOptions = {...widgetOptions, ...(dateTimeWidgetOptions(dateFormat, true))}; + } + if (toType === 'DateTime' && !widgetOptions.timeFormat) { + // Ensure DateTime columns have a time format. This is needed when converting from a Date column. + widgetOptions.timeFormat = timeFormatOptions[0]; + widgetOptions.isCustomTimeFormat = false; } - widgetOptions = dateTimeWidgetOptions(dateFormat, true); break; } case 'Numeric': @@ -119,34 +148,27 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe if (!["Numeric", "Int"].includes(sourceCol.type())) { const numberParse = NumberParse.fromSettings(docModel.docData.docSettings()); const colValues = tableData.getColValues(sourceCol.colId()) || []; - widgetOptions = numberParse.guessOptions(colValues.filter(isString)); - } else { - widgetOptions = prevOptions; - rules = prevRules; + widgetOptions = {...widgetOptions, ...numberParse.guessOptions(colValues.filter(isString))}; } break; } case 'Choice': { - if (Array.isArray(prevOptions.choices)) { - // Use previous choices if they are set, e.g. if converting from ChoiceList - widgetOptions = {choices: prevOptions.choices, choiceOptions: prevOptions.choiceOptions}; - } else { + // Use previous choices if they are set, e.g. if converting from ChoiceList + if (!Array.isArray(widgetOptions.choices)) { // Set suggested choices. Limit to 100, since too many choices is more likely to cause // trouble than desired behavior. For many choices, recommend using a Ref to helper table. const columnData = tableData.getDistinctValues(sourceCol.colId(), 100); if (columnData) { columnData.delete(""); columnData.delete(null); - widgetOptions = {choices: Array.from(columnData, String)}; + widgetOptions = {...widgetOptions, choices: Array.from(columnData, String)}; } } break; } case 'ChoiceList': { - if (Array.isArray(prevOptions.choices)) { - // Use previous choices if they are set, e.g. if converting from ChoiceList - widgetOptions = {choices: prevOptions.choices, choiceOptions: prevOptions.choiceOptions}; - } else { + // Use previous choices if they are set, e.g. if converting from Choice + if (!Array.isArray(widgetOptions.choices)) { // Set suggested choices. This happens before the conversion to ChoiceList, so we do some // light guessing for likely choices to suggest. const choices = new Set(); @@ -160,7 +182,7 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe } } choices.delete(""); - widgetOptions = {choices: Array.from(choices)}; + widgetOptions = {...widgetOptions, choices: Array.from(choices)}; } break; } @@ -204,12 +226,9 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe } } - if (widgetOptions) { + if (Object.keys(widgetOptions).length) { colInfo.widgetOptions = JSON.stringify(widgetOptions); } - if (rules) { - colInfo.rules = rules; - } return colInfo; } diff --git a/sandbox/grist/test_useractions.py b/sandbox/grist/test_useractions.py index f2460349..f5af53e1 100644 --- a/sandbox/grist/test_useractions.py +++ b/sandbox/grist/test_useractions.py @@ -104,7 +104,6 @@ class TestUserActions(test_engine.EngineTestCase): self.assertPartialOutActions(out_actions, { "stored": [ ['ModifyColumn', 'Schools', 'city', {'type': 'Ref:Address'}], ['UpdateRecord', 'Schools', 4, {'city': 0}], - ['UpdateRecord', '_grist_Views_section_field', 1, {'widgetOptions': ''}], ['UpdateRecord', '_grist_Tables_column', 23, { 'type': 'Ref:Address', 'widgetOptions': 'world' }], diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index 6cd9316a..a7780d6f 100644 --- a/sandbox/grist/useractions.py +++ b/sandbox/grist/useractions.py @@ -705,11 +705,10 @@ class UserActions(object): if has_diff_value(values, 'colId', c.colId): self._do_doc_action(actions.RenameColumn(c.parentId.tableId, c.colId, values['colId'])) - # If we change a column's type, we should ALSO unset each affected field's widgetOptions and - # displayCol. + # If we change a column's type, we should ALSO unset each affected field's displayCol. type_changed = [c for c, values in update_pairs if has_diff_value(values, 'type', c.type)] self._docmodel.update([f for c in type_changed for f in c.viewFields], - widgetOptions='', displayCol=0) + displayCol=0) self.doBulkUpdateFromPairs(table_id, update_pairs) @@ -820,10 +819,8 @@ class UserActions(object): # Look at the actual data for that column (first 1000 values) to decide on the type. col_values['type'] = guess_type(self._get_column_values(col), convert=False) - # If changing the type of a column, unset its widgetOptions, displayCol and rules by default. + # If changing the type of a column, unset its displayCol by default. if 'type' in col_values: - col_values.setdefault('widgetOptions', '') - col_values.setdefault('rules', None) col_values.setdefault('displayCol', 0) # Collect all updates for dependent summary columns.