Remove all definitions / uses of _draw_component#743
Conversation
|
|
||
| self._cache_valid = True | ||
|
|
||
| def _draw_overlay(self, gc, view_bounds=None, mode="normal"): |
There was a problem hiding this comment.
_draw_overlay and overlay are now currently the exact same method. Perhaps one should just call the other to avoid the duplication. I think it would make more sense to have overlay call _draw_overlay.
AFAIK we need to keep both methods, but this should be investigated more closely.
There was a problem hiding this comment.
I think it would make more sense to have overlay call _draw_overlay.
sounds good to me.
AFAIK we need to keep both methods, but this should be investigated more closely.
I'm not a 100% sure why both methods need to exist. I know that _draw_overlay is needed by the new (and currently used) way of drawing the various layers in a chaco plot. Not sure what overlay is used for.
There was a problem hiding this comment.
Looking at this PR again I realize that overlay and _draw_overlay do very slightly different things. I think it is going to be worth the deeper investigation into what both are uesd for / if we can refine things down so only 1 is necessary at all.
If they are in fact both needed it is possible they will need to exist stand alone with very similar (largely copied) code, but some minor discrepancies.
There was a problem hiding this comment.
Digging into enable:
overlay is a method defined on AbstractOverlay, and it takes other_component as an argument. Effectively the overlay draws itself on top of another component.
_draw_overlay originates from Component which keeps track of a list of AbstractOverlays, and when _draw_overlay is called, it iterates over each of those overlays and calls their respective overlay methods, passing in itself as the other_component.
There was a problem hiding this comment.
So, I think what makes sense is that classes which are decedents from AbstractOverlay should only need to define overlay not _draw_overlay.
It is a bit strange because AbstractOverlay subclasses Component so it does technically have a _draw_overlay method. However, for Component _draw_overlay gets called via the _draw method. But _draw is overridden by AbstractOverlay and simply calls overlay if self.component is not None.
So, in summary, I believe we can delete all the definitions of _draw_overlay on subclasses of AbstractOverlay since they are actually unused. overlay is the method that actually does the work. Since the current classes which do override _draw_overlay do not also override _draw, nothing ever actually calls _draw_overlay.
There was a problem hiding this comment.
Looking closer, some differences between enable.AbstractOverlay and chaco.AbstractOverlay include:
- enable defines
_do_layout(default is justpass), anddo_layoutmethods, which chaco does not.do_layout(per the docstring) differs from theComponent.do_layoutmethod in that it allows passing an optionalcomponentarg. - in the
_drawmethod, chaco callssuperifself.componentisNone. enable simply returns.
Number 2 is what makes me think maybe there are scenarios in which _draw_component gets called / is needed if we try to draw an Overlay for which component is None. ie a stand alone overlay (which I will have to look for use cases / examples of ...)
There was a problem hiding this comment.
For instance, the _draw_overlay use on Tooltip and its subclass DataLabel is needed, since they override draw and call PlotComponent._draw directly.
I do not see any cases in which PlotAxis does not have a component set (every time it is instantiated in the chaco codebase a component is passed in), however theoretically one could try drawing an Axis standalone (although i can't imagine why) in which case not having _draw_overlay will cause a break...
I am of two minds if it should be removed or not for the cases of PlotAxis and Grid here. If it is to stay, because the two methods do slightly different things, I believe they will both need to stay as is with some duplication (ie overlay can't just call _draw_overlay).
There was a problem hiding this comment.
@rahulporuri I apologize in advance that this is an overly wordy brain dump, but what are your thoughts on this? I hope my findings make sense
There was a problem hiding this comment.
I can't fully digest these findings at the moment - so my instinct is to simply defer this question to a later time. Can you write up these findings in an issue? I apologize for the delay in responding to your comments - it'll probably involve additional effort to recollect everything and put together a meaningful PR.
There was a problem hiding this comment.
No worries, I will open an issue
| # This method should be folded into self._draw_plot(), but is here for | ||
| # backwards compatibilty with non-draw-order stuff. |
There was a problem hiding this comment.
This comment effectively sums up the aim of this PR
|
|
||
| def _draw_component(self, gc, view_bounds=None, mode="normal"): | ||
| super()._draw_component(gc, view_bounds, mode) | ||
| def overlay(self, other_component, gc, view_bounds=None, mode="normal"): |
There was a problem hiding this comment.
I tested this by running examples/demo/basic/regression.py and everything seems to work as expected
| if not self.visible: | ||
| return | ||
|
|
||
| self._compute_ticks(other_component) |
There was a problem hiding this comment.
This method now only differs from _draw_overlay in this line and the self._cache_valid = False line as this is what overlay had done differently previously as well
rahulporuri
left a comment
rahulporuri
left a comment
There was a problem hiding this comment.
Apart from two instances of duplication, this PR mostly LGTM
|
@aaronayres35 looks like there are merge conflicts on this PR. Can you address them? |
rahulporuri
left a comment
rahulporuri
left a comment
There was a problem hiding this comment.
LGTM.
The changes make sense to me now - given that the overlay and _draw_overlay methods do slightly different things - like you have described.
|
Looks like CI is happy and the merge conflicts have been resolved. Can you merge and backport this to the 5.0 maintenance branch? |
* remove all definitions of _draw_component * flake8 * component is not an argument for _draw_overlay
2 participants
closes #736
Note that
_draw_componenthas been removed from enable in enthought/enable#814It was a deprecated method that was a part of the old rendering approach.
These occurrences were all cases in which
_draw_componenthad been overwritten in chaco and called in a few places. This was intended to preserve backwards compatibility, but is now no longer necessary. The logic can simply be copied over into the appropriate methods (which previously called_draw_component).