Skip to content

Commit

Permalink
Optimize column state sync (#3905)
Browse files Browse the repository at this point in the history 
* Remove outdated optimization for ag-grid column state sync

* Update changelog

* Checkpoint

* Fix merge issue

---------

Co-authored-by: Anselm McClain <[email protected]>
  • Loading branch information
@haynesjm42 @amcclain
haynesjm42 and amcclain authored Jan 24, 2025
1 parent 0ed29ce commit 923a4f8
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 61 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
### 🐞 Bug Fixes

* Fixed `ViewManagerModel` unique name validation.
* `GridModel.restoreDefaultsAsync()` now restores default filter rather than simply clearing it.
* Fixed `GridModel.restoreDefaultsAsync()` to restore any default filter, rather than simply clearing it.
* Improved suboptimal column state synchronization between `GridModel` and AG Grid.

### ⚙️ Technical

Expand Down
2 changes: 1 addition & 1 deletion cmp/ag-grid/AgGridModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
91 changes: 43 additions & 48 deletions cmp/grid/Grid.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -17,6 +18,7 @@ import {
LayoutProps,
lookup,
PlainObject,
ReactionSpec,
TestSupportProps,
useLocalModel,
uses,
Expand Down Expand Up @@ -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, isBoolean, isEmpty, isEqual, isNil, max, maxBy, merge} from 'lodash';
import './Grid.scss';
import {GridModel} from './GridModel';
import {columnGroupHeader} from './impl/ColumnGroupHeader';
Expand Down Expand Up @@ -463,7 +465,7 @@ export class GridLocalModel extends HoistModel {
};
}

columnStateReaction() {
columnStateReaction(): ReactionSpec<[GridApi, ColumnState[]]> {
const {model} = this;
return {
track: () => [model.agApi, model.columnState],
Expand All @@ -472,65 +474,57 @@ 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);
const {colId, width, hide, pinned} = autoColState;
colState.splice(agColState.indexOf(autoColState), 0, {
colId,
width,
hidden: hide,
pinned: isBoolean(pinned) ? (pinned ? 'left' : null) : pinned
});
}

// 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;
}
return hasChanges ? ret : null;
})
);

// 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
};
});
if (isEmpty(colState)) return;

this.doWithPreservedState({expansion: false}, () => {
api.applyColumnState({state: colState, applyOrder: true});
api.applyColumnState({state: colState, applyOrder});
});
}
};
Expand Down Expand Up @@ -870,6 +864,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(
Expand Down
26 changes: 15 additions & 11 deletions cmp/grid/GridModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
CellContextMenuEvent,
CellDoubleClickedEvent,
ColumnEvent,
ColumnState as AgColumnState,
RowClickedEvent,
RowDoubleClickedEvent
} from '@ag-grid-community/core';
Expand Down Expand Up @@ -77,6 +78,7 @@ import {
first,
forEach,
isArray,
isBoolean,
isEmpty,
isFunction,
isNil,
Expand Down Expand Up @@ -1073,17 +1075,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<ColumnState>[] = 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);
Expand Down

0 comments on commit 923a4f8

Please sign in to comment.