-
Notifications
You must be signed in to change notification settings - Fork 16
feat: DH-18281 Input Filters UI Component #1165
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
base: main
Are you sure you want to change the base?
Conversation
ui docs preview (Available for 14 days) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, which is great, but I don't like having to mount the input_filters
component to get this to work... if you want input filters in multiple components it's resending the same list multiple times on change. To do this right, I think we need to have the RenderContext itself manage an input filter state from the client. We already have a set_state
function, I think we'd need a set_input_filters
command... and would probably want the component to also notify with subscribe/unsubscribe so it doesn't send it unnecessarily...
The more I think of this, the more I want to break the problem down into two categories:
- Existing one_click functionality: Getting the same functionality we have with Java charts and
oneClick
, where the user is prompted to add a filter before being able to view the chart. - Using the clients input filters from their dashboard within your deephaven.ui component (e.g. a
use_dashboard_inputs
hook, or as you've got here theinput_filters
component).
By doing 2, we can also do 1. Another advantage of 2 is that you don't need to change Deephaven Express or any other widgets, since it's re-rendering them entirely with a new table. On the downside, it's a little more syntax heavy, and it requires some architectural changes in how we manage deephaven.ui rendering (as we need to pass the client info along). That architectural change does open up interesting possibilities (e.g. themeing, formatting, etc)...
That all being said - I think we're straying away from the initial goal here, which is to just require some filters to be set on a table before attempting to plot it. My concern is that we're making this too complicated/confusing, when we really just need 1. Our main argument for avoiding 1 is that it would require changing Deephaven Express, every widget would need to build special support for it. But I think we could do 1 with minimal changes to existing one_clicks/deephaven express, and ultimately we'd be the better for it - we're re-using a lot of existing infrastructure. Then we can also do 2.
Right now a big problem with what's returned from the one_click
API is that it's just a Java object, and you can't access any methods. If you could access the table definition (like you can from Java), then we'd be good. For Deephaven Express, all we really need is the schema/table definition on the Python side, the rest is applied client side. So if we accept anything with a TableDefinition
(rather than requiring a whole table), then we are good for the most part.
And the SelectableDataSet
that is returned from one_click
does have that: https://github.com/deephaven/deephaven-core/blob/main/Plot/src/main/java/io/deephaven/plot/filters/SelectableDataSet.java
So we should be able to get that in Deephaven Express by calling toc.j_object.getTableDefinition()
. Which I think the only spot we really need info from the table is generating the mappings here:
deephaven-plugins/plugins/plotly-express/src/deephaven/plot/express/deephaven_figure/generate.py
Line 162 in 01693d7
for col in table.columns: |
So could just check if it's a selectable data set there and just get the table definition instead.
I think we should look at that first, as it may not actually be a lot of work to do 1 and then we can do 2 which leads to more interesting/powerful cases with deephaven.ui and custom widgets, but is more difficult to implement/more expensive.
); | ||
|
||
const tableColumns = useTableColumns(exportedTable); | ||
const columnsString = JSON.stringify(columnNames); // TODO workaround for changing columnNames reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The columnNames
shouldn't be changing, that's something I'll need to look into.
); | ||
|
||
useEffect(() => { | ||
const id = nanoid(); // TODO use widget id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the widgetId is used in DashboardWidgetHandler
, but isn't passed down. That could be passed down in a context - it would probably make sense to just pass it down to WidgetHandler
and then WidgetHandler
adds it to the WidgetStatus
(which can then be accessed with useWidgetStatus
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the issue wasn't getting widget id. Originally, I was trying to get a ref to the panel
. Then I started a refactor to have the event use panelId
and you suggested widgetId
:
@dgodinez-dh I have a branch modifying the Python side of Deephaven Express to allow passing in the SelectableDataSet.. there'd still need to changes to transport that SelectableDataSet object from the server to the client: 698d46c Or ... we could do something like just wrap the
Though that gets sticky because the python |
My main concern, and hence pushback into building the logic into |
Draft PR to discuss implementation.