Work on image-display user experience #217
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the Jupyter widget system, it is possible to create multiple views of a single widget model. Views can be "detached", in which case their underlying DOM elements are removed from the document. This is what happens if you create a WWT view in a JupyterLab notebook, use the "Create a new view for this output" context menu item to move the output to a new tab, and then hide the original output. There is one underlying widget model and two views, only one of which has active DOM elements. The wrinkle for pywwt is that all of our widget state is stored inside the <iframe> that does the rendering, so we're not really a real model-view setup. The way things currently work, operations that affect the widget state are passed as message from Python to JS, which are then distributed to all of the views. So long as all views receive the same messages, things should stay in sync. (Although if you create a widget, do some things to it, and then create a new view of it, that view will have missed historical messages and so will be out of sync.) The problem I discovered was that if you send messages to a "detached" widget view, you sometimes get JS exceptions. And apparently the code that distributes the messages just terminates, preventing the control messages from being sent to other widget views. This means that those views basically stop responding to any Python controls. This is bad news for the workflow of creating a WWT view then moving it to a separate tab, which is something we very much want to support for UX reasons. The real solution is to Do The Right Thing when a widget is detached, so that the JS exceptions don't happen in the first place, but that's going to take some time to get right. In the meantime, let's just log and swallow those JS exceptions, so that the message passing can keep on working to the views that aren't broken. This will keep things limping along.
… are applied The way that the ImageLayer constructor was structured, any user specification of the vmax and vmin values would be overridden. Restructure to prefer them, and also make sure that the settings are applied but not in a redundant way.
…class This custom subclass exposes some work-in-progress controls that allow the image display parameters to be adjusted interactively.
Now by showing `my_image_layer.controls` you get widgets that let you adjust the opacity, stretch function, and data range. The best UI I could come up with for the data range has "coarse" controls that are typed in manually and "fine" controls that use a range slider widget. I note that in my testing, the adjustments don't apply until focus changes to another widget, which makes for a kind of weak UX. I checked the ipywidgets bug tracker but don't see any complaints about this; I'm not sure what's going on, since this seems like a pretty basic piece of functionality.
Codecov Report
@@ Coverage Diff @@
## master #217 +/- ##
=========================================
- Coverage 57.81% 56.91% -0.9%
=========================================
Files 24 24
Lines 1695 1727 +32
=========================================
+ Hits 980 983 +3
- Misses 715 744 +29
Continue to review full report at Codecov.
|
|
This needs docs, but I'm going to merge now. (I'm also going to ignore the minor hit in code coverage.) I want to rearrange the docs a bit anyway, so that will wait til another pull request. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(This PR is cumulative on #216)
The main goal here is to add controls to the Jupyter widget so that the user can interactively tune the image stretch parameters straight in their notebook.
I set this up so that the Qt version of the widget should be unaffected.
I also ran into some issues with hidden JupyterLab widget views that look pretty thorny; I have some lengthy commit messages describing what I found and a workaround I put into place.