(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

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

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(
+      const formatter = f.visibleColFormatter();
-        f.displayColModel().type(),
-        f.widgetOptionsJson(),
-        f.documentSettings()
-      );
       const _fmtGetter = tableData.getRowPropFunc(this.displayColIds[i])!;
       const _rawGetter = tableData.getRowPropFunc(this.colIds[i])!;
 

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

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<ICellItem> {

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(
+    this._fieldFormatters = this._fieldStepper.array.map(f => f.visibleColFormatter.peek());
-      f => createFormatter(f.displayColModel().type(), f.widgetOptionsJson(), f.documentSettings()));
     return tableModel;
   }
 

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<TableRec|null>;
 
+  // Helper for Reference/ReferenceList columns, which returns a formatter according
+  // to the visibleCol associated with column.
+  visibleColFormatter: ko.Computed<BaseFormatter>;
+
   // Helper which adds/removes/updates column's displayCol to match the formula.
   saveDisplayFormula(formula: string): Promise<void>|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.
+  this.visibleColFormatter = ko.pureComputed(() => visibleColFormatterForRec(this, this, docModel));
-  // TODO: It would be better to replace this with a pureComputed whose value is a formatter.
+}
-  this.createVisibleColFormatter = function() {
+
-    const vcol = this.visibleColModel();
+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(this.type(), this.widgetOptionsJson(), documentSettings);
+    createFormatter(colRec.type(), rec.widgetOptionsJson(), documentSettings);
-  };
 }

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<DocumentSettings>;
 
+  // Helper for Reference/ReferenceList columns, which returns a formatter according
+  // to the visibleCol associated with field.
+  visibleColFormatter: ko.Computed<BaseFormatter>;
+
   createValueParser(): (value: string) => any;
 
   // Helper which adds/removes/updates field's displayCol to match the formula.
   saveDisplayFormula(formula: string): Promise<void>|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<string, string>, options: any): Promise<void>;
@@ -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.
+  this.visibleColFormatter = ko.pureComputed(() => visibleColFormatterForRec(this, this.column(), docModel));
-  // 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.createValueParser = function() {
     const fieldRef = this.useColOptions.peek() ? undefined : this.id.peek();

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

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(() =>
+  this.valueFormatter = this.field.visibleColFormatter;
-    ValueFormatter.createFormatter(field.displayColModel().type(), this.options(), field.documentSettings())));
 
   this.textColor = Computed.create(this, (use) => use(this.field.textColor) || defaultTextColor)
     .onWrite((val) => this.field.textColor(val === defaultTextColor ? undefined : val));

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,
+    this.valueFormatter = fromKo(field.visibleColFormatter);
-    // 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);
   }
 
   /**

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<BaseFormatter>;
   private _visibleColRef: Computed<number>;
   private _validCols: Computed<Array<IOptionFull<number>>>;
 
   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);