From c470c4041bce0db1cc3ce3b6bde3e26f3f3a5da2 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Thu, 16 Dec 2021 00:31:53 +0200 Subject: [PATCH] (core) Use visibleCol instead of displayCol with createFormatter Summary: Some things (like rendering cells) use the `visibleCol` for `createFormatter`, while other things (like `CopySelection`) used the `displayCol`. For references, the display column has type Any and doesn't know about the original formatting. This resulted in formatting being lost when copying from reference columns even though formatting was preserved when copying from the original (visible) column which looked identical. This diff fixes this and ensures that `createFormatter` is always used with the `visibleCol`. This was agreed on in https://grist.slack.com/archives/C0234CPPXPA/p1639571321043000 Additionally: - Replaces the functions `createVisibleColFormatter` computed properties `visibleColFormatter` as suggested by a `TODO`. - Extracts common code from `createVisibleColFormatter` in `ColumnRec` and `ViewFieldRec` Test Plan: Fixed a test in CopyPaste which displayed the previous inconsistent behaviour. Reviewers: jarek Reviewed By: jarek Differential Revision: https://phab.getgrist.com/D3189 --- app/client/components/ChartView.ts | 2 +- app/client/components/CopySelection.ts | 9 ++----- app/client/lib/ReferenceUtils.ts | 2 +- app/client/models/ColumnACIndexes.ts | 2 +- app/client/models/SearchModel.ts | 5 ++-- app/client/models/entities/ColumnRec.ts | 28 ++++++++++++---------- app/client/models/entities/ViewFieldRec.ts | 20 ++++++---------- app/client/ui/ColumnFilterMenu.ts | 2 +- app/client/widgets/AbstractWidget.js | 5 +--- app/client/widgets/NewAbstractWidget.ts | 9 ++----- app/client/widgets/Reference.ts | 8 +------ 11 files changed, 34 insertions(+), 58 deletions(-) diff --git a/app/client/components/ChartView.ts b/app/client/components/ChartView.ts index 3145df13..c40ae0d0 100644 --- a/app/client/components/ChartView.ts +++ b/app/client/components/ChartView.ts @@ -164,7 +164,7 @@ export class ChartView extends Disposable { // within a donut chart. this._formatterComp = this.autoDispose(ko.computed(() => { const field = this.viewSection.viewFields().at(1); - return field?.createVisibleColFormatter(); + return field?.visibleColFormatter(); })); this._update = debounce(() => this._updateView(), 0); diff --git a/app/client/components/CopySelection.ts b/app/client/components/CopySelection.ts index 1b6ceedf..66239b5f 100644 --- a/app/client/components/CopySelection.ts +++ b/app/client/components/CopySelection.ts @@ -1,8 +1,7 @@ +import type {ViewFieldRec} from 'app/client/models/entities/ViewFieldRec'; import type {CellValue} from 'app/common/DocActions'; import type {TableData} from 'app/common/TableData'; import type {UIRowId} from 'app/common/UIRowId'; -import {createFormatter} from 'app/common/ValueFormatter'; -import type {ViewFieldRec} from 'app/client/models/entities/ViewFieldRec'; /** * The CopySelection class is an abstraction for a subset of currently selected cells. @@ -34,11 +33,7 @@ export class CopySelection { this.rowStyle = options.rowStyle; this.colStyle = options.colStyle; this.columns = fields.map((f, i) => { - const formatter = createFormatter( - f.displayColModel().type(), - f.widgetOptionsJson(), - f.documentSettings() - ); + const formatter = f.visibleColFormatter(); const _fmtGetter = tableData.getRowPropFunc(this.displayColIds[i])!; const _rawGetter = tableData.getRowPropFunc(this.colIds[i])!; diff --git a/app/client/lib/ReferenceUtils.ts b/app/client/lib/ReferenceUtils.ts index 84410fd4..cafbbaf5 100644 --- a/app/client/lib/ReferenceUtils.ts +++ b/app/client/lib/ReferenceUtils.ts @@ -30,7 +30,7 @@ export class ReferenceUtils { } this.tableData = tableData; - this.formatter = field.createVisibleColFormatter(); + this.formatter = field.visibleColFormatter(); this.visibleColModel = field.visibleColModel(); this.visibleColId = this.visibleColModel.colId() || 'id'; this.isRefList = isRefListType(colType); diff --git a/app/client/models/ColumnACIndexes.ts b/app/client/models/ColumnACIndexes.ts index 26d5ff1a..4020e7db 100644 --- a/app/client/models/ColumnACIndexes.ts +++ b/app/client/models/ColumnACIndexes.ts @@ -29,7 +29,7 @@ export class ColumnACIndexes { /** * Returns the column index for the given column, using a cached one if available. - * The formatter should be created using field.createVisibleColFormatter(). It's assumed that + * The formatter should be created using field.visibleColFormatter(). It's assumed that * getColACIndex() is called for the same column with the the same formatter. */ public getColACIndex(colId: string, formatter: BaseFormatter): ACIndex { diff --git a/app/client/models/SearchModel.ts b/app/client/models/SearchModel.ts index fc9305a8..0cefb235 100644 --- a/app/client/models/SearchModel.ts +++ b/app/client/models/SearchModel.ts @@ -8,7 +8,7 @@ import {reportError} from 'app/client/models/errors'; import {delay} from 'app/common/delay'; import {waitObs} from 'app/common/gutil'; import {TableData} from 'app/common/TableData'; -import {BaseFormatter, createFormatter} from 'app/common/ValueFormatter'; +import {BaseFormatter} from 'app/common/ValueFormatter'; import {Disposable, Observable} from 'grainjs'; import debounce = require('lodash/debounce'); @@ -218,8 +218,7 @@ class FinderImpl implements IFinder { this._sectionTableData = tableModel.tableData; this._fieldStepper.array = section.viewFields().peek(); - this._fieldFormatters = this._fieldStepper.array.map( - f => createFormatter(f.displayColModel().type(), f.widgetOptionsJson(), f.documentSettings())); + this._fieldFormatters = this._fieldStepper.array.map(f => f.visibleColFormatter.peek()); return tableModel; } diff --git a/app/client/models/entities/ColumnRec.ts b/app/client/models/entities/ColumnRec.ts index 42b4fc80..9439be3f 100644 --- a/app/client/models/entities/ColumnRec.ts +++ b/app/client/models/entities/ColumnRec.ts @@ -52,12 +52,12 @@ export interface ColumnRec extends IRowModel<"_grist_Tables_column"> { // Returns the rowModel for the referenced table, or null, if is not a reference column. refTable: ko.Computed; + // Helper for Reference/ReferenceList columns, which returns a formatter according + // to the visibleCol associated with column. + visibleColFormatter: ko.Computed; + // Helper which adds/removes/updates column's displayCol to match the formula. saveDisplayFormula(formula: string): Promise|undefined; - - // Helper for Reference/ReferenceList columns, which returns a formatter according - // to the visibleCol associated with column. Subscribes to observables if used within a computed. - createVisibleColFormatter(): BaseFormatter; } export function createColumnRec(this: ColumnRec, docModel: DocModel): void { @@ -117,13 +117,15 @@ export function createColumnRec(this: ColumnRec, docModel: DocModel): void { // Helper for Reference/ReferenceList columns, which returns a formatter according to the visibleCol // associated with this column. If no visible column available, return formatting for the column itself. - // Subscribes to observables if used within a computed. - // TODO: It would be better to replace this with a pureComputed whose value is a formatter. - this.createVisibleColFormatter = function() { - const vcol = this.visibleColModel(); - const documentSettings = docModel.docInfoRow.documentSettingsJson(); - return (vcol.getRowId() !== 0) ? - createFormatter(vcol.type(), vcol.widgetOptionsJson(), documentSettings) : - createFormatter(this.type(), this.widgetOptionsJson(), documentSettings); - }; + this.visibleColFormatter = ko.pureComputed(() => visibleColFormatterForRec(this, this, docModel)); +} + +export function visibleColFormatterForRec( + rec: ColumnRec | ViewFieldRec, colRec: ColumnRec, docModel: DocModel +): BaseFormatter { + const vcol = rec.visibleColModel(); + const documentSettings = docModel.docInfoRow.documentSettingsJson(); + return (vcol.getRowId() !== 0) ? + createFormatter(vcol.type(), vcol.widgetOptionsJson(), documentSettings) : + createFormatter(colRec.type(), rec.widgetOptionsJson(), documentSettings); } diff --git a/app/client/models/entities/ViewFieldRec.ts b/app/client/models/entities/ViewFieldRec.ts index 5772e3ae..8ecc4123 100644 --- a/app/client/models/entities/ViewFieldRec.ts +++ b/app/client/models/entities/ViewFieldRec.ts @@ -1,8 +1,9 @@ import {ColumnRec, DocModel, IRowModel, refRecord, ViewSectionRec} from 'app/client/models/DocModel'; +import {visibleColFormatterForRec} from 'app/client/models/entities/ColumnRec'; import * as modelUtil from 'app/client/models/modelUtil'; import * as UserType from 'app/client/widgets/UserType'; import {DocumentSettings} from 'app/common/DocumentSettings'; -import {BaseFormatter, createFormatter} from 'app/common/ValueFormatter'; +import {BaseFormatter} from 'app/common/ValueFormatter'; import {createParser} from 'app/common/ValueParser'; import * as ko from 'knockout'; @@ -69,15 +70,15 @@ export interface ViewFieldRec extends IRowModel<"_grist_Views_section_field"> { documentSettings: ko.PureComputed; + // Helper for Reference/ReferenceList columns, which returns a formatter according + // to the visibleCol associated with field. + visibleColFormatter: ko.Computed; + createValueParser(): (value: string) => any; // Helper which adds/removes/updates field's displayCol to match the formula. saveDisplayFormula(formula: string): Promise|undefined; - // Helper for Reference/ReferenceList columns, which returns a formatter according - // to the visibleCol associated with field. Subscribes to observables if used within a computed. - createVisibleColFormatter(): BaseFormatter; - // Helper for Choice/ChoiceList columns, that saves widget options and renames values in a document // in one bundle updateChoices(renameMap: Record, options: any): Promise; @@ -164,14 +165,7 @@ export function createViewFieldRec(this: ViewFieldRec, docModel: DocModel): void // Helper for Reference/ReferenceList columns, which returns a formatter according to the visibleCol // associated with this field. If no visible column available, return formatting for the field itself. - // Subscribes to observables if used within a computed. - // TODO: It would be better to replace this with a pureComputed whose value is a formatter. - this.createVisibleColFormatter = function() { - const vcol = this.visibleColModel(); - return (vcol.getRowId() !== 0) ? - createFormatter(vcol.type(), vcol.widgetOptionsJson(), this.documentSettings()) : - createFormatter(this.column().type(), this.widgetOptionsJson(), this.documentSettings()); - }; + this.visibleColFormatter = ko.pureComputed(() => visibleColFormatterForRec(this, this.column(), docModel)); this.createValueParser = function() { const fieldRef = this.useColOptions.peek() ? undefined : this.id.peek(); diff --git a/app/client/ui/ColumnFilterMenu.ts b/app/client/ui/ColumnFilterMenu.ts index 608b3096..6a75da53 100644 --- a/app/client/ui/ColumnFilterMenu.ts +++ b/app/client/ui/ColumnFilterMenu.ts @@ -338,7 +338,7 @@ export function createFilterMenu(openCtl: IOpenController, sectionFilter: Sectio function getMapFuncs(columnType: string, tableData: TableData, fieldOrColumn: ViewFieldRec|ColumnRec) { const keyMapFunc = tableData.getRowPropFunc(fieldOrColumn.colId())!; const labelGetter = tableData.getRowPropFunc(fieldOrColumn.displayColModel().colId())!; - const formatter = fieldOrColumn.createVisibleColFormatter(); + const formatter = fieldOrColumn.visibleColFormatter(); let labelMapFunc: (rowId: number) => string | string[]; if (isRefListType(columnType)) { diff --git a/app/client/widgets/AbstractWidget.js b/app/client/widgets/AbstractWidget.js index 8e928dc7..8577531e 100644 --- a/app/client/widgets/AbstractWidget.js +++ b/app/client/widgets/AbstractWidget.js @@ -1,7 +1,5 @@ var dispose = require('../lib/dispose'); -const ko = require('knockout'); const {Computed, fromKo} = require('grainjs'); -const ValueFormatter = require('app/common/ValueFormatter'); const {cssLabel, cssRow} = require('app/client/ui/RightPanel'); const {colorSelect} = require('app/client/ui2018/ColorSelect'); @@ -17,8 +15,7 @@ function AbstractWidget(field, opts = {}) { this.options = field.widgetOptionsJson; const {defaultTextColor = '#000000'} = opts; - this.valueFormatter = this.autoDispose(ko.computed(() => - ValueFormatter.createFormatter(field.displayColModel().type(), this.options(), field.documentSettings()))); + this.valueFormatter = this.field.visibleColFormatter; this.textColor = Computed.create(this, (use) => use(this.field.textColor) || defaultTextColor) .onWrite((val) => this.field.textColor(val === defaultTextColor ? undefined : val)); diff --git a/app/client/widgets/NewAbstractWidget.ts b/app/client/widgets/NewAbstractWidget.ts index 85950f5c..820339ec 100644 --- a/app/client/widgets/NewAbstractWidget.ts +++ b/app/client/widgets/NewAbstractWidget.ts @@ -8,9 +8,8 @@ import {ViewFieldRec} from 'app/client/models/entities/ViewFieldRec'; import {SaveableObjObservable} from 'app/client/models/modelUtil'; import {cssLabel, cssRow} from 'app/client/ui/RightPanel'; import {colorSelect} from 'app/client/ui2018/ColorSelect'; -import {BaseFormatter, createFormatter} from 'app/common/ValueFormatter'; +import {BaseFormatter} from 'app/common/ValueFormatter'; import {Computed, Disposable, DomContents, fromKo, Observable} from 'grainjs'; -import * as ko from 'knockout'; export interface Options { @@ -44,11 +43,7 @@ export abstract class NewAbstractWidget extends Disposable { )).onWrite((val) => this.field.textColor(val === defaultTextColor ? undefined : val)); this.fillColor = fromKo(this.field.fillColor); - // Note that its easier to create a knockout computed from the several knockout observables, - // but then we turn it into a grainjs observable. - const formatter = this.autoDispose(ko.computed(() => - createFormatter(field.displayColModel().type(), this.options(), field.documentSettings()))); - this.valueFormatter = fromKo(formatter); + this.valueFormatter = fromKo(field.visibleColFormatter); } /** diff --git a/app/client/widgets/Reference.ts b/app/client/widgets/Reference.ts index 25909d66..c0336233 100644 --- a/app/client/widgets/Reference.ts +++ b/app/client/widgets/Reference.ts @@ -6,9 +6,7 @@ import {icon} from 'app/client/ui2018/icons'; import {IOptionFull, select} from 'app/client/ui2018/menus'; import {NTextBox} from 'app/client/widgets/NTextBox'; import {isFullReferencingType, isVersions} from 'app/common/gristTypes'; -import {BaseFormatter} from 'app/common/ValueFormatter'; import {Computed, dom, styled} from 'grainjs'; -import * as ko from 'knockout'; /** * Reference - The widget for displaying references to another table's records. @@ -16,16 +14,12 @@ import * as ko from 'knockout'; export class Reference extends NTextBox { protected _formatValue: Computed<(val: any) => string>; - private _refValueFormatter: ko.Computed; private _visibleColRef: Computed; private _validCols: Computed>>; constructor(field: ViewFieldRec) { super(field); - this._refValueFormatter = this.autoDispose(ko.computed(() => - field.createVisibleColFormatter())); - this._visibleColRef = Computed.create(this, (use) => use(this.field.visibleColRef)); // Note that saveOnly is used here to prevent display value flickering on visible col change. this._visibleColRef.onWrite((val) => this.field.visibleColRef.saveOnly(val)); @@ -48,7 +42,7 @@ export class Reference extends NTextBox { this._formatValue = Computed.create(this, (use) => { // If the field is pulling values from a display column, use a general-purpose formatter. if (use(this.field.displayColRef) !== use(this.field.colRef)) { - const fmt = use(this._refValueFormatter); + const fmt = use(this.field.visibleColFormatter); return (val: any) => fmt.formatAny(val); } else { const refTable = use(use(this.field.column).refTable);