(core) Fix some bugs with repositioning rows.

Summary:
- Fixed an issue with manualSort values being very close floats. It is already handled by the data engine, but the client was being unnecessarily proactive and introduced a bug.
- The fix also helps with rearranging rows in filtered situations: they will now stay next to the row before which they were inserted.
- The fix accidentally improves (though doesn't fully fix) the issue where new columns show up in unexpected places in the raw-data column list.
- Fixed another rare bug with row order not getting updated correctly when positions update.

Test Plan: Added test cases for the improved behavior; fixed affected tests.

Reviewers: georgegevoian

Reviewed By: georgegevoian

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

app/client/components/BaseView.js

@@ -656,18 +656,10 @@ BaseView.prototype.scrollToCursor = function() {
  * with the returned positions will place them in between index-1 and index.
  * when the GridView is sorted by MANUALSORT
  **/
- BaseView.prototype._getRowInsertPos = function(index, numInserts) {
+BaseView.prototype._getRowInsertPos = function(index, numInserts) {
-  var lowerRowId = this.viewData.getRowId(index-1);
+  var rowId = this.viewData.getRowId(index);
-  var upperRowId = this.viewData.getRowId(index);
+  var insertPos = this.tableModel.tableData.getValue(rowId, MANUALSORT);
-  if (lowerRowId === 'new') {
+  return Array(numInserts).fill(insertPos);
-    // set the lowerRowId to the rowId of the row before 'new'.
-    lowerRowId = this.viewData.getRowId(index - 2);
-  }
-
-  var lowerPos = this.tableModel.tableData.getValue(lowerRowId, MANUALSORT);
-  var upperPos = this.tableModel.tableData.getValue(upperRowId, MANUALSORT);
-  // tableUtil.insertPositions takes care of cases where upper/lowerPos are non-zero & falsy
-  return tableUtil.insertPositions(lowerPos, upperPos, numInserts);
 };
 
 /**

app/client/lib/tableUtil.ts

@@ -14,55 +14,15 @@ import zipObject = require('lodash/zipObject');
 const G = getBrowserGlobals('document', 'DOMParser');
 
 /**
- *  Returns unique positions given upper and lower position. This function returns a suitable
+ * Returns a sorted array of parentPos values for a viewField to be inserted just before index.
- *  position number for the to-be-inserted element to end up at the given index.
- *  Inserting n elements between a and b should give the positions:
- *  (a+(b-a)/(n+1)), (a+2(b-a)/(n+1)) , ..., (a+(n)(b-a)/(n+1))
- *  @param {number} lowerPos - a lower bound
- *  @param {number} upperPos - an upper bound, must be greater than or equal to lowerPos
- *  @param {number} numInserts - Number of new positions to insert
- *  @returns {number[]} A sorted Array of unique positions bounded by lowerPos and upperPos.
- *                        If neither an upper nor lowerPos is given, return 0, 1, ..., numInserts - 1
- *                        If an upperPos is not given, return consecutive values greater than lowerPos
- *                        If a lowerPos is not given, return consecutive values lower than upperPos
- *                        Else return the avg position of to-be neighboring elements.
- *  Ex: insertPositions(null, 0, 4) = [-4, -3, -2, -1]
- *      insertPositions(0, null, 4) = [1, 2, 3, 4]
- *      insertPositions(0, 1, 4) = [0.2, 0.4, 0.6, 0.8]
- */
-export function insertPositions(lowerPos: number|null, upperPos: number|null, numInserts: number): number[] {
-  numInserts = (typeof numInserts === 'undefined') ? 1 : numInserts;
-  let start = 0;
-  let step = 1;
-  const positions = [];
-
-  if (typeof lowerPos !== 'number' && typeof upperPos !== 'number') {
-    start = 0;
-  } else if (typeof lowerPos !== 'number') {
-    start = upperPos! - numInserts;
-  } else if (typeof upperPos !== 'number') {
-    start = lowerPos + 1;
-  } else {
-    step = (upperPos - lowerPos)/(numInserts + 1);
-    start = lowerPos + step;
-  }
-
-  for(let i = 0; i < numInserts; i++ ){
-    positions.push(start + step*i);
-  }
-  return positions;
-}
-
-/**
- * Returns a sorted array of parentPos values between the parentPos of the viewField at index-1 and index.
  * @param {koArray} viewFields - koArray of viewFields
- * @{param} {number} index - index to insert the viewFields into
+ * @{param} {number} index - index in viewFields at which to insert the new fields
  * @{param} {number} numInserts - number of new fields to insert
  */
-export function fieldInsertPositions(viewFields: KoArray<ViewFieldRec>, index: number, numInserts: number): number[] {
+export function fieldInsertPositions(viewFields: KoArray<ViewFieldRec>, index: number, numInserts: number = 1
-  const leftPos = (index > 0) ? viewFields.at(index - 1)!.parentPos() : null;
+): Array<number|null> {
   const rightPos = (index < viewFields.peekLength) ? viewFields.at(index)!.parentPos() : null;
-  return insertPositions(leftPos, rightPos, numInserts);
+  return Array(numInserts).fill(rightPos);
 }
 
 /**

app/client/models/TreeModel.ts

@@ -12,7 +12,6 @@
  *
  */
 
-import { insertPositions } from "app/client/lib/tableUtil";
 import { BulkColValues, UserAction } from "app/common/DocActions";
 import { nativeCompare } from "app/common/gutil";
 import { obsArray, ObsArray } from "grainjs";
@@ -155,24 +154,21 @@ export class TreeNodeRecord implements TreeNode {
       const indentations = records.map((rec, i) => rec.indentation + indent - records[0].indentation);
 
       // adjust positions
-      let upperPos, lowerPos: number|null;
+      let upperPos: number|null;
       if (nextChild) {
         const index = nextChild.index;
         upperPos = this._records[index].pagePos;
-        lowerPos = index ? this._records[index - 1].pagePos : null;
       } else {
         const lastIndex = this.findLastIndex();
         if (lastIndex !== "root") {
           upperPos = (this._records[lastIndex + 1] || {pagePos: null}).pagePos;
-          lowerPos = this._records[lastIndex].pagePos;
         } else {
-          upperPos = lowerPos = null;
+          upperPos = null;
         }
       }
-      const positions = insertPositions(lowerPos, upperPos, records.length);
 
       // do update
-      const update = records.map((rec, i) => ({...rec, indentation: indentations[i], pagePos: positions[i]}));
+      const update = records.map((rec, i) => ({...rec, indentation: indentations[i], pagePos: upperPos!}));
       await this.sendActions({update});
     }
   }

app/client/models/rowset.ts

@@ -630,7 +630,7 @@ export class SortedRowSet extends RowListener {
     // Note that the logic is all or none, since we can't assume that a single row is in its right
     // place by comparing to neighbors because the neighbors might themselves be affected and wrong.
     const sortedRows = Array.from(rows).sort(this._compareFunc);
-    if (_allRowsSorted(this._koArray.peek(), sortedRows, this._compareFunc)) {
+    if (_allRowsSorted(this._koArray.peek(), this._allRows, sortedRows, this._compareFunc)) {
       return;
     }
 
@@ -724,12 +724,15 @@ function _isIndexInOrder<T>(array: T[], index: number, compareFunc: CompareFunc<
  * Helper function to tell if each of sortedRows, if present in the array, is in order relative to
  * its neighbors. sortedRows should be sorted the same way as the array.
  */
-function _allRowsSorted<T>(array: T[], sortedRows: Iterable<T>, compareFunc: CompareFunc<T>): boolean {
+function _allRowsSorted<T>(array: T[], allRows: Set<T>, sortedRows: Iterable<T>, compareFunc: CompareFunc<T>): boolean {
   let last = 0;
   for (const r of sortedRows) {
+    if (!allRows.has(r)) {
+      continue;
+    }
     const index = array.indexOf(r, last);
-    if (index === -1) { continue; }
+    if (index === -1 || !_isIndexInOrder(array, index, compareFunc)) {
-    if (!_isIndexInOrder(array, index, compareFunc)) {
+      // rows of sortedRows are not present in the array in the same relative order.
       return false;
     }
     last = index;

app/client/ui/VisibleFieldsConfig.ts

@@ -401,7 +401,7 @@ export class VisibleFieldsConfig extends Disposable {
 }
 
 function getFieldNewPosition(fields: KoArray<ViewFieldRec>, item: IField,
-                             nextField: ViewFieldRec|null): number {
+                             nextField: ViewFieldRec|null): number|null {
   const index = getItemIndex(fields, nextField);
   return tableUtil.fieldInsertPositions(fields, index, 1)[0];
 }