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

fix: Allow LayoutHints to be accessible from Python, TreeTables #5543

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 119 additions & 0 deletions py/server/deephaven/table.py
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,66 @@ def with_filters(self, filters: Union[str, Filter, Sequence[str], Sequence[Filte
except Exception as e:
raise DHError(e, "with_filters operation on RollupTable failed.") from e

def layout_hints(self, front: Union[str, List[str]] = None, back: Union[str, List[str]] = None,
freeze: Union[str, List[str]] = None, hide: Union[str, List[str]] = None,
column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> Table:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> Table:
column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> RollupTable:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dict isn't a great type hint because it doesn't indicate the contents of the dict. It should include the acceptable key and value types.

""" Sets layout hints on the RollupTable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pydocs do not mention the default behaviors.


Args:
front (Union[str, List[str]]): the columns to show at the front.
back (Union[str, List[str]]): the columns to show at the back.
freeze (Union[str, List[str]]): the columns to freeze to the front.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leaves a lot of questions for me:

  1. Is this separate from front?
  2. Do the frozen columns appear before the front columns?
  3. Do the frozen columns need to be listed in the front columns?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied these from the existing layout_hints method that is already on Table. Should I make updates there as well?
I'm just trying to get this supposed Enterprise blocker completed. Didn't expect/need to refactor a whole bunch of APIs, as this is supposed to be a hotfix - trying to change as little as possible, while just exposing this functionality in Python (though they did only ask about Groovy...)

These will not be affected by horizontal scrolling.
hide (Union[str, List[str]]): the columns to hide.
column_groups (List[Dict]): A list of dicts specifying which columns should be grouped in the UI.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See typehint comment above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "grouped in the UI" mean?

The dicts can specify the following:

* name (str): The group name
* children (List[str]): The column names in the group
* color (Optional[str]): The hex color string or Deephaven color name
Comment on lines +266 to +270
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very unclear. e.g. is name the dict key? are children and color values? etc.

search_display_mode (SearchDisplayMode): set the search bar to explicitly be accessible or inaccessible,
or use the system default. :attr:`SearchDisplayMode.SHOW` will show the search bar,
:attr:`SearchDisplayMode.HIDE` will hide the search bar, and :attr:`SearchDisplayMode.DEFAULT` will
use the default value configured by the user and system settings.

Returns:
a new rollup table with the layout hints set

Raises:
DHError
"""
try:
_j_layout_hint_builder = _JLayoutHintBuilder.get()

if front is not None:
_j_layout_hint_builder.atFront(to_sequence(front))

if back is not None:
_j_layout_hint_builder.atBack(to_sequence(back))

if freeze is not None:
_j_layout_hint_builder.freeze(to_sequence(freeze))

if hide is not None:
_j_layout_hint_builder.hide(to_sequence(hide))

if column_groups is not None:
for group in column_groups:
_j_layout_hint_builder.columnGroup(group.get("name"), j_array_list(group.get("children")),
group.get("color", ""))
Comment on lines +299 to +300
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably error if there are other keys in the dict. e.g. a misspelling.


if search_display_mode is not None:
_j_layout_hint_builder.setSearchBarAccess(search_display_mode.value)

except Exception as e:
raise DHError(e, "failed to create layout hints") from e

try:
return RollupTable(j_rollup_table=self.j_rollup_table.setLayoutHints(_j_layout_hint_builder.build()),
include_constituents=self.include_constituents, aggs=self.aggs, by=self.by)
except Exception as e:
raise DHError(e, "failed to set layout hints on rollup table") from e


class TreeNodeOperationsRecorder(JObjectWrapper, _FormatOperationsRecorder,
_SortOperationsRecorder, _FilterOperationsRecorder):
Expand Down Expand Up @@ -343,6 +403,65 @@ def with_filters(self, filters: Union[str, Filter, Sequence[str], Sequence[Filte
except Exception as e:
raise DHError(e, "with_filters operation on TreeTable failed.") from e

def layout_hints(self, front: Union[str, List[str]] = None, back: Union[str, List[str]] = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apply other comments here

freeze: Union[str, List[str]] = None, hide: Union[str, List[str]] = None,
column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> Table:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> Table:
column_groups: List[dict] = None, search_display_mode: SearchDisplayMode = None) -> TreeTable:

""" Sets layout hints on the TreeTable

Args:
front (Union[str, List[str]]): the columns to show at the front.
back (Union[str, List[str]]): the columns to show at the back.
freeze (Union[str, List[str]]): the columns to freeze to the front.
These will not be affected by horizontal scrolling.
hide (Union[str, List[str]]): the columns to hide.
column_groups (List[Dict]): A list of dicts specifying which columns should be grouped in the UI.
The dicts can specify the following:

* name (str): The group name
* children (List[str]): The column names in the group
* color (Optional[str]): The hex color string or Deephaven color name
search_display_mode (SearchDisplayMode): set the search bar to explicitly be accessible or inaccessible,
or use the system default. :attr:`SearchDisplayMode.SHOW` will show the search bar,
:attr:`SearchDisplayMode.HIDE` will hide the search bar, and :attr:`SearchDisplayMode.DEFAULT` will
use the default value configured by the user and system settings.

Returns:
a new tree table with the layout hints set

Raises:
DHError
"""
try:
_j_layout_hint_builder = _JLayoutHintBuilder.get()

if front is not None:
_j_layout_hint_builder.atFront(to_sequence(front))

if back is not None:
_j_layout_hint_builder.atBack(to_sequence(back))

if freeze is not None:
_j_layout_hint_builder.freeze(to_sequence(freeze))

if hide is not None:
_j_layout_hint_builder.hide(to_sequence(hide))

if column_groups is not None:
for group in column_groups:
_j_layout_hint_builder.columnGroup(group.get("name"), j_array_list(group.get("children")),
group.get("color", ""))

if search_display_mode is not None:
_j_layout_hint_builder.setSearchBarAccess(search_display_mode.value)

except Exception as e:
raise DHError(e, "failed to create layout hints") from e

try:
return TreeTable(j_tree_table=self.j_tree_table.setLayoutHints(_j_layout_hint_builder.build()),
id_col=self.id_col, parent_col=self.parent_col)
except Exception as e:
raise DHError(e, "failed to set layout hints on tree table") from e

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should PartitionedTable or PartitionedTableProxy also have this method?

Copy link
Member Author

@mofojed mofojed Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those classes don't extend GridAttributes which has the setLayoutHints method defined in Java.
Seems like ticket #2890 (comment) is related

def _j_py_script_session() -> _JPythonScriptSession:
j_execution_context = _JExecutionContext.getContext()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,7 @@ private enum RebuildStep {
private Column rowExpandedCol;
private final Column actionCol;
private final JsArray<Column> groupedColumns;
private JsLayoutHints layoutHints;

// The source JsTable behind the original HierarchicalTable, lazily built at this time
private final JsLazy<Promise<JsTable>> sourceTable;
Expand Down Expand Up @@ -1082,6 +1083,25 @@ public String getDescription() {
return tableDefinition.getAttributes().getDescription();
}

@JsProperty
@JsNullable
public JsLayoutHints getLayoutHints() {
if (layoutHints == null) {
createLayoutHints();
}
return layoutHints;
}

private void createLayoutHints() {
String hintsString = tableDefinition.getAttributes().getLayoutHints();
JsLayoutHints jsHints = new JsLayoutHints();
if (hintsString == null) {
layoutHints = null;
} else {
layoutHints = jsHints.parse(hintsString);
}
}

/**
* The current number of rows given the table's contents and the various expand/collapse states of each node. (No
* totalSize is provided at this time; its definition becomes unclear between roll-up and tree tables, especially
Expand Down
Loading