Use features instead of properties internally #7084
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7084 +/- ##
==========================================
- Coverage 92.95% 92.88% -0.07%
==========================================
Files 618 618
Lines 56449 56446 -3
==========================================
- Hits 52471 52429 -42
- Misses 3978 4017 +39 ☔ View full report in Codecov by Sentry. |
|
Marking this ready for review, but understood this won't make it for v0.5.0, so no rush. One thing I'm still thinking about if I should fully implement the behavior of the |
I advocated for changing this behavior before (you might remember, see #5748 and #5722 (comment)). I would be in favor now too :) |
To clarify, you are in favor of something like One thing I don't like is the asymmetry between the @property
def feature_defaults(self):
...
@feature_defaults.setter
def feature_defaults(self, feature_defaults):
...
def set_selected_features(self, values):
...where the What do you think? |
|
For some more context, another reason I don't like the As they're defined in napari, I'd also be open to property getter/setter if that fits better with napari's API. @property
def selected_features(self):
...
@selected_features.setter
def selected_features(self, values):
...The feature values passed to the setter would just need to be broadcast-able to the full table. |
No, I'm in favour of having two separate concept: I actually don't think your proposed solution is clearer than what we have currently, it's just differently unexpected ^^' IMO we shoudl have: @property
def features_selected(self):
...
@features_selected.setter
def features_selected(self, values):
...
@property
def features_default(self):
...
@features_default.setter
def features_default(self, values):
...And none of them should affect each other. Similarly, we should have: @property
def size_selected(self):
...
@size_selected.setter
def size_selected(self, values):
...
@property
def size_default(self):
...
@size_default.setter
def size_default(self, values):
...I understand that the latter makes the gui a bit more cumbersome (two sliders?), and that was the main complaint... but at least programmatically it should be the way to go. |
References and relevant issues
Split from the very large #5477 to make reviewing these changes easier.
Description
This uses
featuresinstead ofpropertiesin the implementation details of napari, but does not yet deprecate use ofproperties. As part of these changes, it also adds thefeaturesevent toTracks.events.The intent of this PR is to make these changes without breaking the tests and without changing the tests. When actually deprecating
propertiesin practice, we may need to modify some of the tests.