Skip to content

Commit

Permalink
fix: Don't recreate JS TreeTable subscriptions when viewport changes (d…
Browse files Browse the repository at this point in the history
…eephaven#6060)

Resolves an issue where subscription recreation would release an export
ticket that was still needed for the next subscription.

Also improves tree viewport performance by sending changes instead of
waiting for a new subscription to exist.

Fixes deephaven#6056
  • Loading branch information
niloc132 committed Sep 13, 2024
1 parent 564b146 commit ae73645
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -599,14 +599,25 @@ private void replaceSubscription(RebuildStep step) {
viewTicket.release();
viewTicket = null;
}
case SUBSCRIPTION:

// In all of the above cases, we replace the subscription
if (stream != null) {
stream.then(stream -> {
stream.close();
return null;
});
stream = null;
}
break;
case SUBSCRIPTION:
// If it exists, adjust the existing subscription, otherwise create a new one
if (stream != null) {
stream.then(subscription -> {
subscription.setViewport(firstRow, lastRow, Js.uncheckedCast(columns), (double) updateInterval);
return null;
});
return;
}
}

Promise<TreeSubscription> stream = Promise.resolve(defer())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ public void testStaticTreeTable() {
JsTreeTable.EVENT_UPDATED, 2001d);
}).then(event -> {
assertEquals(10d, event.detail.getTreeSize());
assertEquals(10, event.detail.getRows().length);


// move the viewport and try again
treeTable.setViewport(5, 50, treeTable.getColumns(), null);
return treeTable.<TreeViewportData>nextEvent(
JsTreeTable.EVENT_UPDATED, 2002d);
}).then(event -> {
assertEquals(10d, event.detail.getTreeSize());
assertEquals(5, event.detail.getRows().length);

treeTable.close();

Expand Down Expand Up @@ -133,6 +143,15 @@ public void testRefreshingTreeTable() {
assertEquals(green, data.getFormat(1, idCol));
assertEquals(green, row2.getFormat(idCol));
assertEquals(green, idCol.getFormat(row2));

// Move the viewport and make sure we get the correct data
treeTable.setViewport(5, 49, treeTable.getColumns(), null);
return treeTable.<TreeViewportData>nextEvent(
JsTreeTable.EVENT_UPDATED, 2002d);
}).then(event -> {
assertEquals(10d, event.detail.getTreeSize());
assertEquals(5, event.detail.getRows().length);

return Promise.resolve(treeTable);
})
.then(event -> {
Expand Down

0 comments on commit ae73645

Please sign in to comment.