From 007c0f2af0f2b5fd1f218b10af84d7773279e391 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Tue, 7 Jun 2022 16:01:59 -0400 Subject: [PATCH] (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 | 16 +++------ app/client/lib/tableUtil.ts | 50 +++------------------------- app/client/models/TreeModel.ts | 10 ++---- app/client/models/rowset.ts | 11 +++--- app/client/ui/VisibleFieldsConfig.ts | 2 +- 5 files changed, 20 insertions(+), 69 deletions(-) diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index f1e3aab2..4a11daf1 100644 --- a/app/client/components/BaseView.js +++ b/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) { - var lowerRowId = this.viewData.getRowId(index-1); - var upperRowId = this.viewData.getRowId(index); - if (lowerRowId === 'new') { - // 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); +BaseView.prototype._getRowInsertPos = function(index, numInserts) { + var rowId = this.viewData.getRowId(index); + var insertPos = this.tableModel.tableData.getValue(rowId, MANUALSORT); + return Array(numInserts).fill(insertPos); }; /** diff --git a/app/client/lib/tableUtil.ts b/app/client/lib/tableUtil.ts index e7453f59..64d15395 100644 --- a/app/client/lib/tableUtil.ts +++ b/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 - * 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. + * Returns a sorted array of parentPos values for a viewField to be inserted just before 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, index: number, numInserts: number): number[] { - const leftPos = (index > 0) ? viewFields.at(index - 1)!.parentPos() : null; +export function fieldInsertPositions(viewFields: KoArray, index: number, numInserts: number = 1 +): Array { const rightPos = (index < viewFields.peekLength) ? viewFields.at(index)!.parentPos() : null; - return insertPositions(leftPos, rightPos, numInserts); + return Array(numInserts).fill(rightPos); } /** diff --git a/app/client/models/TreeModel.ts b/app/client/models/TreeModel.ts index 36d2ec45..b077257f 100644 --- a/app/client/models/TreeModel.ts +++ b/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}); } } diff --git a/app/client/models/rowset.ts b/app/client/models/rowset.ts index d821617c..0b3468a8 100644 --- a/app/client/models/rowset.ts +++ b/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(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(array: T[], sortedRows: Iterable, compareFunc: CompareFunc): boolean { +function _allRowsSorted(array: T[], allRows: Set, sortedRows: Iterable, compareFunc: CompareFunc): 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 (!_isIndexInOrder(array, index, compareFunc)) { + if (index === -1 || !_isIndexInOrder(array, index, compareFunc)) { + // rows of sortedRows are not present in the array in the same relative order. return false; } last = index; diff --git a/app/client/ui/VisibleFieldsConfig.ts b/app/client/ui/VisibleFieldsConfig.ts index b1942daa..f8219650 100644 --- a/app/client/ui/VisibleFieldsConfig.ts +++ b/app/client/ui/VisibleFieldsConfig.ts @@ -401,7 +401,7 @@ export class VisibleFieldsConfig extends Disposable { } function getFieldNewPosition(fields: KoArray, item: IField, - nextField: ViewFieldRec|null): number { + nextField: ViewFieldRec|null): number|null { const index = getItemIndex(fields, nextField); return tableUtil.fieldInsertPositions(fields, index, 1)[0]; }