address PR comments

deephaven · Aug 22, 2024 · 1cbc60d · 1cbc60d
1 parent 8c80230
commit 1cbc60d
Show file tree

Hide file tree

Showing 7 changed files with 41 additions and 32 deletions.
diff --git a/packages/code-studio/src/settings/FormattingSectionContent.tsx b/packages/code-studio/src/settings/FormattingSectionContent.tsx
@@ -617,11 +617,11 @@ export class FormattingSectionContent extends PureComponent<
           <div className="form-row mb-3" id="show_extra_group_column_div">
             <label
               className="col-form-label col-3"
-              htmlFor="default-integer-format-input"
+              htmlFor="default-show-extra-group-column"
             >
               Rollup
             </label>
-            <div className="col pr-0 pt-2" id="show_extra_grou_column_cb">
+            <div className="col pr-0 pt-2" id="default-show-extra-group-column">
               <Checkbox
                 checked={showExtraGroupColumn}
                 onChange={this.handleShowExtraGroupColumnChange}

diff --git a/packages/eslint-config/index.js b/packages/eslint-config/index.js
@@ -20,6 +20,12 @@ module.exports = {
     'react/jsx-uses-react': 'error',
     'react/jsx-uses-vars': 'error',
     'react/jsx-filename-extension': [1, { extensions: ['.jsx', '.tsx'] }],
+    'react-hooks/exhaustive-deps': [
+      'warn',
+      {
+        additionalHooks: '(useOnChange)',
+      },
+    ],
     'react/react-in-jsx-scope': 'off',
     'react/sort-comp': [
       2,

diff --git a/packages/iris-grid/src/IrisGrid.tsx b/packages/iris-grid/src/IrisGrid.tsx
@@ -1986,8 +1986,7 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
       this.integerFormatOptions,
       this.truncateNumbersWithPound,
       this.showEmptyStrings,
-      this.showNullStrings,
-      this.showExtraGroupColumn
+      this.showNullStrings
     );
 
     log.debug('updateFormatter', this.globalColumnFormats, mergedColumnFormats);
@@ -4866,6 +4865,7 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> {
                 frozenColumns={frozenColumns}
                 columnHeaderGroups={columnHeaderGroups}
                 partitionConfig={partitionConfig}
+                showExtraGroupColumn={this.showExtraGroupColumn}
               />
             )}
             {!isMenuShown && (

diff --git a/packages/iris-grid/src/IrisGridModelUpdater.tsx b/packages/iris-grid/src/IrisGridModelUpdater.tsx
@@ -14,6 +14,7 @@ import {
   PartitionConfig,
   isPartitionedGridModel,
 } from './PartitionedGridModel';
+import { isIrisGridTreeTableModel } from './IrisGridTreeTableModel';
 
 const COLUMN_BUFFER_PAGES = 1;
 
@@ -40,6 +41,7 @@ interface IrisGridModelUpdaterProps {
   pendingRowCount?: number;
   pendingDataMap?: PendingDataMap;
   partitionConfig?: PartitionConfig;
+  showExtraGroupColumn?: boolean;
 }
 
 /**
@@ -68,10 +70,15 @@ function IrisGridModelUpdater({
   formatColumns,
   columnHeaderGroups,
   partitionConfig,
+  showExtraGroupColumn,
 }: IrisGridModelUpdaterProps): JSX.Element | null {
-  if (model.formatter !== formatter) {
-    model.formatter = formatter;
-  }
+  // Check for showExtraGroupColumn before memoizing columns, since updating it will change the columns
+  useOnChange(() => {
+    if (isIrisGridTreeTableModel(model) && showExtraGroupColumn != null) {
+      model.showExtraGroupColumn = showExtraGroupColumn;
+    }
+  }, [model, showExtraGroupColumn]);
+
   const columns = useMemo(
     () =>
       IrisGridUtils.getModelViewportColumns(
@@ -92,7 +99,6 @@ function IrisGridModelUpdater({
       alwaysFetchColumns,
     ]
   );
-
   useOnChange(
     function updateFilter() {
       model.filter = filter;

diff --git a/packages/iris-grid/src/IrisGridTreeTableModel.ts b/packages/iris-grid/src/IrisGridTreeTableModel.ts
@@ -63,43 +63,46 @@ function isLayoutTreeTable(table: DhType.TreeTable): table is LayoutTreeTable {
   return (table as LayoutTreeTable).layoutHints !== undefined;
 }
 
+export function isIrisGridTreeTableModel(
+  tableModel: IrisGridModel
+): tableModel is IrisGridTreeTableModel {
+  return (tableModel as IrisGridTreeTableModel).showExtraGroupColumn != null;
+}
+
 class IrisGridTreeTableModel extends IrisGridTableModelTemplate<
   DhType.TreeTable,
   UITreeRow
 > {
   /** We keep a virtual column at the front that tracks the "group" that is expanded */
   private virtualColumns: DisplayColumn[];
 
+  private showExtraGroupCol = true;
+
   constructor(
     dh: typeof DhType,
     table: DhType.TreeTable,
     formatter = new Formatter(dh),
     inputTable: DhType.InputTable | null = null
   ) {
     super(dh, table, formatter, inputTable);
-    this.virtualColumns = this.formatter.showExtraGroupColumn
-      ? [VirtualGroupColumn]
-      : [];
+
+    this.virtualColumns = this.showExtraGroupColumn ? [VirtualGroupColumn] : [];
   }
 
-  set formatter(newFormatter: Formatter) {
-    const oldFormatter = super.formatter;
-    super.formatter = newFormatter;
-    if (
-      newFormatter.showExtraGroupColumn !== oldFormatter.showExtraGroupColumn
-    ) {
-      this.updateVirtualColumns();
-    }
+  get showExtraGroupColumn(): boolean {
+    return this.showExtraGroupCol;
   }
 
-  get formatter(): Formatter {
-    return super.formatter;
+  set showExtraGroupColumn(showExtraGroupCol: boolean) {
+    if (this.showExtraGroupCol === showExtraGroupCol) {
+      return;
+    }
+    this.showExtraGroupCol = showExtraGroupCol;
+    this.updateVirtualColumns();
   }
 
   updateVirtualColumns(): void {
-    this.virtualColumns = this.formatter.showExtraGroupColumn
-      ? [VirtualGroupColumn]
-      : [];
+    this.virtualColumns = this.showExtraGroupColumn ? [VirtualGroupColumn] : [];
     this.dispatchEvent(
       new EventShimCustomEvent(IrisGridModel.EVENT.COLUMNS_CHANGED, {
         detail: this.columns,

diff --git a/packages/jsapi-utils/src/Formatter.ts b/packages/jsapi-utils/src/Formatter.ts
@@ -86,8 +86,7 @@ export class Formatter {
     >[1],
     truncateNumbersWithPound = false,
     showEmptyStrings = true,
-    showNullStrings = true,
-    showExtraGroupColumn = true
+    showNullStrings = true
   ) {
     // Formatting order:
     // - columnFormatMap[type][name]
@@ -121,7 +120,6 @@ export class Formatter {
 
     this.showEmptyStrings = showEmptyStrings;
     this.showNullStrings = showNullStrings;
-    this.showExtraGroupColumn = showExtraGroupColumn;
   }
 
   defaultColumnFormatter: TableColumnFormatter;
@@ -136,8 +134,6 @@ export class Formatter {
 
   showNullStrings: boolean;
 
-  showExtraGroupColumn: boolean;
-
   /**
    * Gets columnFormatMap indexed by name for a given column type, creates new Map entry if necessary
    * @param columnType column type

diff --git a/packages/jsapi-utils/src/FormatterUtils.ts b/packages/jsapi-utils/src/FormatterUtils.ts
@@ -26,7 +26,6 @@ export function createFormatterFromSettings(
     truncateNumbersWithPound,
     showEmptyStrings,
     showNullStrings,
-    showExtraGroupColumn,
   } = settings ?? {};
 
   return new Formatter(
@@ -37,8 +36,7 @@ export function createFormatterFromSettings(
     defaultIntegerFormatOptions,
     truncateNumbersWithPound,
     showEmptyStrings,
-    showNullStrings,
-    showExtraGroupColumn
+    showNullStrings
   );
 }