superstar54 / weas-widget

A widget to visualize and edit atomic structures in Jupyter Notebook
https://weas-widget.readthedocs.io/en/latest/index.html
MIT License
19 stars 3 forks source link

Tidy up top level namespace of `WeasWidget`? #43

Closed ltalirz closed 5 months ago

ltalirz commented 6 months ago

The top level namespace of WeasWidget is rather crowded (30+ attributes)

https://github.com/superstar54/weas-widget/blob/43d39f71088af5710186216ff40858ea368c8132/src/weas_widget/__init__.py#L15

Perhaps it would make sense to group some of those together and nest them.

Also, it appears there are some traitlets while other things are manipulated with set_ methods, and to me as a new user it is not obvious why.

superstar54 commented 6 months ago

Thanks, @ltalirz, for the great valuable feedback and suggestions on enhancing the code. I will try to reply to some of them now, and others later once I have a clearer idea.

Indeed, there are too many attributes on the top level, and it better to group and nest them. But I need to look into the widget framework, anywidget, and see if it supports traits within nested namespaces.

manzt commented 5 months ago

My recommendation would be to wrap the widget class and expose the _repr_mimebundle_ (what displays the actual widget), and encapsulate the anywidget instance from end users. You can then proxy assignment to properties with getter/setter patterns:

class Wrapper:

    def __init__(self, *args, **kwargs):
        self._widget = WeasWidget(*args, **kwargs)

    @property
    def value(self):
        return self._widget.value

    @value.setter
    def value(self, update):
        self._widget.value = update

    def _repr_mimebundle_(self, *args, **kwargs):
        return self._widget._repr_mimebundle_(*args, **kwargs)

FWIW, I've been exploring trying to reduce the API surface area of AnyWidget, but this complexity is inherited from (and meant to expose) the API from ipywidgets.Widget familiar to Jupyter Widgets developers.

superstar54 commented 5 months ago

@manzt, thanks for the suggestion; it's very helpful! I made some adjustments to ensure compatibility with ipywidget 7.x.

def _repr_mimebundle_(self, *args, **kwargs):
    # if ipywdigets > 8.0.0, use _repr_mimebundle_ instead of _ipython_display_
    if hasattr(self._widget, "_repr_mimebundle_"):
        return self._widget._repr_mimebundle_(*args, **kwargs)
    else:
        return self._widget._ipython_display_(*args, **kwargs)
superstar54 commented 5 months ago

@manzt One issue I notice with the _repr_mimebundle_ method is that I can not embed the widget into the HBox or VBox from ipywidgets.

Thus, I decide to wrapper the base widget by inheriting the HBox

class Wrapper(HBox):

    def __init__(self, *args, **kwargs):
        self._widget = WeasWidget(*args, **kwargs)
        super().__init__(children=[self._widget])

    @property
    def value(self):
        return self._widget.value

    @value.setter
    def value(self, update):
        self._widget.value = update