[WIP] Refactor chaco plot containers #674
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 13 commits
April 16, 2021 13:25
instead import it from plot_component module where it is already defined. we might want to move this to chaco.base or something of that sort later modified: chaco/plot_containers.py
i.e. from the chaco BasePlotContainer to the subclasses i.e. OverlayPlotContainer, StackedPlotContainer and GridPlotContainer. modified: chaco/plot_containers.py
instead of relying on BasePlotContainer which itself inherits from Container. We can do so now because we moved all of relevant traits down to the subclasses from the BasePlotContainer in the previous commit. Note that there are still a few traits on the BasePlotContainer but those are deprecated and are being removed elsewhere modified: chaco/plot_containers.py
the enable OverlayContainer inherits from the enable Container and overrides the preferred size and layout methods - which are duplicated in the chaco OverlayContainer. So, we can simply update the chaco OverlayContainer to inherit from the enable OverlayContainer - and simply override/set traits on the subclass in Chaco modified: chaco/plot_containers.py
specifically, in the methods StackedPlotContainer.get_preferred_size and StackedPlotContainer._do_layout. note that the get_preferred_size method is slightly non-trivial because the underlying enable util stacked_preferred_size does not fully cover the needs of the method - we might want to fix the underlying enable util to do so. modified: chaco/plot_containers.py
simply update the _do_layout methods in the subclasses and remove the StackedPlotContainer._do_layout method modified: chaco/plot_containers.py
by simply moving the method to the two subclasses modified: chaco/plot_containers.py
and remove them from the chaco StackedPlotContainer modified: chaco/plot_containers.py
from the baseclass StackedPlotContainer. Now, we are finally in a place where we can simply remove the chaco baseclass StackedPlotContainer completely because it now finally (almost) looks like the enable StackedPlotContainer modified: chaco/plot_containers.py
and remove the chaco StackedPlotContainer modified: chaco/plot_containers.py
and a few duplicated traits on the subclass have been removed. Sadly, it looks like there are bugs in the baseclass which need to be fixed in enable before more code can be killed modified: chaco/plot_containers.py
and we removed a bunch of traits from the subclass which are already defined in the baseclass. ideally, we would also be able to remove the get_preferred_size and persistence (__getstate__) methods from the subclass but once those are fixed in enable and a new version of enable is available, we can clean this code up further. modified: chaco/plot_containers.py
and replace the old/outdated String trait with the Str trait modified: chaco/plot_containers.py
rahulporuri
commented
Apr 16, 2021
|
|
||
| try: | ||
| from enable.api import ConstraintsContainer | ||
| except ImportError: | ||
| ConstraintsContainer = None | ||
|
|
||
| # Local relative imports | ||
| from .base_plot_container import BasePlotContainer | ||
| from .plot_component import DEFAULT_DRAWING_ORDER |
There was a problem hiding this comment.
we might want to move DEFAULT_DRAWING_ORDER somewhere else/somewhere better e.g. chaco.base instead of leaving it in chaco.plot_component
chaco/plot_containers.py
Outdated
| valign = Enum("bottom", "top", "center") | ||
|
|
||
| _cached_preferred_size = Tuple(transient=True) | ||
| return stacked_preferred_size(self, components=components) |
There was a problem hiding this comment.
if self.resizable == "":
self._cached_preferred_size = self.outer_bounds[:]
return self.outer_bounds
isn't handled by enables stacked_preferred_size. If we can update the function in enable, after the next release of enable, we will be able to get rid of this method completely. both in this class and in VPlotContainer
There was a problem hiding this comment.
ref: enthought/enable#778
chaco/plot_containers.py
Outdated
| @@ -321,51 +140,59 @@ def _do_layout(self): | |||
| else: | |||
| align = "max" | |||
|
|
|||
| return self._do_stack_layout(components, align) | |||
| return stack_layout(self, components=components, align=align) | |||
There was a problem hiding this comment.
there are bugs in HStackedContainer and the HStackedContainer._do_layout - if those get fixed, we will be able to remove this HPlotContainer._do_layout method and HPlotContainer.valign trait.
chaco/plot_containers.py
Outdated
| state = super(VStackedContainer, self).__getstate__() | ||
| if "stack_index" in state: | ||
| del state["stack_index"] | ||
| return state |
There was a problem hiding this comment.
If we can push this __getstate__ down into enable, we can get rid of these in the chaco subclasses. Note that this PR also removes a bunch of traits in the subclasses which were marked as transient in the subclasses but they are not transient in the superclasses e.g. _cached_preferred_size (which isn't even defined on the baseclasses in the first place)
|
|
||
|
|
||
| class GridPlotContainer(BasePlotContainer): |
There was a problem hiding this comment.
Note that we are now in a place where we can get rid of BasePlotContainer completely.
| simple_container_do_layout, | ||
| ) | ||
| from enable.api import Container, OverlayContainer | ||
| from enable.stacked_container import HStackedContainer, VStackedContainer |
There was a problem hiding this comment.
enable needs to expose these via the api module.
There was a problem hiding this comment.
ref: enthought/enable#779
This was referenced Apr 16, 2021
rahulporuri
pushed a commit
to enthought/enable
that referenced
this pull request
Apr 16, 2021
this might have been a simple copy-paste mistake. discovered when working on enthought/chaco#674 modified: enable/stacked_container.py
rahulporuri
pushed a commit
to enthought/enable
that referenced
this pull request
Apr 19, 2021
) this might have been a simple copy-paste mistake. discovered when working on enthought/chaco#674 modified: enable/stacked_container.py
added 2 commits
June 1, 2021 16:32
the methods in the baseclasses got fixed so the inheriting classes can remove the overridden methods note that the __getstate__ methods also got removed - but the corresponding change in enable needs to be made - specifically to make stack_index a transient trait modified: chaco/plot_containers.py
This was referenced Jun 3, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.