Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mofojed opened this issue Jun 21, 2024 · 6 comments · Fixed by #2155 or #2181
Closed

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

mofojed opened this issue Jun 21, 2024 · 6 comments · Fixed by #2155 or #2181
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@mofojed
Copy link
Member

mofojed commented Jun 21, 2024

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: #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.

@mofojed mofojed added enhancement New feature or request triage Issue requires triage labels Jun 21, 2024
@alexbassy
Copy link
Contributor

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
Copy link
Member Author

mofojed commented Jun 21, 2024

@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
Copy link
Collaborator

mattrunyon commented Jun 21, 2024

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
Copy link
Collaborator

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 mofojed removed the triage Issue requires triage label Jun 25, 2024
@mofojed mofojed added this to the July 2024 milestone Jun 25, 2024
@mofojed
Copy link
Member Author

mofojed commented Jul 17, 2024

@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: #1982

AkshatJawne added a commit that referenced this issue Jul 18, 2024
Closes #2093 (For now, will reopen ticket after merge for better
solution to handle deprecated `panel` prop)
@AkshatJawne
Copy link
Contributor

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

@AkshatJawne AkshatJawne reopened this Jul 18, 2024
mofojed pushed a commit that referenced this issue Jul 23, 2024
Closes #2093 (For now, will reopen ticket after merge for better
solution to handle deprecated `panel` prop)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants