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

Reorganize Tools #798

Open
aaronayres35 opened this issue Jun 21, 2021 · 0 comments
Open

Reorganize Tools #798

aaronayres35 opened this issue Jun 21, 2021 · 0 comments

Comments

@aaronayres35
Copy link
Contributor

aaronayres35 commented Jun 21, 2021

Related to #438
Pulling the contents of #739 into an issue:

This is the current starte of Pan and Zoom tools accross Chaco and Enable (red = in enable):

``` digraph OldToolInheritanceTree { BetterZoom -> DragZoom; BetterZoom -> BetterSelectingZoom; ZoomTool [ label=Currently just an alias for BetterSelectingZoom> ]; BetterSelectingZoom -> ZoomTool [dir=none]; RectZoom [ label=Just sets 2 traits on ZoomTool> ]; ZoomTool -> RectZoom; TrackingZoom [ label=Defines one additional method on ZoomTool> ]; ZoomTool -> TrackingZoom; DragTool [fillcolor=red, style=filled]; DragTool -> DragZoom; BaseZoomTool [fillcolor=red, style=filled]; ViewportZoomTool [fillcolor=red, style=filled]; ViewportPanTool [fillcolor=red, style=filled]; DragTool -> ViewportPanTool; BaseZoomTool -> ViewportZoomTool; PanTool; TrackingPanTool [ label=Defines one additional method on PanTool> ]; PanTool -> TrackingPanTool; PanTool2 [ label=Not in chaco.tools.api, very rarely used.
However, seems to have been intended as improvement over PanTool.> ]; DragTool -> PanTool2; } ```

image

Until recently, there had also been a BaseZoomTool and SimpleZoom living in chaco. These now exist in enable (with some trimming / modifications) and are the BaseZoomTool and ViewportZoomTool above. Additionally, we recently removed the duplicate of the enable DragTool which was living in chaco.

RectZoom and TrackingZoom are intended as convenience classes but end up clouding the api. Instead we should simply have a ZoomTool class (which is actually BetterSelectingZoom, but that code can be copied over and BetterSelectingZoom removed). This is the "default" / go-to zoom tool most users will use. If they want the current RectZoom or TrackingZoom functionality, there should simply be an easy way to configure a ZoomTool to do so. Either by setting a trait like rect=True or tracking=True, or perhaps with some class method on ZoomTool. This way it will be more obvious what tool you want if you want zoom functionality (you want the ZoomTool!) and it can be confiugred to your needs. Similar logic applies to TrackingPanTool and either PanTool. Although currently, TrackingPanTool subclasses PanTool1.

The current BetterZoom class can be renamed as BaseZoomTool. The way things currently are (with ZoomTool an alias for BetterSelectingZoom), the "default" zoom tool has selecting functionality. There are situations like DragZoom where you don't want this. AFAICT, users are not expected to use BetterZoom directly. As such, it makes sense to make it an explicit base class.

In enable, I do not really see why BaseZoomTool needs to be its own class. It is only used by ViewportZoomTool, and is not exposed in any api module. Further, from what I can tell, enable zoom functionality is only possible via a Canvas with a Viewport. So only having a ViewportZoomTool seems reasonable. However, if the class is anticipated to be used as a base class for other zoom tool variants, BaseZoomTool can easily stay.

Additionally, either pan_tool.PanTool or pan_tool2.Pantool should be removed. (I still need to investigate the feature disparity / advantages of one over the other, if any exist)

After some initial investigation it is clear much of the code is copy pasted. However, some obvious differences include:

  • PanTool1 subclasses BaseTool whereas PanTool2 subclasses DragTool
  • PanTool1 has pan_{right/left/up/down}_key traits, PanTool2 does not
  • PanTool1 also allows "middle" for drag_button, PanTool2 (inheriting drag_button from enable DragTool) only allows "left" or "right"
  • PanTool1 sets event state to "panning" and defines "panning" specific methods whereas PanTool2 overrides methods on DragTool (e.g. dragging, drag_cancel, drag_end)

Potentially relevant issues include #519, and enthought/enable#90.

The following is a proposal for a new class heirarchy:

``` digraph NewToolInheritanceTree { BaseZoomTool [ label=Formally BetterZoom> ]; ZoomTool [ label=Formally BetttterSelectingZoom / ZoomTool> ] BaseZoomTool -> ZoomTool; BaseZoomTool -> DragZoom; DragTool [fillcolor=red, style=filled]; DragTool -> DragZoom; ViewportZoomTool [fillcolor=red, style=filled]; ViewportPanTool [fillcolor=red, style=filled]; DragTool -> ViewportPanTool; PanTool [style="dotted"]; PanTool2 [ label=Not in chaco.tools.api, very rarely used.
However, seems to have been intended as improvement over PanTool.>, style="dotted" ]; DragTool -> PanTool2; } ```

image

Migration Steps:

  1. Rename BetterZoom as BaseZoomTool
  2. Copy BetterSelectingZoom into ZoomTool and delete old BetterSelectingZoom, or delete old ZoomTool and rename BetterSelectingZoom as ZoomTool
  3. Decide on means for replacing RectZoom and TrackingZoom and with functionality on ZoomTool
    • RectZoom currently just subclasses ZoomTool (aka
      BetterSelectingZoom) and sets tool_mode = "box" / always_on = True
    • TrackingZoom currently just subclasses ZoomTool (aka
      BetterSelectingZoom) and defines a normal_mouse_wheel method.
  4. Do the same for TrackingPanTool (which just subclasses PanTool and overrides _end_pan).
  5. Chose one of pan_tool.PanTool and pan_tool2.PanTool to be the go-to PanTool moving forawd. Update as needed / Delete the other.
  6. Decide fate of BaseZoomTool in enable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant