Skip to content

fix: Use correct offset in snapshot#2217

Merged
mofojed merged 6 commits intodeephaven:mainfrom
mofojed:fix-viewport-data
Sep 12, 2024
Merged

fix: Use correct offset in snapshot#2217
mofojed merged 6 commits intodeephaven:mainfrom
mofojed:fix-viewport-data

Conversation

@mofojed
Copy link
Copy Markdown
Member

@mofojed mofojed commented Sep 11, 2024

  • Was using row.offsetInSnapshot, which was a "protected" API and recently removed from the JS API
  • API was removed in JS API refactoring: refactor: Rewrite JS API to share server implementation of Flight/Barrage deephaven-core#5890
  • Instead just use the viewport offset and add the index of the row in the snapshot, which is there in both versions of the API
    • New API does support row.index, but this way is compatible with both
  • Updated unit tests
  • Tested using a deephaven.ui.list_view:
from deephaven import time_table, ui
import datetime

initial_row_count = 200
column_types = time_table(
    "PT1S",
    start_time=datetime.datetime.now() - datetime.timedelta(seconds=initial_row_count),
).update(
    [
        "Id=new Integer(i)",
        "Display=new String(`Display `+i)",
    ]
)

@ui.component
def ui_list_view_table():
    value, set_value = ui.use_state([])

    lv = ui.list_view(
        column_types,
        aria_label="List View",
        on_change=set_value,
        selected_keys=value,
    )

    text = ui.text("Selection: " + ", ".join(map(str, value)))

    return ui.flex(
        lv,
        text,
        direction="column",
        margin=10,
        gap=10,
        width=500,
        # necessary to avoid overflowing container height
        min_height=0,
    )

lv_table = ui_list_view_table()

- Was using `row.offsetInSnapshot`, which was a "protected" API and recently removed from the JS API
- API was removed in JS API refactoring: deephaven/deephaven-core#5890
- Instead just use the viewport `offset` and add the index of the row in the snapshot, which is there in both versions of the API
  - New API does support `row.index`, but this way is compatible with both
- Updated unit tests
- Tested using a deephaven.ui.list_view:
```python
from deephaven import time_table, ui
import datetime

initial_row_count = 200
column_types = time_table(
    "PT1S",
    start_time=datetime.datetime.now() - datetime.timedelta(seconds=initial_row_count),
).update(
    [
        "Id=new Integer(i)",
        "Display=new String(`Display `+i)",
    ]
)

@ui.component
def ui_list_view_table():
    value, set_value = ui.use_state([])

    lv = ui.list_view(
        column_types,
        aria_label="List View",
        on_change=set_value,
        selected_keys=value,
    )

    text = ui.text("Selection: " + ", ".join(map(str, value)))

    return ui.flex(
        lv,
        text,
        direction="column",
        margin=10,
        gap=10,
        width=500,
        # necessary to avoid overflowing container height
        min_height=0,
    )

lv_table = ui_list_view_table()
```
@mofojed mofojed requested a review from a team September 11, 2024 15:39
@mofojed mofojed self-assigned this Sep 11, 2024
@mofojed mofojed requested review from dgodinez-dh and removed request for a team September 11, 2024 15:39
dgodinez-dh
dgodinez-dh previously approved these changes Sep 11, 2024
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.77%. Comparing base (99b8d59) to head (d789b90).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    deephaven/web-client-ui#2217      +/-   ##
==========================================
- Coverage   46.78%   46.77%   -0.01%     
==========================================
  Files         694      694              
  Lines       38627    38625       -2     
  Branches     9659     9780     +121     
==========================================
- Hits        18070    18068       -2     
+ Misses      20546    20504      -42     
- Partials       11       53      +42     
Flag Coverage Δ
unit 46.77% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- Throw if gridLocation is null, removes some unnecessary null checks
- Have a little wait after getting the grid location, looks like we think it's loaded before it actually is
  - There should be a more robust way to do this, this is not the best.
- They are causing tests to fail (as they should)
@mofojed
Copy link
Copy Markdown
Member Author

mofojed commented Sep 12, 2024

e2e tests are failing due to deephaven/deephaven-core#6056
Forcing this one to merge as it fixes an issue that is affecting the e2e tests for deephaven-plugins.

@mofojed mofojed merged commit a479d6c into deephaven:main Sep 12, 2024
@mofojed mofojed deleted the fix-viewport-data branch September 12, 2024 14:03
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants