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

refactor: Make all the valued containers subclass ValueWidget #663

Merged
merged 11 commits into from
Nov 22, 2024

Conversation

hanjinliu
Copy link
Collaborator

Closes #372 #661

Now this works:

from magicgui.widgets import FileEdit, PushButton
from magicgui.widgets.bases import ValueWidget

assert isinstance(PushButton(), ValueWidget)
assert isinstance(FileEdit(), ValueWidget)

I had to refactor more than I first thought. The major problem was that ValueWidget had ValueWidgetProtocol before, which is not compatible with container-like widgets (e.g. there's no _mgui_get_value for FileEdit). In this PR, I made ValueWidget more abstract: it only implements the basic interface for getting/setting values, thus independent of backends. PrimitiveValueWidget is the class that require backends, which is identical to the former ValueWidget. Besides, I introduced _BaseContainerWidget for ValuedContainerWidget because many methods of ContainerWidget is not publicly needed for value widgets (e.g. we never use insert for FileEdit).

Inheritance map is now like below.

Widget ----------------> ValueWidget --> PrimitiveValueWidget --> Label, LineEdit, Image etc.
   |                        |
   v                        v
_BaseContainerWidget --> ValuedContainerWidget -----------------> FileEdit, RangeEdit, ListEdit etc.
   |
   v
ContainerWidget ------------------------------------------------> Container

Notes:

  • EmptyWidget is now a ValuedContainerWidget because it's simply an empty container. Backend implementation of EmptyWidget is no longer needed.
  • ValueWidget now inherits ABC because abstractmethod get_value and set_value are needed. This means if one made a Qt widget and magicgui interface like
    class QMyWidget(QBaseValueWidget): ...
    
    class MyWidget(ValueWidget):
        def __init__(self, **kwargs):
            kwargs["widget_type"] = QMyWidget
            super().__init__(**kwargs)
    then it fails with TypeError due to undefined abstract methods.

Please let me know what you think.

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 91.42857% with 18 lines in your changes missing coverage. Please review.

Project coverage is 88.87%. Comparing base (a5669e3) to head (4fc613c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/magicgui/widgets/_concrete.py 83.09% 12 Missing ⚠️
src/magicgui/widgets/bases/_container_widget.py 93.87% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #663      +/-   ##
==========================================
- Coverage   89.00%   88.87%   -0.14%     
==========================================
  Files          39       39              
  Lines        4777     4754      -23     
==========================================
- Hits         4252     4225      -27     
- Misses        525      529       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@tlambert03
Copy link
Member

tlambert03 commented Aug 2, 2024

this looks like a great start @hanjinliu, the one thing I'm not thrilled about is the changing of the meaning of ValueWidget to PrimitiveValueWidget... it's a breaking change to expect everyone to now inherit from PrimitiveValueWidget, and as you can see it breaks your own magic-class tests here. I'd rather use a shared base class like ValueWidgetBase and have it not be the case that isinstance(TupleWidget(), ValueWidget) than to rename all ValueWidgets

@hanjinliu
Copy link
Collaborator Author

That makes sense. Keeping ValueWidget in the same state is definitely safer.
The magic-class tests failed by the isinstance check of ValueWidget but I think this should be fixed on my side (this is due to the update in EmpytWidget).
The recent breaking change in griffe affects the docs test.

@tlambert03
Copy link
Member

sorry that this has sat so long @hanjinliu. would you mind resolving conflicts now that TypeMap is in?

@tlambert03
Copy link
Member

docs are not building correctly anymore. I think it may require magicgui.widgets.bases.BaseValueWidget to be added to the autosummary in docs/api/widgets/bases.md Please also check whether the mermaid diagram in that document is still correct after this change, new things may need to be added and the connections may need to be changed

@hanjinliu
Copy link
Collaborator Author

Thank you for your help! I found that after the implementation of TypeMap, functions that refer to the global type map are not very informative (please look at the API reference of magicgui.type_map.get_widget_class in the latest docs for instance). It'll be better to have some links pointing to the TypeMap method like (see [TypeMap.get_widget_class](...)), but we need additional hooks to this because these lines are built by the pre-defined autoref hook.

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

thanks @hanjinliu!

@tlambert03 tlambert03 merged commit 4bc81e9 into pyapp-kit:main Nov 22, 2024
36 checks passed
@tlambert03 tlambert03 changed the title Make all the valued containers subclass ValueWidget refactor: Make all the valued containers subclass ValueWidget Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All value-like Container widgets should inherit from ValueWidget
2 participants