From fd55bda2fb568ae39d3eba2c58d60b9f21dfc3a8 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Tue, 24 Jun 2025 10:49:05 -0500 Subject: [PATCH 1/6] DH-19292: Add ui.table selection event --- plugins/ui/docs/components/table.md | 26 ++++- .../ui/src/deephaven/ui/components/table.py | 4 + plugins/ui/src/deephaven/ui/types/types.py | 1 + .../src/js/src/elements/UITable/UITable.tsx | 52 ++++++--- .../UITable/UITableContextMenuHandler.ts | 3 +- .../elements/UITable/UITableMouseHandler.ts | 50 +------- .../{UITableUtils.tsx => UITableUtils.ts} | 110 ++++++++++++++++++ 7 files changed, 179 insertions(+), 67 deletions(-) rename plugins/ui/src/js/src/elements/UITable/{UITableUtils.tsx => UITableUtils.ts} (55%) diff --git a/plugins/ui/docs/components/table.md b/plugins/ui/docs/components/table.md index df099eb98..47a063769 100644 --- a/plugins/ui/docs/components/table.md +++ b/plugins/ui/docs/components/table.md @@ -200,7 +200,9 @@ t_top = ui.table( ## Events -You can listen for different user events on a `ui.table`. There is both a `press` and `double_press` event for `row`, `cell`, and `column`. These events typically correspond to a click or double click on the table. The event payloads include table data related to the event. For `row` and `column` events, the corresponding data within the viewport will be sent to the event handler. The viewport is typically the visible area ± a window equal to the visible area (e.g., if rows 5-10 are visible, rows 0-15 will be in the viewport). +### Press Events + +You can listen for different user press events on a `ui.table`. There is both a `press` and `double_press` event for `row`, `cell`, and `column`. These events typically correspond to a click or double click on the table. The event payloads include table data related to the event. For `row` and `column` events, the corresponding data within the viewport will be sent to the event handler. The viewport is typically the visible area ± a window equal to the visible area (e.g., if rows 5-10 are visible, rows 0-15 will be in the viewport). Note that there is no row index in event data because the row index is not a safe way to reference a row between the client and server since the user could have manipulated the table, resulting in a different client order. @@ -223,6 +225,26 @@ t = ui.table( ) ``` +### Selection Event + +The `on_selection_change` event is triggered when the user selects or deselects a row. The event data will contain all selected rows within the viewport as a list of dictionaries keyed by column name. There are a few caveats to the selection event. + +1. The event will **only** send data from columns in the `always_fetch_columns` prop. +2. The event will **only** send data from rows that are visible in the viewport. +3. The event will **not** be triggered if a ticking table row is replaced or shifted. This may cause what the user sees after row shifts to differ from the selection event data. + +With these caveats in mind, it is highly recommended that the `on_selection_change` event be used only with static tables. It is also recommended to only use this event for relatively small actions where you can see all selected rows at once. + +```python +from deephaven import ui +import deephaven.plot.express as dx + +t = ui.table( + dx.data.stocks(), + on_selection_change=lambda data: print(f"Selection: {data}"), +) +``` + ## Context menu Items can be added to the bottom of the `ui.table` context menu (right-click menu) by using the `context_menu` or `context_header_menu` props. The `context_menu` prop adds items to the cell context menu, while the `context_header_menu` prop adds items to the column header context menu. You can pass either a single dictionary for a single item or a list of dictionaries for multiple items. @@ -391,7 +413,7 @@ Deephaven only fetches data for visible rows and columns within a window around The `always_fetch_columns` prop takes a single column name, a list of column names, or a boolean to always fetch all columns. The data for these columns is included in row event data (e.g. `on_row_press`) and context menu callbacks. -> [!WARNING] +> [!WARNING] > Setting `always_fetch_columns` to `True` will fetch all columns and can be slow for tables with many columns. This example shows how to use `always_fetch_columns` to always fetch the `Sym` column for a row press event. Without the `always_fetch_columns` prop, the press callback will fail because the `Sym` column is not fetched when hidden. diff --git a/plugins/ui/src/deephaven/ui/components/table.py b/plugins/ui/src/deephaven/ui/components/table.py index 10c8dde7b..941ebafb8 100644 --- a/plugins/ui/src/deephaven/ui/components/table.py +++ b/plugins/ui/src/deephaven/ui/components/table.py @@ -14,6 +14,7 @@ QuickFilterExpression, RowPressCallback, ResolvableContextMenuItem, + SelectionChangeCallback, ) from .._internal import dict_to_react_props, RenderContext @@ -144,6 +145,8 @@ class table(Element): The callback is invoked with the column name. on_column_double_press: The callback function to run when a column is double clicked. The callback is invoked with the column name. + on_selection_change: The callback function to run when the selection changes. + The callback is invoked with the selected rows with data from the columns in `always_fetch_columns`. always_fetch_columns: The columns to always fetch from the server regardless of if they are in the viewport. If True, all columns will always be fetched. This may make tables with many columns slow. quick_filters: The quick filters to apply to the table. Dictionary of column name to filter value. @@ -230,6 +233,7 @@ def __init__( on_cell_double_press: CellPressCallback | None = None, on_column_press: ColumnPressCallback | None = None, on_column_double_press: ColumnPressCallback | None = None, + on_selection_change: SelectionChangeCallback | None = None, always_fetch_columns: ColumnName | list[ColumnName] | bool | None = None, quick_filters: dict[ColumnName, QuickFilterExpression] | None = None, show_quick_filters: bool = False, diff --git a/plugins/ui/src/deephaven/ui/types/types.py b/plugins/ui/src/deephaven/ui/types/types.py index 729e1562d..bf3f62632 100644 --- a/plugins/ui/src/deephaven/ui/types/types.py +++ b/plugins/ui/src/deephaven/ui/types/types.py @@ -437,6 +437,7 @@ class SliderChange(TypedDict): RowPressCallback = Callable[[RowDataMap], None] CellPressCallback = Callable[[CellData], None] ColumnPressCallback = Callable[[ColumnName], None] +SelectionChangeCallback = Callable[[List[RowDataMap]], None] AggregationOperation = Literal[ "COUNT", "COUNT_DISTINCT", diff --git a/plugins/ui/src/js/src/elements/UITable/UITable.tsx b/plugins/ui/src/js/src/elements/UITable/UITable.tsx index 87f4706fc..e66100c57 100644 --- a/plugins/ui/src/js/src/elements/UITable/UITable.tsx +++ b/plugins/ui/src/js/src/elements/UITable/UITable.tsx @@ -36,13 +36,15 @@ import { useApi } from '@deephaven/jsapi-bootstrap'; import type { dh as DhType } from '@deephaven/jsapi-types'; import Log from '@deephaven/log'; import { getSettings, RootState } from '@deephaven/redux'; -import { GridMouseHandler, GridState } from '@deephaven/grid'; +import { GridMouseHandler, GridRange, GridState } from '@deephaven/grid'; import { EMPTY_ARRAY, ensureArray } from '@deephaven/utils'; +import { useDebouncedCallback } from '@deephaven/react-hooks'; import { usePersistentState } from '@deephaven/plugin'; import { DatabarConfig, FormattingRule, getAggregationOperation, + getSelectionDataMap, UITableProps, } from './UITableUtils'; import UITableMouseHandler from './UITableMouseHandler'; @@ -159,6 +161,7 @@ export function UITable({ onColumnDoublePress, onRowPress, onRowDoublePress, + onSelectionChange, quickFilters, sorts, aggregations, @@ -374,13 +377,8 @@ export function UITable({ return alwaysFetch; }, [format, columns]); - const alwaysFetchColumnsArray = useMemo( - () => [...ensureArray(alwaysFetchColumnsProp), ...formatColumnSources], - [alwaysFetchColumnsProp, formatColumnSources] - ); - - const alwaysFetchColumns = useMemo(() => { - if (alwaysFetchColumnsArray[0] === true) { + const alwaysFetchColumnsPropArray = useMemo(() => { + if (alwaysFetchColumnsProp === true) { if (columns.length > ALWAYS_FETCH_COLUMN_LIMIT) { throwError( `Table has ${columns.length} columns, which is too many to always fetch. ` + @@ -391,14 +389,16 @@ export function UITable({ } return columns.map(column => column.name); } - if (alwaysFetchColumnsArray[0] === false) { + if (alwaysFetchColumnsProp === false || alwaysFetchColumnsProp == null) { return []; } - return alwaysFetchColumnsArray.filter( - // This v is string can be removed when we're on a newer TS version. 5.7 infers this properly at least - (v): v is string => typeof v === 'string' - ); - }, [alwaysFetchColumnsArray, columns, throwError]); + return [...ensureArray(alwaysFetchColumnsProp)]; + }, [alwaysFetchColumnsProp, columns, throwError]); + + const alwaysFetchColumns = useMemo( + () => [...alwaysFetchColumnsPropArray, ...formatColumnSources], + [alwaysFetchColumnsPropArray, formatColumnSources] + ); const mouseHandlers = useMemo( () => @@ -512,6 +512,29 @@ export function UITable({ const initialIrisGridServerProps = useRef(irisGridServerProps); + const handleSelectionChanged = useCallback( + async (ranges: readonly GridRange[]) => { + if (model == null || irisGrid == null || onSelectionChange == null) { + return; + } + + const selected = getSelectionDataMap( + ranges, + model, + irisGrid, + alwaysFetchColumnsPropArray + ); + + onSelectionChange(selected); + }, + [irisGrid, model, onSelectionChange, alwaysFetchColumnsPropArray] + ); + + const debouncedHandleSelectionChanged = useDebouncedCallback( + handleSelectionChanged, + 250 + ); + /** * We want to set the props based on a combination of server state and client state. * If the server state is the same as its initial state, then we are rehydrating and @@ -561,6 +584,7 @@ export function UITable({ ref={ref => setIrisGrid(ref)} model={model} onStateChange={onStateChange} + onSelectionChanged={debouncedHandleSelectionChanged} // eslint-disable-next-line react/jsx-props-no-spreading {...mergedIrisGridProps} inputFilters={inputFilters} diff --git a/plugins/ui/src/js/src/elements/UITable/UITableContextMenuHandler.ts b/plugins/ui/src/js/src/elements/UITable/UITableContextMenuHandler.ts index 11642a7f5..c5f520244 100644 --- a/plugins/ui/src/js/src/elements/UITable/UITableContextMenuHandler.ts +++ b/plugins/ui/src/js/src/elements/UITable/UITableContextMenuHandler.ts @@ -12,10 +12,9 @@ import { import type { dh as DhType } from '@deephaven/jsapi-types'; import { type ColumnName } from '@deephaven/jsapi-utils'; import { ensureArray } from '@deephaven/utils'; -import { RowDataMap, type UITableProps } from './UITableUtils'; +import { getRowDataMap, RowDataMap, type UITableProps } from './UITableUtils'; import { getIcon } from '../utils/IconElementUtils'; import { ELEMENT_PREFIX, ElementPrefix } from '../model/ElementConstants'; -import { getRowDataMap } from './UITableMouseHandler'; interface UIContextItemParams { value: unknown; diff --git a/plugins/ui/src/js/src/elements/UITable/UITableMouseHandler.ts b/plugins/ui/src/js/src/elements/UITable/UITableMouseHandler.ts index 6a182a68e..fb6c69869 100644 --- a/plugins/ui/src/js/src/elements/UITable/UITableMouseHandler.ts +++ b/plugins/ui/src/js/src/elements/UITable/UITableMouseHandler.ts @@ -2,57 +2,9 @@ import { EventHandlerResult, GridMouseHandler, GridPoint, - isExpandableGridModel, - type ModelIndex, } from '@deephaven/grid'; import { IrisGridModel, type IrisGridType } from '@deephaven/iris-grid'; -import { CellData, RowDataMap, UITableProps } from './UITableUtils'; - -function getCellData( - columnIndex: ModelIndex, - rowIndex: ModelIndex, - model: IrisGridModel -): CellData { - const column = model.columns[columnIndex]; - const { type } = column; - const value = model.valueForCell(columnIndex, rowIndex); - const text = model.textForCell(columnIndex, rowIndex); - return { - value, - text, - type, - }; -} - -/** - * Get the data map for the given row - * @param rowIndex Row to get the data map for - * @returns Data map for the row - */ -export function getRowDataMap( - rowIndex: ModelIndex, - model: IrisGridModel -): RowDataMap { - const { columns, groupedColumns } = model; - const dataMap: RowDataMap = {}; - for (let i = 0; i < columns.length; i += 1) { - const column = columns[i]; - const { name } = column; - const isExpandable = - isExpandableGridModel(model) && model.isRowExpandable(rowIndex); - const isGrouped = groupedColumns.find(c => c.name === name) != null; - const cellData = getCellData(i, rowIndex, model); - // If the cellData.value is undefined, that means we don't have any data for that column (i.e. the column is not visible), don't send it back - if (cellData.value !== undefined) { - dataMap[name] = { - ...cellData, - isGrouped, - isExpandable, - }; - } - } - return dataMap; -} +import { getCellData, getRowDataMap, UITableProps } from './UITableUtils'; /** * Mouse handler for UITable. Will call the appropriate callbacks when a cell, row, or column is clicked or double clicked with the data structure expected. diff --git a/plugins/ui/src/js/src/elements/UITable/UITableUtils.tsx b/plugins/ui/src/js/src/elements/UITable/UITableUtils.ts similarity index 55% rename from plugins/ui/src/js/src/elements/UITable/UITableUtils.tsx rename to plugins/ui/src/js/src/elements/UITable/UITableUtils.ts index d66ce7187..4137ca6bd 100644 --- a/plugins/ui/src/js/src/elements/UITable/UITableUtils.tsx +++ b/plugins/ui/src/js/src/elements/UITable/UITableUtils.ts @@ -4,7 +4,16 @@ import { type ColumnName, type DehydratedSort, AggregationOperation, + type IrisGridModel, + IrisGridType, } from '@deephaven/iris-grid'; +import { + BoundedGridRange, + GridRange, + isExpandableGridModel, + type ModelIndex, +} from '@deephaven/grid'; +import { assertNotNull } from '@deephaven/utils'; import { ELEMENT_KEY, type ElementNode, @@ -71,6 +80,7 @@ export type UITableProps = StyleProps & { onRowDoublePress?: (rowData: RowDataMap) => void; onColumnPress?: (columnName: ColumnName) => void; onColumnDoublePress?: (columnName: ColumnName) => void; + onSelectionChange?: (selectedRows: RowDataMap[]) => void; alwaysFetchColumns?: string | string[] | boolean; quickFilters?: Record; sorts?: DehydratedSort[]; @@ -127,3 +137,103 @@ export function getAggregationOperation(agg: string): AggregationOperation { return operation; } + +export function getCellData( + columnIndex: ModelIndex, + rowIndex: ModelIndex, + model: IrisGridModel +): CellData { + const column = model.columns[columnIndex]; + const { type } = column; + const value = model.valueForCell(columnIndex, rowIndex); + const text = model.textForCell(columnIndex, rowIndex); + return { + value, + text, + type, + }; +} + +/** + * Get the data map for the given row + * @param rowIndex Row to get the data map for + * @param model The IrisGridModel to get the data from + * @param columnNames Optional array of column names to filter the data map. + * If not provided, all columns in the viewport will be included. + * @returns Data map for the row + */ +export function getRowDataMap( + rowIndex: ModelIndex, + model: IrisGridModel, + columnNames?: string[] +): RowDataMap { + const { columns, groupedColumns } = model; + const columnNamesSet = new Set(columnNames); + const dataMap: RowDataMap = {}; + for (let i = 0; i < columns.length; i += 1) { + const column = columns[i]; + const { name } = column; + if (columnNames == null || columnNamesSet.has(name)) { + const isExpandable = + isExpandableGridModel(model) && model.isRowExpandable(rowIndex); + const isGrouped = groupedColumns.find(c => c.name === name) != null; + const cellData = getCellData(i, rowIndex, model); + // If the cellData.value is undefined, that means we don't have any data for that column (i.e. the column is not visible), don't send it back + if (cellData.value !== undefined) { + dataMap[name] = { + ...cellData, + isGrouped, + isExpandable, + }; + } + } + } + + return dataMap; +} + +export function getSelectionDataMap( + ranges: readonly GridRange[], + model: IrisGridModel, + irisGrid: IrisGridType, + columnNames?: string[] +): RowDataMap[] { + const dataMaps: RowDataMap[] = []; + + const boundedSortedRanges = ( + GridRange.boundedRanges( + GridRange.consolidate(ranges), + model.columnCount, + model.rowCount + ) as BoundedGridRange[] + ).sort((a, b) => a.startRow - b.startRow); // Ensure we're in ascending order by row + + const { metrics } = irisGrid.state; + assertNotNull(metrics); + const { top, bottomViewport } = metrics; + + for (let i = 0; i < boundedSortedRanges.length; i += 1) { + const visibleRange = GridRange.intersection( + boundedSortedRanges[i], + GridRange.makeNormalized(null, top, null, bottomViewport) + ) as BoundedGridRange; + + if (visibleRange == null) { + // eslint-disable-next-line no-continue + continue; + } + + for ( + let row = visibleRange.startRow; + row <= visibleRange.endRow; + row += 1 + ) { + const modelRow = irisGrid.getModelRow(row); + if (modelRow != null) { + const rowDataMap = getRowDataMap(modelRow, model, columnNames); + dataMaps.push(rowDataMap); + } + } + } + return dataMaps; +} From 3e7d05df0e85194b14619edc004f87afbd8f10a6 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Tue, 24 Jun 2025 11:33:41 -0500 Subject: [PATCH 2/6] Add snapshot --- plugins/ui/docs/components/table.md | 1 + plugins/ui/docs/snapshots/3ed9b83cce3a4e67bd180e96b2b62d45.json | 1 + 2 files changed, 2 insertions(+) create mode 100644 plugins/ui/docs/snapshots/3ed9b83cce3a4e67bd180e96b2b62d45.json diff --git a/plugins/ui/docs/components/table.md b/plugins/ui/docs/components/table.md index 47a063769..046ecfa2e 100644 --- a/plugins/ui/docs/components/table.md +++ b/plugins/ui/docs/components/table.md @@ -242,6 +242,7 @@ import deephaven.plot.express as dx t = ui.table( dx.data.stocks(), on_selection_change=lambda data: print(f"Selection: {data}"), + always_fetch_columns=["Sym", "Exchange"], ) ``` diff --git a/plugins/ui/docs/snapshots/3ed9b83cce3a4e67bd180e96b2b62d45.json b/plugins/ui/docs/snapshots/3ed9b83cce3a4e67bd180e96b2b62d45.json new file mode 100644 index 000000000..bf6b51155 --- /dev/null +++ b/plugins/ui/docs/snapshots/3ed9b83cce3a4e67bd180e96b2b62d45.json @@ -0,0 +1 @@ +{"file":"components/table.md","objects":{"t":{"type":"deephaven.ui.Element","data":{"document":{"__dhElemName":"deephaven.ui.elements.UITable","props":{"table":{"__dhObid":0},"onSelectionChange":{"__dhCbid":"cb0"},"alwaysFetchColumns":["Sym","Exchange"],"showQuickFilters":false,"showGroupingColumn":true,"showSearch":false,"reverse":false}},"state":"{}"}}}} \ No newline at end of file From 3ae81edaa3c46c5c61e8d24c55386cc26300e306 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Wed, 25 Jun 2025 01:32:53 -0500 Subject: [PATCH 3/6] Add e2e test --- tests/app.d/ui_table.py | 8 ++++++++ tests/ui_table.spec.ts | 20 +++++++++++++++++++- tests/utils.ts | 24 ++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 1 deletion(-) diff --git a/tests/app.d/ui_table.py b/tests/app.d/ui_table.py index 7647439e9..e72c25a70 100644 --- a/tests/app.d/ui_table.py +++ b/tests/app.d/ui_table.py @@ -173,3 +173,11 @@ def toggle_table_component(): _stocks, aggregations=ui.TableAgg("sum"), ) + +t_selection = ui.table( + _stocks, + on_selection_change=lambda d: print( + "Selection:", [[row["Sym"]["text"], row["Exchange"]["text"]] for row in d] + ), + always_fetch_columns=["Sym", "Exchange"], +) diff --git a/tests/ui_table.spec.ts b/tests/ui_table.spec.ts index 8c84969fc..7f44a1aef 100644 --- a/tests/ui_table.spec.ts +++ b/tests/ui_table.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from '@playwright/test'; -import { openPanel, gotoPage } from './utils'; +import { openPanel, gotoPage, clickGridRow } from './utils'; const REACT_PANEL_VISIBLE = '.dh-react-panel:visible'; @@ -40,3 +40,21 @@ test('UI table responds to prop changes', async ({ page }) => { await locator.getByRole('button', { name: 'case' }).click(); await expect(locator).toHaveScreenshot(); }); + +test('UI table on_selection_change', async ({ page }) => { + await gotoPage(page, ''); + await openPanel(page, 't_selection', REACT_PANEL_VISIBLE); + + const locator = page.locator(REACT_PANEL_VISIBLE); + + await clickGridRow(locator, 3); + await expect(page.getByText("Selection: [['CAT', 'NYPE']]")).toBeVisible(); + + await clickGridRow(locator, 0, { modifiers: ['ControlOrMeta'] }); + await expect( + page.getByText("Selection: [['FISH', 'TPET'], ['CAT', 'NYPE']]") + ).toBeVisible(); + + await page.keyboard.press('Escape'); + await expect(page.getByText('Selection: []')).toBeVisible(); +}); diff --git a/tests/utils.ts b/tests/utils.ts index 184a7459b..4715888cb 100644 --- a/tests/utils.ts +++ b/tests/utils.ts @@ -7,6 +7,9 @@ export const SELECTORS = { REACT_PANEL_OVERLAY: '.dh-react-panel-overlay', }; +const ROW_HEIGHT = 19; +const COLUMN_HEADER_HEIGHT = 30; + /** * Goes to a page and waits for the progress bar to disappear * @param page The page @@ -168,3 +171,24 @@ export async function pasteInMonaco( await expect(locator.locator('textarea')).not.toBeEmpty(); } } + +/** + * Clicks the specified row for the grid. + * Clicks in the first column of the row as column width is variable. + * Assumes there is only one level of column headers (i.e., no column groups). + * @param gridContainer The Playwright Locator of the grid container + * @param row The row index to click + * @param clickOptions The Locator click options such as modifies to use + */ +export async function clickGridRow( + gridContainer: Locator, + row: number, + clickOptions?: Parameters[0] +): Promise { + const x = 1; + const y = COLUMN_HEADER_HEIGHT + (row + 0.5) * ROW_HEIGHT; + await gridContainer.click({ + ...clickOptions, + position: { x, y }, + }); +} From 44c5a808c6a5ebfeb3b0bf8fe57473262150497b Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Wed, 25 Jun 2025 02:20:56 -0500 Subject: [PATCH 4/6] Fix e2e --- tests/app.d/ui_table.py | 32 +++++++++++++++++++++++++------- tests/ui_table.spec.ts | 8 +++----- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/tests/app.d/ui_table.py b/tests/app.d/ui_table.py index e72c25a70..713fa1c1a 100644 --- a/tests/app.d/ui_table.py +++ b/tests/app.d/ui_table.py @@ -174,10 +174,28 @@ def toggle_table_component(): aggregations=ui.TableAgg("sum"), ) -t_selection = ui.table( - _stocks, - on_selection_change=lambda d: print( - "Selection:", [[row["Sym"]["text"], row["Exchange"]["text"]] for row in d] - ), - always_fetch_columns=["Sym", "Exchange"], -) + +@ui.component +def t_selection_component(): + selection, set_selection = ui.use_state([]) + selection_str = ( + ", ".join( + [f"{row['Sym']['text']}/{row['Exchange']['text']}" for row in selection] + ) + if len(selection) > 0 + else "None" + ) + return ui.flex( + ui.text( + f"Selection: {selection_str}", + ), + ui.table( + _stocks, + on_selection_change=lambda d: set_selection(d), + always_fetch_columns=["Sym", "Exchange"], + ), + direction="column", + ) + + +t_selection = t_selection_component() diff --git a/tests/ui_table.spec.ts b/tests/ui_table.spec.ts index 7f44a1aef..e96b0813a 100644 --- a/tests/ui_table.spec.ts +++ b/tests/ui_table.spec.ts @@ -48,13 +48,11 @@ test('UI table on_selection_change', async ({ page }) => { const locator = page.locator(REACT_PANEL_VISIBLE); await clickGridRow(locator, 3); - await expect(page.getByText("Selection: [['CAT', 'NYPE']]")).toBeVisible(); + await expect(page.getByText('Selection: CAT/NYPE')).toBeVisible(); await clickGridRow(locator, 0, { modifiers: ['ControlOrMeta'] }); - await expect( - page.getByText("Selection: [['FISH', 'TPET'], ['CAT', 'NYPE']]") - ).toBeVisible(); + await expect(page.getByText('Selection: FISH/TPET, CAT/NYPE')).toBeVisible(); await page.keyboard.press('Escape'); - await expect(page.getByText('Selection: []')).toBeVisible(); + await expect(page.getByText('Selection: None')).toBeVisible(); }); From bb730756c0c6f7f073f145a01703f9f61cfd980c Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Wed, 25 Jun 2025 10:44:34 -0500 Subject: [PATCH 5/6] Fix e2e for real --- tests/ui_table.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui_table.spec.ts b/tests/ui_table.spec.ts index e96b0813a..9b32434b1 100644 --- a/tests/ui_table.spec.ts +++ b/tests/ui_table.spec.ts @@ -45,7 +45,7 @@ test('UI table on_selection_change', async ({ page }) => { await gotoPage(page, ''); await openPanel(page, 't_selection', REACT_PANEL_VISIBLE); - const locator = page.locator(REACT_PANEL_VISIBLE); + const locator = page.locator(`${REACT_PANEL_VISIBLE} .iris-grid`); await clickGridRow(locator, 3); await expect(page.getByText('Selection: CAT/NYPE')).toBeVisible(); From 792cc082a21c9f30828c5ec9e90f978146a26e55 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Mon, 30 Jun 2025 16:28:02 -0500 Subject: [PATCH 6/6] Throw when missing always_fetch_columns --- .../ui/src/deephaven/ui/components/table.py | 5 ++++ plugins/ui/test/deephaven/ui/test_ui_table.py | 27 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/plugins/ui/src/deephaven/ui/components/table.py b/plugins/ui/src/deephaven/ui/components/table.py index 941ebafb8..1766737b6 100644 --- a/plugins/ui/src/deephaven/ui/components/table.py +++ b/plugins/ui/src/deephaven/ui/components/table.py @@ -293,6 +293,11 @@ def __init__( right: DimensionValue | None = None, z_index: int | None = None, ) -> None: + if on_selection_change is not None and always_fetch_columns is None: + raise ValueError( + "ui.table on_selection_change requires always_fetch_columns to be set" + ) + props = locals() del props["self"] self._props = props diff --git a/plugins/ui/test/deephaven/ui/test_ui_table.py b/plugins/ui/test/deephaven/ui/test_ui_table.py index d858b5acf..a2f6e1ab9 100644 --- a/plugins/ui/test/deephaven/ui/test_ui_table.py +++ b/plugins/ui/test/deephaven/ui/test_ui_table.py @@ -226,3 +226,30 @@ def test_column_groups(self): ], }, ) + + def test_on_selection_change(self): + import deephaven.ui as ui + + on_change = Mock() + + self.assertRaises( + ValueError, + lambda: ui.table( + self.source, + on_selection_change=on_change, + ), + ) + + t = ui.table( + self.source, + on_selection_change=on_change, + always_fetch_columns=["X"], + ) + + self.expect_render( + t, + { + "alwaysFetchColumns": ["X"], + "onSelectionChange": on_change, + }, + )