Fix behavior of PublicOnlyProxy in setattr, wrapped methods, and calling #5997
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5997 +/- ##
==========================================
+ Coverage 91.56% 91.57% +0.01%
==========================================
Files 573 573
Lines 49809 49865 +56
==========================================
+ Hits 45606 45663 +57
+ Misses 4203 4202 -1
|
|
@Czaki I'm sorry, I don't really understand this PR... I trust that you know what you're doing 😅 but as a reviewer I'd like to understand it. Do you think you could explain it in a bit more detail? Something like, "When passing the viewer to plugins, we wrap it in a PublicOnlyProxy, to avoid plugins accessing private attributes and methods. This has worked fine in most cases but fails when X, because Y. To fix it, we remove the proxy before doing Z." It's also unclear to me whether this fixes the "whole class of issues" exemplified by #5767 as you commented there, or just two ( |
The pointed issue shows the problem with one function. I mean that same problem may happen in any place where we pass napari objects to Qt, as it uses something like
Ok. I will add more explanation. |
Czaki
changed the title
|
@jni I have added docstrings. Could you check now? |
jni
changed the title
|
@Czaki I've updated the comments, updated the PR description, and made the variables in the tests more descriptive because I was having trouble following the logic. Could you please double-check them and thumbs-up here if all good? Otherwise I think this is ready to merge! 🚀 Thanks for the pair session which made things clear. 😊 |
| @@ -134,7 +152,15 @@ def __dir__(self): | |||
| @classmethod | |||
| def create(cls, obj: Any) -> Union['PublicOnlyProxy', Any]: | |||
| # restrict the scope of this proxy to napari objects | |||
| mod = getattr(type(obj), '__module__', None) or '' | |||
| if type(obj).__name__ == 'method': | |||
| # If the given object is a method, we check the module *of the | |||
…ing (#5997) # Fixes/Closes Closes #5767 # Description This PR makes a few tweaks to the behavior of PublicOnlyProxy: - in setattr, if we want to set an attribute of a wrapped object, if the attribute value is itself an object wrapped with PublicOnlyProxy, we unwrap it. This has two benefits: (1) checking the attribute later incurs a significant cost in `_is_called_from_napari`, which involves frame inspection. This way we don't incur that cost. And (2) Certain equality checks fail when PublicOnlyProxy's interact with Qt. This is #5767 and this PR fixes that. - in `PublicOnlyProxy.create`, we now check whether the object we want to wrap is a method, because then the module we need to check is not that of the method (this is always `builtins`), but rather the module of the object *that that method is bound to.* This is now fixed. - When a PublicOnlyProxy'd object is callable, we unwrap the arguments, call the unwrapped function with the unwrapped arguments, then wrap the result. This avoids the performance issues detailed above. --------- Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
…ing (napari#5997) # Fixes/Closes Closes napari#5767 # Description This PR makes a few tweaks to the behavior of PublicOnlyProxy: - in setattr, if we want to set an attribute of a wrapped object, if the attribute value is itself an object wrapped with PublicOnlyProxy, we unwrap it. This has two benefits: (1) checking the attribute later incurs a significant cost in `_is_called_from_napari`, which involves frame inspection. This way we don't incur that cost. And (2) Certain equality checks fail when PublicOnlyProxy's interact with Qt. This is napari#5767 and this PR fixes that. - in `PublicOnlyProxy.create`, we now check whether the object we want to wrap is a method, because then the module we need to check is not that of the method (this is always `builtins`), but rather the module of the object *that that method is bound to.* This is now fixed. - When a PublicOnlyProxy'd object is callable, we unwrap the arguments, call the unwrapped function with the unwrapped arguments, then wrap the result. This avoids the performance issues detailed above. --------- Co-authored-by: Juan Nunez-Iglesias <jni@fastmail.com>
brisvag
removed
the
ready to merge
Fixes/Closes
Closes #5767
Description
This PR makes a few tweaks to the behavior of PublicOnlyProxy:
_is_called_from_napari, which involves frame inspection. This way we don't incur that cost. And (2) Certain equality checks fail when PublicOnlyProxy's interact with Qt. This is Setting active layer from a plugin on PublicOnlyProxy works partially, but not in GUI #5767 and this PR fixes that.PublicOnlyProxy.create, we now check whether the object we want to wrap is a method, because then the module we need to check is not that of the method (this is alwaysbuiltins), but rather the module of the object that that method is bound to. This is now fixed.