Skip to content

Commit

Permalink
Fix up based on review
Browse files Browse the repository at this point in the history
- Don't show the proxy column in the Organize Columns, Search Columns, or aggregations sections
- Fix an issue with rehydrating tree tables with an aggregation set (table.size can be negative on initial load)
  • Loading branch information
mofojed committed Dec 6, 2023
1 parent dfddb23 commit e3667e5
Show file tree
Hide file tree
Showing 8 changed files with 65 additions and 37 deletions.
6 changes: 3 additions & 3 deletions packages/iris-grid/src/ColumnStatistics.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ interface ColumnStatisticsProps {
interface ColumnStatisticsState {
error: unknown;
loading: boolean;
statistics: Statistic[] | null;
statistics: readonly Statistic[] | null;
numRows: number;
}

Expand Down Expand Up @@ -81,15 +81,15 @@ class ColumnStatistics extends Component<
cancelablePromise: CancelablePromise<APIColumnStatistics> | null;

maybeGenerateStatistics(): void {
const { model } = this.props;
const { column, model } = this.props;

const numRows =
model.rowCount -
model.pendingRowCount -
model.floatingBottomRowCount -
model.floatingTopRowCount;
this.setState({ numRows });
if (!model.isColumnStatisticsAvailable) {
if (!model.isColumnStatisticsAvailable || column.isProxy === true) {
this.setState({ loading: false });
} else if (numRows < ColumnStatistics.AUTO_GENERATE_LIMIT) {
this.handleGenerateStatistics();
Expand Down
39 changes: 22 additions & 17 deletions packages/iris-grid/src/CrossColumnSearch.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type {
import { TableUtils } from '@deephaven/jsapi-utils';
import './CrossColumnSearch.scss';
import { ColumnName } from './CommonTypes';
import { DisplayColumn } from './IrisGridModel';

interface CrossColumnSearchProps {
value: string;
Expand All @@ -27,7 +28,7 @@ interface CrossColumnSearchProps {
selectedColumns: readonly ColumnName[],
invertSelection: boolean
) => void;
columns: readonly Column[];
columns: readonly DisplayColumn[];
}

interface CrossColumnSearchState {
Expand Down Expand Up @@ -233,23 +234,27 @@ class CrossColumnSearch extends PureComponent<
<div className="cross-column-popup">
Searched Columns
<div className="cross-column-scroll">
{columns.map(column => (
<React.Fragment key={column.name}>
<Checkbox
className="cross-column-checkbox"
checked={
invertSelection
? !selectedColumns.includes(column.name)
: selectedColumns.includes(column.name)
}
onChange={() => this.toggleColumn(column.name)}
>
{column.name}
</Checkbox>
{columns.map(column => {
if (column.isProxy === true) return null;

{column.type.substring(column.type.lastIndexOf('.') + 1)}
</React.Fragment>
))}
return (
<React.Fragment key={column.name}>
<Checkbox
className="cross-column-checkbox"
checked={
invertSelection
? !selectedColumns.includes(column.name)
: selectedColumns.includes(column.name)
}
onChange={() => this.toggleColumn(column.name)}
>
{column.name}
</Checkbox>

{column.type.substring(column.type.lastIndexOf('.') + 1)}
</React.Fragment>
);
})}
</div>
<div className="cross-column-button-bar">
<button
Expand Down
6 changes: 6 additions & 0 deletions packages/iris-grid/src/IrisGridModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@ export type DisplayColumn = Column & {
* whereas `displayName` can be any string and does not need to be unique.
*/
displayName?: string;

/**
* Whether this column is a proxy column for other columns or not.
* If it's a proxy column, it may not appear in some lists.
*/
isProxy?: boolean;
};

type IrisGridModelEventNames =
Expand Down
3 changes: 2 additions & 1 deletion packages/iris-grid/src/IrisGridTableModelTemplate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,8 @@ class IrisGridTableModelTemplate<

get rowCount(): number {
return (
this.table.size +
// Table size can be negative if the table isn't ready yet
Math.max(this.table.size, 0) +
this.pendingNewRowCount +
(this.totals?.operationOrder?.length ?? 0)
);
Expand Down
9 changes: 4 additions & 5 deletions packages/iris-grid/src/IrisGridTreeTableModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class IrisGridTreeTableModel extends IrisGridTableModelTemplate<
constituentType: TableUtils.dataType.STRING,
isPartitionColumn: false,
isSortable: false,
isProxy: true,
description: 'Key column',
filter: () => {
throw new Error('Filter not implemented for virtual column');
Expand Down Expand Up @@ -112,13 +113,11 @@ class IrisGridTreeTableModel extends IrisGridTableModelTemplate<
if (hasChildren) {
for (let i = 0; i < this.virtualColumns.length; i += 1) {
const key = i + (depth - 1) + (this.virtualColumns.length - 1);
if (!modifiedData.has(key)) {
const cellData = modifiedData.get(key);
if (cellData == null) {
log.warn('Missing key data for virtual column', i, depth, key, row);
} else {
const cellData = modifiedData.get(key);
if (cellData != null) {
modifiedData.set(i, cellData);
}
modifiedData.set(i, cellData);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ import classNames from 'classnames';
import { CSSTransition } from 'react-transition-group';
import { Button, Checkbox, ItemList, ThemeExport } from '@deephaven/components';
import { dhSortAlphaDown, dhSortAlphaUp } from '@deephaven/icons';
import type { Column } from '@deephaven/jsapi-types';
import { TableUtils } from '@deephaven/jsapi-utils';
import { Aggregation } from './Aggregations';
import { filterValidColumns } from './AggregationUtils';
import './AggregationEdit.scss';
import { DisplayColumn } from '../../IrisGridModel';

interface AggregationEditItem {
value: string;
Expand All @@ -16,7 +16,7 @@ interface AggregationEditItem {

export type AggregationEditProps = {
aggregation: Aggregation;
columns: readonly Column[];
columns: readonly DisplayColumn[];
onChange: (aggregation: Aggregation) => void;
};

Expand Down
11 changes: 7 additions & 4 deletions packages/iris-grid/src/sidebar/aggregations/AggregationUtils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { Column } from '@deephaven/jsapi-types';
import { TableUtils } from '@deephaven/jsapi-utils';
import { DisplayColumn } from '../../IrisGridModel';
import AggregationOperation from './AggregationOperation';

export const SELECTABLE_OPTIONS = [
Expand Down Expand Up @@ -68,12 +68,15 @@ export const isValidOperation = (
};

export const filterValidColumns = (
columns: readonly Column[],
columns: readonly DisplayColumn[],
operationType: AggregationOperation
): Column[] => columns.filter(c => isValidOperation(operationType, c.type));
): DisplayColumn[] =>
columns.filter(
c => c.isProxy !== true && isValidOperation(operationType, c.type)
);

export const getOperationColumnNames = (
columns: readonly Column[],
columns: readonly DisplayColumn[],
operationType: AggregationOperation,
selected: readonly string[],
invert: boolean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import { Button, SearchInput } from '@deephaven/components';
import clamp from 'lodash.clamp';
import throttle from 'lodash.throttle';
import './VisibilityOrderingBuilder.scss';
import IrisGridModel from '../../IrisGridModel';
import IrisGridModel, { DisplayColumn } from '../../IrisGridModel';
import { ColumnName } from '../../CommonTypes';
import ColumnHeaderGroup from '../../ColumnHeaderGroup';
import VisibilityOrderingItem from './VisibilityOrderingItem';
Expand Down Expand Up @@ -92,6 +92,11 @@ class VisibilityOrderingBuilder extends PureComponent<
DOWN: 'DOWN',
} as const;

static shouldRenderColumn(column: DisplayColumn): boolean {
// We don't want to render the proxy column in the visibility ordering list
return column.isProxy !== true;
}

constructor(props: VisibilityOrderingBuilderProps) {
super(props);

Expand Down Expand Up @@ -1081,7 +1086,10 @@ class VisibilityOrderingBuilder extends PureComponent<
}

makeVisibilityOrderingList = memoize(
(columns: readonly Column[], treeItems: readonly IrisGridTreeItem[]) => {
(
columns: readonly DisplayColumn[],
treeItems: readonly IrisGridTreeItem[]
) => {
const { movedColumns } = this.props;

const elements = [];
Expand Down Expand Up @@ -1117,7 +1125,9 @@ class VisibilityOrderingBuilder extends PureComponent<
movedColumns
);
const column = columns[modelIndex];
elements.push(this.renderImmovableItem(column.name));
if (VisibilityOrderingBuilder.shouldRenderColumn(column)) {
elements.push(this.renderImmovableItem(column.name));
}
}

return elements;
Expand All @@ -1131,7 +1141,9 @@ class VisibilityOrderingBuilder extends PureComponent<
) {
const modelIndex = GridUtils.getModelIndex(visibleIndex, movedColumns);
const column = columns[modelIndex];
elements.push(this.renderImmovableItem(column.name));
if (VisibilityOrderingBuilder.shouldRenderColumn(column)) {
elements.push(this.renderImmovableItem(column.name));
}
}

if (firstMovableIndex !== null && firstMovableIndex > 0) {
Expand Down Expand Up @@ -1159,7 +1171,9 @@ class VisibilityOrderingBuilder extends PureComponent<
) {
const modelIndex = GridUtils.getModelIndex(visibleIndex, movedColumns);
const column = columns[modelIndex];
elements.push(this.renderImmovableItem(column.name));
if (VisibilityOrderingBuilder.shouldRenderColumn(column)) {
elements.push(this.renderImmovableItem(column.name));
}
}

return elements;
Expand Down

0 comments on commit e3667e5

Please sign in to comment.