(core) Fix y-axis blinking in chart view configuration

Summary:
This is a follow up diff for https://phab.getgrist.com/D3021.  Y-axis
draggable list used to blink when user changed either one of the x
axis or groupdata column.

This was due to the fact that all of theses axis are stored into the
same array and changing one of them changes the whole array even
though items relative to the y-axis actually were not changing.

This diff addresses this issue by 1) being carefull at not updating
the array of items when the changes do not impact y axis. And 2) by
adding a freeze observable allowing to freeze the draggable list of
y-axis while actions are being treated on the server.

Test Plan:
Catching such bug is hard, and given that it's only look and fill, maybe not worth the time and effort.

Tested manually though.

Reviewers: georgegevoian

Reviewed By: georgegevoian

Differential Revision: https://phab.getgrist.com/D3023

app/client/components/ChartView.ts

@@ -330,6 +330,8 @@ export class ChartConfig extends GrainJSDisposable {
   // observable is part of a hack to fix this issue.
   private _freezeXAxis = Observable.create(this, false);
 
+  private _freezeYAxis = Observable.create(this, false);
+
   // The column is of the x-axis.
   private _xAxis: Computed<number> = Computed.create(
     this, this._xAxisFieldIndex, this._freezeXAxis, (use, i, freeze) => {
@@ -381,21 +383,6 @@ export class ChartConfig extends GrainJSDisposable {
 
     if (this._section.parentKey() !== 'chart') { return null; }
 
-    // The y-axis are all visible fields that comes after the x-axis and maybe the group data
-    // column. Hence the draggable list of y-axis needs to skip either one or two visible fields.
-    const skipFirst = Computed.create(this, fromKo(this._optionsObj.prop('multiseries')), (_use, multiseries) =>  (
-      multiseries ? 2 : 1
-    ));
-
-    // The draggable list of y-axis
-    const fieldsDraggable = this._configFieldsHelper.buildVisibleFieldsConfigHelper({
-      itemCreateFunc: (field) => this._buildField(field),
-      draggableOptions: {
-        removeButton: false,
-        drag_indicator: cssDragger,
-      }, skipFirst
-    });
-
     return [
       cssRow(
         select(fromKoSave(this._section.chartTypeDef), [
@@ -460,7 +447,7 @@ export class ChartConfig extends GrainJSDisposable {
       ),
 
       cssLabel('SERIES'),
-      fieldsDraggable,
+      this._buildYAxis(),
       cssRow(
         cssAddYAxis(
           cssAddIcon('Plus'), 'Add Series',
@@ -480,25 +467,30 @@ export class ChartConfig extends GrainJSDisposable {
     const viewFields = this._section.viewFields.peek();
 
     await this._gristDoc.docData.bundleActions('selected new x-axis', async () => {
-      // first remove the current field
+      this._freezeYAxis.set(true);
-      if (this._xAxisFieldIndex.get() < viewFields.peek().length) {
+      try {
-        await this._configFieldsHelper.removeField(viewFields.peek()[this._xAxisFieldIndex.get()]);
+        // first remove the current field
-      }
+        if (this._xAxisFieldIndex.get() < viewFields.peek().length) {
+          await this._configFieldsHelper.removeField(viewFields.peek()[this._xAxisFieldIndex.get()]);
+        }
 
-      // if  new field was used to group by column series, disable multiseries
+        // if  new field was used to group by column series, disable multiseries
-      const fieldIndex = viewFields.peek().findIndex((f) => f.column.peek().getRowId() === colId);
+        const fieldIndex = viewFields.peek().findIndex((f) => f.column.peek().getRowId() === colId);
-      if (fieldIndex === 0 && optionsObj.prop('multiseries').peek()) {
+        if (fieldIndex === 0 && optionsObj.prop('multiseries').peek()) {
-        await optionsObj.prop('multiseries').setAndSave(false);
+          await optionsObj.prop('multiseries').setAndSave(false);
-        return;
+          return;
-      }
+        }
 
-      // if new field is already visible, moves the fields to the first place else add the field to the first
+        // if new field is already visible, moves the fields to the first place else add the field to the first
-      // place
+        // place
-      const xAxisField = viewFields.peek()[this._xAxisFieldIndex.get()];
+        const xAxisField = viewFields.peek()[this._xAxisFieldIndex.get()];
-      if (fieldIndex > -1) {
+        if (fieldIndex > -1) {
-        await this._configFieldsHelper.changeFieldPosition(viewFields.peek()[fieldIndex], xAxisField);
+          await this._configFieldsHelper.changeFieldPosition(viewFields.peek()[fieldIndex], xAxisField);
-      } else {
+        } else {
-        await this._configFieldsHelper.addField(col, xAxisField);
+          await this._configFieldsHelper.addField(col, xAxisField);
+        }
+      } finally {
+        this._freezeYAxis.set(false);
       }
     });
   }
@@ -508,6 +500,7 @@ export class ChartConfig extends GrainJSDisposable {
 
     await this._gristDoc.docData.bundleActions('selected new x-axis', async () => {
       this._freezeXAxis.set(true);
+      this._freezeYAxis.set(true);
       try {
         // if grouping was already set, first remove the current field
         if (this._groupDataColId.get() > -1) {
@@ -530,6 +523,7 @@ export class ChartConfig extends GrainJSDisposable {
         await this._optionsObj.prop('multiseries').setAndSave(colId > -1);
       } finally {
         this._freezeXAxis.set(false);
+        this._freezeYAxis.set(false);
       }
     });
   }
@@ -545,6 +539,23 @@ export class ChartConfig extends GrainJSDisposable {
       testId('y-axis'),
     );
   }
+
+  private _buildYAxis() {
+
+    // The y-axis are all visible fields that comes after the x-axis and maybe the group data
+    // column. Hence the draggable list of y-axis needs to skip either one or two visible fields.
+    const skipFirst = Computed.create(this, fromKo(this._optionsObj.prop('multiseries')), (_use, multiseries) =>  (
+      multiseries ? 2 : 1
+    ));
+
+    return this._configFieldsHelper.buildVisibleFieldsConfigHelper({
+      itemCreateFunc: (field) => this._buildField(field),
+      draggableOptions: {
+        removeButton: false,
+        drag_indicator: cssDragger,
+      }, skipFirst, freeze: this._freezeYAxis
+    });
+  }
 }
 
 function cssCheckboxRow(label: string, value: KoSaveableObservable<unknown>, ...args: DomElementArg[]) {

app/client/ui/VisibleFieldsConfig.ts

@@ -13,6 +13,7 @@ import { icon } from "app/client/ui2018/icons";
 import * as gutil from 'app/common/gutil';
 import { Computed, Disposable, dom, IDomArgs, makeTestId, Observable, styled, subscribe } from "grainjs";
 import difference = require("lodash/difference");
+import isEqual = require("lodash/isEqual");
 
 const testId = makeTestId('test-vfc-');
 
@@ -27,6 +28,11 @@ interface DraggableFieldsOption {
   // the group-by-column in the chart type widget.
   skipFirst?: Observable<number>;
 
+  // Allows to prevent updates of the list. This option is to be used when skipFirst option is used
+  // and it is useful to prevent the list to update during changes that only affect the skipped
+  // fields.
+  freeze?: Observable<boolean>;
+
   // the itemCreateFunc callback passed to kf.draggableList for the visible fields.
   itemCreateFunc(field: IField): Element|undefined;
 }
@@ -93,14 +99,21 @@ export class VisibleFieldsConfig extends Disposable {
     let fields = this._section.viewFields.peek();
 
     if (options.skipFirst) {
+      const freeze = options.freeze;
       const allFields = this._section.viewFields.peek();
       const newArray = new KoArray<ViewFieldRec>();
       function update() {
-        newArray.assign(allFields.peek().filter((_v, i) => i + 1 > options.skipFirst!.get()));
+        if (freeze && freeze.get()) { return; }
+        const newValues = allFields.peek().filter((_v, i) => i + 1 > options.skipFirst!.get());
+        if (isEqual(newArray.all(), newValues)) { return; }
+        newArray.assign(newValues);
       }
       update();
       this.autoDispose(allFields.subscribe(update));
       this.autoDispose(subscribe(options.skipFirst, update));
+      if (options.freeze) {
+        this.autoDispose(subscribe(options.freeze, update));
+      }
       fields = newArray;
     }