From 074aa68d5c8ccc212da1b729d740bfb3d700c890 Mon Sep 17 00:00:00 2001 From: John Haynes Date: Wed, 15 Jan 2025 18:05:09 -0500 Subject: [PATCH 1/4] Remove outdated optimization for ag-grid column state sync --- cmp/grid/Grid.ts | 89 ++++++++++++++++++++++-------------------------- 1 file changed, 40 insertions(+), 49 deletions(-) diff --git a/cmp/grid/Grid.ts b/cmp/grid/Grid.ts index 5137631b9..c65fa9cfa 100644 --- a/cmp/grid/Grid.ts +++ b/cmp/grid/Grid.ts @@ -4,9 +4,10 @@ * * Copyright © 2025 Extremely Heavy Industries Inc. */ +import {ColumnState as AgColumnState, GridApi} from '@ag-grid-community/core'; import composeRefs from '@seznam/compose-react-refs'; import {agGrid, AgGrid} from '@xh/hoist/cmp/ag-grid'; -import {getTreeStyleClasses} from '@xh/hoist/cmp/grid'; +import {ColumnState, getTreeStyleClasses} from '@xh/hoist/cmp/grid'; import {gridHScrollbar} from '@xh/hoist/cmp/grid/impl/GridHScrollbar'; import {getAgGridMenuItems} from '@xh/hoist/cmp/grid/impl/MenuSupport'; import {div, fragment, frame, vframe} from '@xh/hoist/cmp/layout'; @@ -17,6 +18,7 @@ import { LayoutProps, lookup, PlainObject, + ReactionSpec, TestSupportProps, useLocalModel, uses, @@ -44,7 +46,7 @@ import {wait} from '@xh/hoist/promise'; import {consumeEvent, isDisplayed, logWithDebug} from '@xh/hoist/utils/js'; import {createObservableRef, getLayoutProps} from '@xh/hoist/utils/react'; import classNames from 'classnames'; -import {debounce, isEmpty, isEqual, isNil, max, maxBy, merge} from 'lodash'; +import {compact, debounce, isEmpty, isEqual, isNil, max, maxBy, merge} from 'lodash'; import './Grid.scss'; import {GridModel} from './GridModel'; import {columnGroupHeader} from './impl/ColumnGroupHeader'; @@ -463,7 +465,7 @@ export class GridLocalModel extends HoistModel { }; } - columnStateReaction() { + columnStateReaction(): ReactionSpec<[GridApi, ColumnState[]]> { const {model} = this; return { track: () => [model.agApi, model.columnState], @@ -472,65 +474,53 @@ export class GridLocalModel extends HoistModel { const agColState = api.getColumnState(); - // 0) Insert the auto group col state if it exists, since we won't have it in our column state list + // Insert the auto group col state if it exists, since we won't have it in our column state list const autoColState = agColState.find(c => c.colId === 'ag-Grid-AutoColumn'); if (autoColState) { - colState.splice(agColState.indexOf(autoColState), 0, autoColState); + colState.splice( + agColState.indexOf(autoColState), + 0, + autoColState as ColumnState + ); } - // 1) Columns all in right place -- simply update incorrect props we maintain - if ( - isEqual( - colState.map(c => c.colId), - agColState.map(c => c.colId) - ) - ) { - let hasChanges = false; - colState.forEach((col, index) => { - const agCol = agColState[index], - id = col.colId; - - if (agCol.width !== col.width) { - api.setColumnWidths([{key: id, newWidth: col.width}]); + // Determine if column order has changed + const applyOrder = !isEqual( + colState.map(c => c.colId), + agColState.map(c => c.colId) + ); + + // Build a list of column state changes + colState = compact( + colState.map(({colId, width, hidden, pinned}) => { + const agCol: AgColumnState = agColState.find(c => c.colId === colId) || { + colId + }, + ret: any = {colId}; + + let hasChanges = applyOrder; + + if (agCol.width !== width) { + ret.width = width; hasChanges = true; } - if (agCol.hide !== col.hidden) { - api.setColumnsVisible([id], !col.hidden); + + if (agCol.hide !== hidden) { + ret.hide = hidden; hasChanges = true; } - if (agCol.pinned !== col.pinned) { - api.setColumnsPinned([id], col.pinned); + + if (agCol.pinned !== pinned) { + ret.pinned = pinned; hasChanges = true; } - }); - - // We need to tell agGrid to refresh its flexed column sizes due to - // a regression introduced in 25.1.0. See #2341 - if (hasChanges) { - api.columnModel.refreshFlexedColumns({ - updateBodyWidths: true, - fireResizedEvent: true - }); - } - - return; - } - // 2) Otherwise do an (expensive) full refresh of column state - // Merge our state onto the ag column state to get any state which we do not yet support - colState = colState.map(({colId, width, hidden, pinned}) => { - const agCol = agColState.find(c => c.colId === colId) || {}; - return { - colId, - ...agCol, - width, - pinned, - hide: hidden - }; - }); + return hasChanges ? ret : null; + }) + ); this.doWithPreservedState({expansion: false}, () => { - api.applyColumnState({state: colState, applyOrder: true}); + api.applyColumnState({state: colState, applyOrder}); }); } }; @@ -870,6 +860,7 @@ export class GridLocalModel extends HoistModel { * by conditionally stopping the focus event from propagating. */ private static didAddFocusFixListener = false; + static addFocusFixListener() { if (this.didAddFocusFixListener) return; document.addEventListener( From 59dfb64537991a9da6c517102d192eb1ee5b34bf Mon Sep 17 00:00:00 2001 From: John Haynes Date: Wed, 15 Jan 2025 18:06:56 -0500 Subject: [PATCH 2/4] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d2be7a506..f8a9d7d20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## v72.0.0-SNAPSHOT - unreleased +### 🐞 Bug Fixes + +* Fixed un-optimal column state synchronization between `GridModel` and Ag-Grid + ### ⚙️ Technical * Added support for providing custom `PersistenceProvider` implementations to `PersistOptions`. From 17d1b0e22ac361bd5353296486869215b404fd47 Mon Sep 17 00:00:00 2001 From: John Haynes Date: Thu, 16 Jan 2025 14:15:28 -0500 Subject: [PATCH 3/4] Checkpoint --- cmp/ag-grid/AgGridModel.ts | 2 +- cmp/grid/Grid.ts | 16 ++++++++++------ cmp/grid/GridModel.ts | 26 +++++++++++++++----------- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/cmp/ag-grid/AgGridModel.ts b/cmp/ag-grid/AgGridModel.ts index 36b9b187a..e93f3b1ae 100644 --- a/cmp/ag-grid/AgGridModel.ts +++ b/cmp/ag-grid/AgGridModel.ts @@ -395,7 +395,7 @@ export class AgGridModel extends HoistModel { ]; let {isPivot, columns} = colState; - agApi.setPivotMode(isPivot); + agApi.setGridOption('pivotMode', isPivot); if (isPivot && columns.some(it => !isNil(it.pivotIndex) && it.pivotIndex >= 0)) { // Exclude the auto group column as this causes issues with ag-grid when in pivot mode diff --git a/cmp/grid/Grid.ts b/cmp/grid/Grid.ts index c65fa9cfa..c3995caf1 100644 --- a/cmp/grid/Grid.ts +++ b/cmp/grid/Grid.ts @@ -46,7 +46,7 @@ import {wait} from '@xh/hoist/promise'; import {consumeEvent, isDisplayed, logWithDebug} from '@xh/hoist/utils/js'; import {createObservableRef, getLayoutProps} from '@xh/hoist/utils/react'; import classNames from 'classnames'; -import {compact, debounce, isEmpty, isEqual, isNil, max, maxBy, merge} from 'lodash'; +import {compact, debounce, isBoolean, isEmpty, isEqual, isNil, max, maxBy, merge} from 'lodash'; import './Grid.scss'; import {GridModel} from './GridModel'; import {columnGroupHeader} from './impl/ColumnGroupHeader'; @@ -477,11 +477,13 @@ export class GridLocalModel extends HoistModel { // Insert the auto group col state if it exists, since we won't have it in our column state list const autoColState = agColState.find(c => c.colId === 'ag-Grid-AutoColumn'); if (autoColState) { - colState.splice( - agColState.indexOf(autoColState), - 0, - autoColState as ColumnState - ); + const {colId, width, hide, pinned} = autoColState; + colState.splice(agColState.indexOf(autoColState), 0, { + colId, + width, + hidden: hide, + pinned: isBoolean(pinned) ? (pinned ? 'left' : null) : pinned + }); } // Determine if column order has changed @@ -519,6 +521,8 @@ export class GridLocalModel extends HoistModel { }) ); + if (isEmpty(colState)) return; + this.doWithPreservedState({expansion: false}, () => { api.applyColumnState({state: colState, applyOrder}); }); diff --git a/cmp/grid/GridModel.ts b/cmp/grid/GridModel.ts index d191422c2..dc7f4be47 100644 --- a/cmp/grid/GridModel.ts +++ b/cmp/grid/GridModel.ts @@ -9,6 +9,7 @@ import { CellContextMenuEvent, CellDoubleClickedEvent, ColumnEvent, + ColumnState as AgColumnState, RowClickedEvent, RowDoubleClickedEvent } from '@ag-grid-community/core'; @@ -77,6 +78,7 @@ import { first, forEach, isArray, + isBoolean, isEmpty, isFunction, isNil, @@ -1072,17 +1074,19 @@ export class GridModel extends HoistModel { (this.colChooserModel as any)?.open(); } - noteAgColumnStateChanged(agColState) { - const colStateChanges = agColState.map(({colId, width, hide, pinned}) => { - const col = this.findColumn(this.columns, colId); - if (!col) return null; - return { - colId, - pinned: pinned ?? null, - hidden: !!hide, - width: col.flex ? undefined : width - }; - }); + noteAgColumnStateChanged(agColState: AgColumnState[]) { + const colStateChanges: Partial[] = agColState.map( + ({colId, width, hide, pinned}) => { + const col = this.findColumn(this.columns, colId); + if (!col) return null; + return { + colId, + pinned: isBoolean(pinned) ? (pinned ? 'left' : null) : pinned, + hidden: !!hide, + width: col.flex ? undefined : width + }; + } + ); pull(colStateChanges, null); this.applyColumnStateChanges(colStateChanges); From b424c9191f9865b529485f3e1cf7288a4e9dabbb Mon Sep 17 00:00:00 2001 From: John Haynes Date: Thu, 16 Jan 2025 14:35:28 -0500 Subject: [PATCH 4/4] Fix merge issue --- cmp/viewmanager/ViewManagerModel.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmp/viewmanager/ViewManagerModel.ts b/cmp/viewmanager/ViewManagerModel.ts index e8df53f97..aba09dc6b 100644 --- a/cmp/viewmanager/ViewManagerModel.ts +++ b/cmp/viewmanager/ViewManagerModel.ts @@ -456,7 +456,8 @@ export class ViewManagerModel extends HoistModel { if (name.length > maxLength) { return `Name cannot be longer than ${maxLength} characters`; } - if (this.ownedViews.some(view => view.name === name && view.token != existing?.token)) { + const views = existing?.isGlobal ? this.globalViews : this.ownedViews; + if (views.some(view => view.name === name && view.token != existing?.token)) { return `A ${this.typeDisplayName} with name '${name}' already exists.`; } return null;