Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RectangleSelectionTool #162

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

RectangleSelectionTool #162

wants to merge 15 commits into from

Conversation

timdiller
Copy link
Contributor

@timdiller timdiller commented Dec 13, 2013

This PR adds a RectangleSelectionTool based on the BetterSelectingZoom tool (thanks @tonysyu for the initial implementation). As such, it includes an overlay() method and subclasses AbstractOverlay as well as BaseTool so that it can be appended both to the tools and overlays lists.

Included is a demo chaco/examples/demos/basic/rectangle_selection.py which uses the rectangle tool to select a region in one plot to display zoomed in the other plot.

image

from chaco.abstract_overlay import AbstractOverlay


class RectangleSelectionTool(AbstractOverlay, BaseTool):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know there should be not need to subclass from BaseTool in order for the RectangleSelection to work as a tool. The tools attribute is defined in Interactor and there is no constraint (If there was then it would probably be Interactor which all overlays/components have as parent).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, if we use this class as a tool it will not draw itself since tools only receive key and mouse events, but do not participate by default in drawing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inheritance is copied from BetterSelectingZoom, which subclasses AbstractOverlay and BetterZoom, which itself inherits from BaseTool and ToolHistoryMixin.

Would it be better to separate the tool and the overlay, as in RangeSelection and RangeSelectionOverlay?

@itziakos
Copy link
Member

I would be good to have basic testing for the new overlay (see example using EnableTestsAssistant in DragToolTestCase)

x2, y2 = self._screen_end + self._move_offset
rect = (x, y, x2 - x + 1, y2 - y + 1)
if self.color != "transparent":
if self.alpha:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would place this logic into a cached property (something named fill_color). It would make the code easier to understand.

@timdiller
Copy link
Contributor Author

Completing this PR probably needs to wait for a discussion on the use case and the canonical architecture for tools and overlays.

@tmreay
Copy link
Member

tmreay commented Jul 5, 2018

I'm currently working on building a tool to crop regions of an image, and this overlay has proved very useful.

We don't necessarily need to merge it because of my use case, but it should probably be documented that someone else has found this useful since it was originally written.

@notmatthancock
Copy link
Contributor

Is this PR superseded by #482 ?

@corranwebster
Copy link
Contributor

They are different enough that this probably shouldn't be closed - this looks like it is designed for selecting rectangular regions of image plots, not scatter plots.

@timdiller
Copy link
Contributor Author

It sounds like this tool could use a name change to highlight the difference from RectangularSelection.
I propose RectangleRegionSelection, with appropriate updates to docstrings to make it clear what this is good for. It might be worth adding a section in the user docs discussing the differences, similar to TableEditor and TabularEditor in traitsui. @corranwebster if that seems like a good idea, I'm willing to take it on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants