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

Add SearchDisplayMode layout hints #3512

Merged
merged 6 commits into from
Apr 13, 2023
Merged

Conversation

mofojed
Copy link
Member

@mofojed mofojed commented Mar 9, 2023

  • Ported over from Enterprise, where these are already implemented
  • Not sure how best to handle the enum from the Python API, just using a String right now. Will get Jianfeng/Chip to review.
  • Tested in Python and Groovy, ensuring you could bring up the Search Bar (from the side menu) in table canSearch, and it was not available in cannotSearch.
    Python:
from deephaven import empty_table
t = empty_table(100).update(["a=`abc`"])
canSearch = t.layout_hints(search_display_mode="Show")
cannotSearch = t.layout_hints(search_display_mode="Hide")

Groovy:

import io.deephaven.engine.util.LayoutHintBuilder
import io.deephaven.engine.util.LayoutHintBuilder.SearchDisplayModes

t = emptyTable(5).updateView("a=`abc`")
canSearch = t.setLayoutHints(LayoutHintBuilder.get().setSearchBarAccess(SearchDisplayModes.Show).build())
cannotSearch = t.setLayoutHints(LayoutHintBuilder.get().setSearchBarAccess(SearchDisplayModes.Hide).build())

- Ported over from Enterprise, where these are already implemented
- Not sure how best to handle the enum from the Python API, just using a String right now. Will get Jianfeng/Chip to review.
- Tested in Python and Groovy, ensuring you could bring up the Search Bar (from the side menu) in table `canSearch`, and it was not available in `cannotSearch`.
Python:
```python
from deephaven import empty_table
t = empty_table(100).update(["a=`abc`"])
canSearch = t.layout_hints(search_display_mode="Show")
cannotSearch = t.layout_hints(search_display_mode="Hide")
```
Groovy:
```groovy
import io.deephaven.engine.util.LayoutHintBuilder
import io.deephaven.engine.util.LayoutHintBuilder.SearchDisplayModes

t = emptyTable(5).updateView("a=`abc`")
canSearch = t.setLayoutHints(LayoutHintBuilder.get().setSearchBarAccess(SearchDisplayModes.Show).build())
cannotSearch = t.setLayoutHints(LayoutHintBuilder.get().setSearchBarAccess(SearchDisplayModes.Hide).build())
```
@mofojed mofojed added this to the Mar 2023 milestone Mar 9, 2023
@mofojed mofojed requested a review from niloc132 March 9, 2023 14:03
@mofojed mofojed self-assigned this Mar 9, 2023
@@ -1852,7 +1852,7 @@ def format_row_where(self, cond: str, formula: str) -> Table:

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) -> Table:
column_groups: List[dict] = None, search_display_mode: str = None) -> Table:
Copy link
Member Author

Choose a reason for hiding this comment

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

@jmao-denver is there a better way to use an enum from in python? I tried using jpy.get_type("io.deephaven.engine.util.LayoutHintBuilder.SearchDisplayModes") but I get a Class not found error.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is an inner class, the JNI syntax is to use '$' notation.

io.deephaven.engine.util.LayoutHintBuilder$SearchDisplayModes

Please see this file for an example wrapping of an Enum.

py/server/deephaven/plot/linestyle.py

Copy link
Contributor

Choose a reason for hiding this comment

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

I am actually not very sure about introducing an Enum in this case. I am fine with the parameter being a string with 3 valid, self-evident values.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I've used an Enum, let me know if you want me to revert to string. I think @chipkent has previously said he prefers enums, not sure. Downside is you need a from deephaven.table import SearchDisplayMode before this.

Or I could make it accept both a string or the enum?

niloc132
niloc132 previously approved these changes Mar 9, 2023
Copy link
Member

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

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

Approved js api changes

py/server/deephaven/table.py Outdated Show resolved Hide resolved
- Added tests as well, but not sure how to run them
- Tests now verify all layout hints, instead of just checking if the table had blown up
@mofojed mofojed requested a review from niloc132 March 31, 2023 20:34
Copy link
Contributor

@jmao-denver jmao-denver left a comment

Choose a reason for hiding this comment

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

LGTM

@mofojed mofojed merged commit 069b543 into deephaven:main Apr 13, 2023
@mofojed mofojed deleted the search-display-mode branch April 13, 2023 13:00
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2023
@deephaven-internal
Copy link
Contributor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants