feat: Pass parent to backend widget #440
Conversation
| if isinstance(parent, Widget): | ||
| parent = parent.native | ||
| try: | ||
| self._widget = widget_type(parent=parent, **backend_kwargs) |
There was a problem hiding this comment.
👀 @hanjinliu ... this is the specific line that would break magic-class. There's a fallback to the old behavior with a warning below
Codecov Report
@@ Coverage Diff @@
## main #440 +/- ##
==========================================
+ Coverage 88.99% 89.02% +0.02%
==========================================
Files 30 30
Lines 3944 3953 +9
==========================================
+ Hits 3510 3519 +9
Misses 434 434
Continue to review full report at Codecov.
|
|
Hi @tlambert03 , |
|
@Czaki, would be good to get your review on this one |
There was a problem hiding this comment.
I see a big profit from this PR, but I have worries about exposing users to the Qt cleaning system. The profit from this PR is big enough to implement this. But proper information should be added to the documentation.
I also think it may be profitable to connect to destroyed signal to prepare a better error message or reinstance widget on failure.
| @@ -66,6 +67,7 @@ def __init__( | |||
| visible: bool | None = None, | |||
| enabled: bool = True, | |||
| gui_only=False, | |||
| parent: Any = None, | |||
There was a problem hiding this comment.
The parent is not documented in the docstring. As this is a fragile part of this PR. There should be informed that if the parent widget is removed (for example, Qt native one) then the object will be unavailable, and trying to access it may end with RuntimeError: C++ wrapped object has been destroyed.
I think that it is fragile as many magicgui users are not familiar with Qt and its errors.
There was a problem hiding this comment.
yeah, that's a very good point. lemme think about that
There was a problem hiding this comment.
added docs, it's too much to tackle in this PR. An approval would be appreciated so I can merge (without having to force it)
Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
This PR will allow passing a parent to any magicgui widget.
@hanjinliu, this requires that custom widget types accept a parent argument and will affect you. I ran your tests to make sure they pass with this PR, and they do, but you'll see a lot of
FutureWarningnow. Let me know if you have any thoughts/concernsin addition to passing
parent=all the way up the widget chain, this adds aparent=argument torequest_values, to allow a dialog to be associated with some other widget. parent can be either a magicgui widget or a native backend widget (if a magicgui widget,widget.nativewill be passed to the backend)