deephaven / web-client-ui

Deephaven Web Client UI
Apache License 2.0
28 stars 30 forks source link

TablePlugin needs to know current grid selection, table name #2093

Closed mofojed closed 2 days ago

mofojed commented 1 month ago

As a developer of a TablePlugin, I would like to be able to get the table name and the current selection state from within my plugin. Previously we could do this with the panel prop that was passed in before it was removed: https://github.com/deephaven/web-client-ui/pull/1982 Could use panel.getTableName() to get the table name and panel.irisGrid.current.state to get the selected range of rows. Would like to still be able to do that.

alexbassy commented 1 month ago

Thanks for creating the issue @mofojed. For context, I use a table plugin to add custom menu items when right clicking on a row or selection of rows.

Use cases:

Table name

Used for filtering menu items depending on which table is being interacted with. E.g. if we're on the "addresses" table, we can add a menu item to "send a letter", without showing this menu item on all tables

Iris grid state

Used for applying a menu action to multiple rows. E.g. a table full of orders, I can select rows 0-10 and when I click "cancel order" (a custom menu item), the plugin can get those 10 rows and run a command on each one

mofojed commented 1 month ago

@alexbassy for the second case, it would make sense for the IrisGridContextMenuData that is provided to include the selection range. I think that would cover your use case if we add that? For table name, could you instead infer filtering the menu items based on the schema of the table/columns available? The problem with using a table name is that in some cases, the table may not even have a name (if it's provided as a child of a deephaven.ui panel for example). Or perhaps we could allow access to the table attributes and you could set an attribute for what kind of menu items should be available?

mattrunyon commented 1 month ago

One potential pitfall of the 2nd item is that tables could rearrange their rows or the user could have sorted/filtered causing the range to not match the underlying table if you're just trying to use row indices.

Consider a ticking table with .reverse() on the server. Rows 0-10 will always be the latest 10 rows.

The safest way to do this would include adding your own key column to the table, setting that column to always be fetched (by default we only fetch the columns on screen plus a buffer window), and then using that column value to figure out what rows you're operating on.

Even if the table is append only, a user could sort by a column and the row index would not match the original table.

mattrunyon commented 1 month ago

I should clarify the pitfall is mostly if you talk to the server at all. If you're keeping this all on the client, then it should mostly be fine. The row number can still be changed out from under you on the client if a table is sorted such that new data ticks in at the top. That is why you would want to use some key info about the row rather than row number to identify the rows.

mofojed commented 1 month ago

@AkshatJawne just add the removed panel prop back until we come up with a better solution (should still be marked deprecated). See PR that removed it: https://github.com/deephaven/web-client-ui/pull/1982

AkshatJawne commented 1 month ago

Merged in fix, but as described in PR, reopening ticket in order to look at more long-term solutions.