Replace on_trait_change decorators with observe#585
Conversation
…chronous_updates.py
…(example seems like it was already broken anyway)
| @on_trait_change('line_color, line_width, fill_color, alpha') | ||
| def _attributes_changed(self): | ||
| @observe('line_color, line_width, fill_color, alpha') | ||
| def _attributes_changed(self, event): |
There was a problem hiding this comment.
i'm not a fan of the *_changed method name especially when it's not a trait change handler but i can't think of one better either.
There was a problem hiding this comment.
In general, i've been told that the methods shouldn't be named based on what changed - rather, they should be named to convey what they are going to do.
Until we find a better name, maybe we can just call this _attributes_updated - which i've seen elsewhere in this PR.
There was a problem hiding this comment.
alternatively, we could rename it to _invalidate_and_redraw - if no such method already exists in the class heirarchy - because that's what the method does.
There was a problem hiding this comment.
the base enable component class has an invalidate_and_redraw method (just no leading underscore, which also simply just calls self.invalidate_draw() and self.request_redraw()
| @on_trait_change('color_mapper:updated') | ||
| def _color_mapper_updated(self): | ||
| @observe('color_mapper:updated') | ||
| def _color_mapper_updated(self, event): |
There was a problem hiding this comment.
same comment as above about updating method name.
| @on_trait_change('edge_color, edge_width, edge_style, face_color, alpha') | ||
| def _attributes_changed(self): | ||
| @observe('edge_color, edge_width, edge_style, face_color, alpha') | ||
| def _attributes_changed(self, event): |
There was a problem hiding this comment.
same comment as above - about renaming this to _invalidate_and_redraw if such a method doesn't already exist in the class heirarchy
Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>
…use metadata on traits to trigger it
|
Okay I have gone through and done a bunch of renaming and also started to follow Corran's suggestion about using metadata and pulling the hook for Now if a particular trait changing will require an invalidate and redraw, one can simply set the metadata Nonetheless, I believe this PR is close to being ready to push through |
rahulporuri
left a comment
rahulporuri
left a comment
There was a problem hiding this comment.
Mostly LGTM with a few comments -
- I would have preferred it if we made the
request_redrawrelated changes in a separate PR - given that those changes are adding a new feature and are sort of unrelated to the scope of this PR. - We need to document the fact that classes which inherit from
PlotComponentcan define traits with the metadatarequest_redrawif they want changes to the trait to trigger an invalidate and redraw. - I'm trying to think of a different/better name than
request_redrawbut i can't come up with any at the moment. We want to be conscious of the choice because it will be part of the public API.
|
|
||
| #: Overall alpha value of the image. Ranges from 0.0 for transparent to 1.0 | ||
| alpha = Range(0.0, 1.0, 1.0) | ||
| alpha = Range(0.0, 1.0, 1.0, requires_redraw=True) |
There was a problem hiding this comment.
This will now spill over into other classes which inherit from BaseXYPlot i.e. any subclass of BaseXYPlot will invalidate and redraw if the alpha value changes.. That might be the right behavior but we need to be conscious of the fact that we might be modifying behavior.
Yeah you are right, my bad on that. I can still pull it out at this point if you prefer
That makes sense, I will add this
I believe it is currently |
No no lets not.
Let's go with |
2 participants
This PR replaces uses of the
@on_trait_changedecorator with the equivalent form usingobserve.I made changes one file at a time and ran test suite / tested examples after each change. However, it is possible certain changed code is not under test.