From e3b61840c14c467a525803f5cfde58e05d7e29c9 Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Thu, 12 Sep 2024 10:03:18 -0400 Subject: [PATCH] fix: Use correct offset in snapshot (#2217) - Was using `row.offsetInSnapshot`, which was a "protected" API and recently removed from the JS API - API was removed in JS API refactoring: https://github.com/deephaven/deephaven-core/pull/5890 - Instead just use the viewport `offset` and add the index of the row in the snapshot, which is there in both versions of the API - New API does support `row.index`, but this way is compatible with both - Updated unit tests - Tested using a deephaven.ui.list_view: ```python from deephaven import time_table, ui import datetime initial_row_count = 200 column_types = time_table( "PT1S", start_time=datetime.datetime.now() - datetime.timedelta(seconds=initial_row_count), ).update( [ "Id=new Integer(i)", "Display=new String(`Display `+i)", ] ) @ui.component def ui_list_view_table(): value, set_value = ui.use_state([]) lv = ui.list_view( column_types, aria_label="List View", on_change=set_value, selected_keys=value, ) text = ui.text("Selection: " + ", ".join(map(str, value))) return ui.flex( lv, text, direction="column", margin=10, gap=10, width=500, # necessary to avoid overflowing container height min_height=0, ) lv_table = ui_list_view_table() ``` (cherry picked from commit a479d6c5f907f53aaa6500845ea168ab0eb9bb09) --- .../src/useViewportData.test.tsx | 11 ++---- .../jsapi-utils/src/ViewportDataUtils.test.ts | 36 +++++-------------- packages/jsapi-utils/src/ViewportDataUtils.ts | 27 +++----------- 3 files changed, 17 insertions(+), 57 deletions(-) diff --git a/packages/jsapi-components/src/useViewportData.test.tsx b/packages/jsapi-components/src/useViewportData.test.tsx index 2072f44361..7cd93cde06 100644 --- a/packages/jsapi-components/src/useViewportData.test.tsx +++ b/packages/jsapi-components/src/useViewportData.test.tsx @@ -3,7 +3,6 @@ import type { dh as DhType } from '@deephaven/jsapi-types'; import dh from '@deephaven/jsapi-shim'; import { OnTableUpdatedEvent, - ViewportRow, generateEmptyKeyedItems, isClosed, ITEM_KEY_PREFIX, @@ -23,13 +22,9 @@ jest.mock('@deephaven/react-hooks', () => ({ jest.mock('./useSetPaddedViewportCallback'); jest.mock('./useTableSize'); -function mockViewportRow(offsetInSnapshot: number): ViewportRow { - return { offsetInSnapshot } as ViewportRow; -} - function mockUpdateEvent( offset: number, - rows: ViewportRow[] + rows: DhType.Row[] ): OnTableUpdatedEvent { return { detail: { @@ -213,14 +208,14 @@ it('should update state on dh.Table.EVENT_UPDATED event', () => { ); const offset = 3; - const row = mockViewportRow(5); + const row = TestUtils.createMockProxy(); const event = mockUpdateEvent(offset, [row]); act(() => { updateEventHandler?.(event); }); - const expectedKeyIndex = offset + row.offsetInSnapshot; + const expectedKeyIndex = offset; const expectedInitialItems = [...generateEmptyKeyedItems(0, table.size - 1)]; const expectedItems = [ ...expectedInitialItems.slice(0, expectedKeyIndex), diff --git a/packages/jsapi-utils/src/ViewportDataUtils.test.ts b/packages/jsapi-utils/src/ViewportDataUtils.test.ts index 0bdc658b9d..ba5f3b62e5 100644 --- a/packages/jsapi-utils/src/ViewportDataUtils.test.ts +++ b/packages/jsapi-utils/src/ViewportDataUtils.test.ts @@ -5,8 +5,6 @@ import { ITEM_KEY_PREFIX, OnTableUpdatedEvent, RowDeserializer, - ViewportRow, - createKeyFromOffsetRow, createOnTableUpdatedHandler, defaultRowDeserializer, generateEmptyKeyedItems, @@ -18,10 +16,6 @@ import { const { asMock, createMockProxy } = TestUtils; -function mockViewportRow(offsetInSnapshot: number): ViewportRow { - return { offsetInSnapshot } as ViewportRow; -} - function mockColumn(name: string) { return { name, @@ -44,28 +38,15 @@ describe('createdKeyedItemKey', () => { }); }); -describe('createKeyFromOffsetRow', () => { - it.each([ - [{ offsetInSnapshot: 4 } as ViewportRow, 5, `${ITEM_KEY_PREFIX}_9`], - [{ offsetInSnapshot: 27 } as ViewportRow, 99, `${ITEM_KEY_PREFIX}_126`], - ] as const)( - 'should create a string key based on the actual row offset: %o', - (row, offset, expected) => { - const actual = createKeyFromOffsetRow(row, offset); - expect(actual).toEqual(expected); - } - ); -}); - describe('createOnTableUpdatedHandler', () => { const mock = { deserializeRow: jest.fn() as RowDeserializer, rows: [ - createMockProxy({ offsetInSnapshot: 0 }), - createMockProxy({ offsetInSnapshot: 1 }), - createMockProxy({ offsetInSnapshot: 2 }), + createMockProxy(), + createMockProxy(), + createMockProxy(), ], - updateEvent: (offset: number, rows: ViewportRow[], columns: dh.Column[]) => + updateEvent: (offset: number, rows: dh.Row[], columns: dh.Column[]) => createMockProxy({ detail: { offset, @@ -107,10 +88,11 @@ describe('createOnTableUpdatedHandler', () => { describe('defaultRowDeserializer', () => { it('should map all columns with original names', () => { - const row = mockViewportRow(10); - // mock our get function by mapping capital column name to lowercase value - // e.g. A: 'a' - row.get = jest.fn(({ name }: { name: string }) => name.toLowerCase()); + const row = createMockProxy({ + // mock our get function by mapping capital column name to lowercase value + // e.g. A: 'a' + get: jest.fn(({ name }: { name: string }) => name.toLowerCase()), + }); const actual = defaultRowDeserializer(row, [ mockColumn('A'), diff --git a/packages/jsapi-utils/src/ViewportDataUtils.ts b/packages/jsapi-utils/src/ViewportDataUtils.ts index e3d4a9b92d..db833005e9 100644 --- a/packages/jsapi-utils/src/ViewportDataUtils.ts +++ b/packages/jsapi-utils/src/ViewportDataUtils.ts @@ -9,12 +9,10 @@ export const ITEM_KEY_PREFIX = 'DH_ITEM_KEY'; export type OnTableUpdatedEvent = CustomEvent<{ offset: number; columns: dh.Column[]; - rows: ViewportRow[]; + rows: dh.Row[]; }>; -export type RowDeserializer = (row: ViewportRow, columns: dh.Column[]) => T; - -export type ViewportRow = dh.Row & { offsetInSnapshot: number }; +export type RowDeserializer = (row: dh.Row, columns: dh.Column[]) => T; const log = Log.module('ViewportDataUtils'); @@ -29,21 +27,6 @@ export function createKeyedItemKey(index: number): string { return `${ITEM_KEY_PREFIX}_${index}`; } -/** - * Create a unique string key for a row based on its ordinal position in its - * source table. This is calculated based on it's offset in the viewport - * (row.offsetInSnapshot) + the offset of the snapshot. - * @param row Row from a Table update event. - * @param offset Offset of the current viewport. - * @returns Unique string key for the ordinal position of the given row. - */ -export function createKeyFromOffsetRow( - row: ViewportRow, - offset: number -): string { - return createKeyedItemKey(row.offsetInSnapshot + offset); -} - /** * Creates a handler function for a `dh.Table.EVENT_UPDATED` event. Rows that * get passed to the handler will be bulk updated in the given `viewportData` @@ -66,9 +49,9 @@ export function createOnTableUpdatedHandler( const updateKeyMap = new Map>(); - rows.forEach(row => { + rows.forEach((row, offsetInSnapshot) => { const item = deserializeRow(row, columns); - const key = createKeyFromOffsetRow(row, offset); + const key = createKeyedItemKey(offset + offsetInSnapshot); updateKeyMap.set(key, { key, item }); }); @@ -86,7 +69,7 @@ export function createOnTableUpdatedHandler( * @returns A key / value object for the row. */ export function defaultRowDeserializer( - row: ViewportRow, + row: dh.Row, columns: dh.Column[] ): T { return columns.reduce((result, col) => {