Skip to content

Reimplement editable for PerspectiveWidget in JupyterLab to take into account the latest changes#1850

Merged
texodus merged 5 commits intomasterfrom
tkp/editable
Jun 16, 2022
Merged

Reimplement editable for PerspectiveWidget in JupyterLab to take into account the latest changes#1850
texodus merged 5 commits intomasterfrom
tkp/editable

Conversation

@timkpaine
Copy link
Member

This PR does 2 things: Fixes some small bugs with boolean values in the perspective datagrid extension (smaller part), and reimplements the editable attribute to account for the latest changes in perspective-viewer.

Editability of boolean values in the datagrid plugin

CC: #1833

Due to a small bug, boolean columns were always shown as editable.

Hover behavior:
hover

Click behavior:
click

This behavior is fixed to not toggle booleans and not trigger cursor:pointer unless the data grid is marked as editable

Hover behavior:
hover

Click behavior:
click

Editable in PerspectiveWidget

editable was added to the PerspectiveWidget and PerspectiveTraitlets for legacy reasons. Even though the client/server interface existed, the PerspectiveWidget plays a dual role as a wrapper for both tables and views, but also the frontend perspective-viewer. However, the perspective-viewer has no formal client/server interface.

Previously, it was written in pure JS, and a 1-1 wrapper was written in Python in the form of the PerspectiveTraitlets class. However, no end-to-end tests were ever written to assert this functionality. As changes were made, most notably the shift to current rust-based perspective-viewer, various parts regressed, and any problems had to be manually discovered and fixed.

One of the major regressions was the removal of editable as an attribute of perspective-viewer, and the inclusion of it in the plugin_config for the perspective datagrid. This PR brings the python code in line with the latest rust/JS state by removing editable completely from the PerspectiveTraitlets and PerspectiveWidget class, and reimplementing the required bidirectional syncing to allow for the plugin_config based editable flag. This is technically a breaking change as the editable attribute is now removed from the various python classes, however this behavior was already non-functional.

Old Behavior

Construction (non-functional):
constructor

Toggling from JS (non-functional):
js

Toggling from Python (non-functional):
python

New Behavior

Construction:
constructor

Toggling from JS:
js

Toggling from Python:
python

Working end-to-end:
works

Tests

Almost more important than these bug fixes, the JupyterLab tests are greatly expanded. Previous tests only checked the load-time behavior of the Perspective Widget:

  • loading data
  • loading data with an update
  • loading data from a Table instance
  • loading with settings=False

These existing tests are expanded with additional tests, including tests which dynamically execute additional python cells, or manipulate the frontend components and assert the syncing of state back to python:

  • Loading a widget with editable
  • Toggling editability from python (syncs to JS)
  • Toggling editability from JS (syncs to Python)
  • Toggling columns, filter, plugin, group_by, split_by, sort, and theme from Python (syncs to JS)
  • Toggling columns, filter, plugin, group_by, split_by, sort, and theme from JS (syncs to Python)
  • Editing in the frontend (syncs back to Python data)

Helper functions are added to run Python code in a cell after initial notebook run, and to run Python assertions in a cell and track if Python exceptions were raised. This should aid in future extension of these tests.

@timkpaine timkpaine added bug Concrete, reproducible bugs JS Python breaking labels Jun 11, 2022
@timkpaine timkpaine force-pushed the tkp/editable branch 5 times, most recently from 5b540e5 to bc29607 Compare June 12, 2022 14:28
@texodus texodus removed the breaking label Jun 12, 2022
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Great tests coverage and thorough explanation of the issue is much appreciated.

I have a minor gripe, which is that tests that rely on arbitrary timeouts are doomed to fail over enough time. I understand that these are tiny, few and arguably inconsequential here, but #1823 corrects another set of seemingly-innocuous timeouts which caused hundreds of hours of wasted CI time by only failing ~5% of the runs (but after 45m of build).

Basically, if shrinking these timeouts causes consistent failures, we need to remove them and use discrete await conditions instead. I'm willing to take your word that these have been tested and/or are well understood, but I'm going to hold off on merging completely until I've verified that the tests are stable in CI through several build cycles.

@timkpaine
Copy link
Member Author

timkpaine commented Jun 13, 2022

I've updated the tests but I dont really expect them to pass, I expect there to be random TimeoutErrors due to arbitrarily small specifications and the varied load of the test machine.

@timkpaine
Copy link
Member Author

welp nevermind, this is testing pretty stable for me locally but I thought it would've had issues on CI. we can bump the ci a few times to test if we want

timkpaine and others added 5 commits June 14, 2022 20:35
…table because the click handler does not check whether the perspective datagrid plugin is editable (fixes #1833)
…rid plugin's editability is toggled (partial fix for #1832)
running in JupyterLab.

This commit removes the `editable` attribute from PerspectiveWidget and
PerspectiveTraitlets, bringing it inline with the changes made to the
PerspectiveViewer. The Perspective JupyterLab extension is reimplemented
to leverage `plugin_config` for editability instead of `editable`, and
tests are greatly extended to test the various js<->python
synchronizeable attributes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Concrete, reproducible bugs JS Python

Development

Successfully merging this pull request may close these issues.

2 participants