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

Widget should not inherit from VBox #132

Closed
pllim opened this issue Mar 2, 2021 · 2 comments
Closed

Widget should not inherit from VBox #132

pllim opened this issue Mar 2, 2021 · 2 comments

Comments

@pllim
Copy link
Member

pllim commented Mar 2, 2021

This was brought up by @astrofrog in relation to #93, #126, #131, and spacetelescope/jdaviz#445 .

Need inputs from @mwcraig and @eteq .

Idea (from Tom R)

I think for the single viewer, Imviz, we might want astrowidgets to actually wrap the whole Imviz/JupyterApplication, rather than the viewer instance, where we can get a reference to the viewer to use in the rest of the wrapper.

Imviz here is from spacetelescope/jdaviz#429 :

from astrowidgets import AstroWidgetsWrapper
from jdaviz import Imviz

imviz = Imviz()
imviz_aw = AstroWidgetsWrapper(imviz)  # Exposes astrowidgets API

So basically you don’t need to worry about the ipywidgets.VBox stuff. We show the GUI using regular jdaviz and the wrapper just exposes the astrowidgets API and translates it on-the-fly to what is needed for jdaviz but doesn’t deal with any UI aspect (so no mouseover, no VBox, etc).

Pros

UI is now completely handled by the chosen backend, thus freeing astrowidgets of the burden and might make #122 implementation less complicated.

Cons

In the idea above, we won't be able to do the cursor positioning like this anymore:

@cursor.setter
def cursor(self, val):
if val is None:
self._jup_coord.layout.visibility = 'hidden'
self._jup_coord.layout.display = 'none'
elif val == 'top' or val == 'bottom':
self._jup_coord.layout.visibility = 'visible'
self._jup_coord.layout.display = 'flex'
if val == 'top':
self.layout.flex_flow = 'column-reverse'
else:
self.layout.flex_flow = 'column'
else:
raise ValueError('Invalid value {} for cursor.'
'Valid values are: '
'{}'.format(val, ALLOWED_CURSOR_LOCATIONS))
self._cursor = val

Things like this is also no longer possible:

@property
def image_height(self):
return int(self._jup_img.height)
@image_height.setter
def image_height(self, value):
# widgets expect width/height as strings, but most users will not, so
# do the conversion.
self._jup_img.height = str(value)

By farming out all GUI controls to backends, while #122 implementation here might be less complicated, it might also not be entirely possible to simply "switch backend" transparently. For instance, Ginga implementation might still rely on VBox and will implement the cursor positioning in its own subclass, which is not available in other backends.

https://xkcd.com/927/

@astrofrog
Copy link
Member

Just one clarifying comment - it's still possible to offer a base class that does rely on ipywidgets. What I'm advocating here is that beneath that there should be an abstract base class that is completely independent of the technology for the front-end (and could even be used outside a Jupyter context, for example to wrap pyds9 to drive DS9 or other GUI tools). The ipywidgets base class could then inherit from that ABC.

@pllim
Copy link
Member Author

pllim commented Jun 16, 2023

This is naturally superseded by #162 because by using Protocol, we will no longer enforce how each backend actually does the implementation, VBox or not.

@pllim pllim closed this as completed Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants