Refactor wheel_zoom_tool. Add zoom button tools #916 #4841

Merged
merged 15 commits into from Sep 22, 2016

Projects

None yet

5 participants

@timsnyder
Contributor
timsnyder commented Jul 20, 2016 edited
  • issues: fixes #916
  • tests added / passed
  • release document entry (if new feature or API change)

Refactors models/tools/gestures/wheel_zoom_tool into
a scale_range() helper provided by models/tools/zoom_util.

scale_range() is then used to add zoom_in_tool and zoom_out_tool
to models/tools/actions, implementing toolbar buttons that will
zoom

Remaining work on #916 includes:

  • add new models to bokehjs/src/coffee/api/typings/models/tools.d.ts. somehow I missed those in my initial commit
  • actual design done for icons/Zoom{In,Out}.png. I think what I slapped together is good enough until the tool icons are revisited en masse in #3250
  • ensure zoom-in and zoom-out are "bundled" won't be included in this issue, will address at some point in the future.
  • add 'factor' as a parameter on the zoom buttons. Currently, it
    is hardcoded to 10%
  • review user-guide for documentation needs
  • possibly rebase this into multiple commits. won't do since squash merge will be used per this comment by mattpap

Possible work for future (or already existing) tickets:

  • Does it make sense to expose zoom-only mode on wheel-zoom? I
    personally find it awkward to use wheel-zoom when it pans and zooms
    at the same time. This could be implemented easily now by
    programmatically eliding the 'center' parameter to scale_range().
    Perhaps it could be implemented as wheel-zoom button having a dropdown
    radio-button list like tap tool that lets the user choose to lock down the center of the plot and only scale in and out
@timsnyder timsnyder Refactor wheel_zoom_tool. Add zoom button tools #916
Refactors models/tools/gestures/wheel_zoom_tool into
a scale_range() helper provided by models/tools/zoom_util.

scale_range() is then used to add zoom_in_tool and zoom_out_tool
to models/tools/actions, implementing toolbar buttons that will
zoom

Remaining work on #916 includes:
* actual design done for icons/Zoom{In,Out}.png.  I slapped something
  together for initial concept from the wheel-zoom icon.

* ensure zoom-in and zoom-out are "bundled".  It doesn't make sense
  to have one and not the other.

  Can this be done in helpers.py and plotting.coffee helpers
  or should the object hierarchy have a zoom-buttons layer?

* add 'factor' as a parameter on the zoom buttons.  Currently, it
  is hardcoded to 10%

* review user-guide for documentation needs

* testing. scale_range() should probably have some unit tests and
  I've thought through how to do view testing of the zoom buttons

* possibly rebase this into multiple commits so that git (and github?)
  correctly display the history via praise:
  1. code movement from wheel_zoom_tool to zoom_utils
  2. code reorg and usage of locals to enable sharing with zoom buttons
  3. implement zoom buttons

  I'm not sure how picky you guys are about having clean, easy
  to follow history but I don't mind rebasing this into smaller commits.

Possible work for future (or already existing) tickets:
* Does it make sense to expose zoom-only mode on wheel-zoom? I
  personally find it awkward to use wheel-zoom when it pans and zooms
  at the same time.  This could be implemented easily now by
  programmatically eliding the 'center' parameter to scale_range().
  Perhaps it could be implemented as wheel-zoom button having a dropdown
  radio-button list like tap tool that lets the user
4c136be
@timsnyder
Contributor

This should be labeled status:WIP. I wanted to at least get the initial code I have working checked in for others to look at. As the PR description states, there are quite a few odds and ends that need to be ironed out still.

@bryevdv bryevdv added the status: WIP label Jul 20, 2016
@bryevdv bryevdv commented on an outdated diff Jul 21, 2016
bokehjs/src/coffee/models/tools/zoom_util.coffee
+ [start, end] = mapper.v_map_from_target([sy0, sy1], true)
+ yrs[name] = {start: start, end: end}
+
+ # OK this sucks we can't set factor independently in each direction. It is used
+ # for GMap plots, and GMap plots always preserve aspect, so effective the value
+ # of 'dimensions' is ignored.
+ zoom_info = {
+ xrs: xrs
+ yrs: yrs
+ factor: factor
+ }
+ return zoom_info
+
+module.exports = {
+ scale_range: scale_range
+}
@bryevdv
bryevdv Jul 21, 2016 Member

maybe move this to src/coffee/util/zoom.coffee ?

@bryevdv bryevdv and 1 other commented on an outdated diff Jul 21, 2016
...c/coffee/models/tools/gestures/wheel_zoom_tool.coffee
@@ -43,52 +53,14 @@ class WheelZoomToolView extends GestureTool.View
factor = @mget('speed') * delta
@bryevdv
bryevdv Jul 21, 2016 Member

As long as we are in this code, every standard @define property can just be accessed with @speed (when @ is the model) or @model.speed (when @ is the view) No more get or mget needed

@timsnyder
timsnyder Aug 14, 2016 edited Contributor

@bryevdv, thanks for the tip. I'll be on the lookout for that in the files I touch.

Just to be sure that I understand, is it only in cases where we call get or mget on @ that we can reduce to @prop or is it that all @define (and also @internal) properties can be directly accessed with the . operator? In other words if if we were accessing a more complex property that was itself a property using something like @get('prop').get('subprop'), could that be changed into @prop.subprop or would we still need the second get (i.e. @prop.get('subprop'))?

@bryevdv bryevdv commented on an outdated diff Jul 21, 2016
bokehjs/src/coffee/models/tools/zoom_util.coffee
@@ -0,0 +1,86 @@
+scale_range = (args) ->
+ # Util for Tools to Scale ranges of a frame for use by PlotCanvasView.update_range()
+ #
+ # args passed as an object:
+ # * args.frame
+ # * args.factor - scaling factor (clamped: -0.9 < args.factor < 0.9)
+ # * args.center - center point for scaling
+ # * args.v_axis_only - if true only scale the vertical axis
+ # * args.h_axis_only - if true only scale the horizontal axis
+ frame = args.frame
+ factor = args.factor
+ v_axis_only = args?.v_axis_only ? false
+ h_axis_only = args?.h_axis_only ? false
@bryevdv
bryevdv Jul 21, 2016 Member

in coffee script these can actually all go in the function parameter list

@mattpap mattpap commented on the diff Aug 3, 2016
bokeh/models/tools.py
@@ -392,6 +392,41 @@ class BoxZoomTool(Drag):
""")
+class ZoomInTool(Action):
+ """ *toolbar icon*: |zoom_in_icon|
+
+ The zoom-in tool allows users to click a button to zoom in
+ by a fixed amount.
+
+ .. |zoom_in_icon| image:: /_images/icons/ZoomIn.png
+ :height: 18pt
+
+ """
+ # TODO ZoomInTool dimensions should probably be constrained to be the same as ZoomOutTool
@mattpap
mattpap Aug 3, 2016 edited Contributor

Currently this is no tooling in bokeh to make this happen. Neither properties can depend on each other like this, nor there is support for button groups (otherwise there could be a ZoomInOutTool). This is a similar situation to what arises around undo and redo tools.

@timsnyder
timsnyder Aug 13, 2016 Contributor

@mattpap, at the SciPy sprint when I started looking at this, I remember @bryevdv mentioning wanting to look into a way to bundle tools that should come together. Since it impacts more than these new zoom buttons, maybe that should be broken out into its own issue. I'd be happy to discuss and/or remove the TODO from the code.

It is also entirely possible that I'm making up this idea or that I misunderstood @bryevdv

@canavandl
canavandl Sep 14, 2016 Contributor

I'd remove the TODO for now. Future work around bundling tools together should handle coalescing dimensions or other related properties, but we'll handle it then.

@timsnyder
timsnyder Sep 19, 2016 Contributor

Sounds good. I agree with you and mattpap that we shouldn't try to do all of the things here.

timsnyder added some commits Aug 13, 2016
@timsnyder timsnyder add factor property to new zoom buttons #916
The zoom button tools will now take a Percent property named factor to control
how much they zoom in or out per button click
f980693
@timsnyder timsnyder mv src/coffee/models/tools/zoom_util.coffee -> src/coffee/util/zoom.c…
…offee

As a part of #916 based on feedback from @bryevdv, move scale_range into
toplevel src/coffee/util/zoom.coffee

Also added missing require for wheel_zoom_tool.coffee. I didn't test
wheel zoom for breakage, and/or there isn't a test that caught it.
0ca945c
@timsnyder timsnyder use coffee named params in util/zoom::scale_range #916 9411e2b
@timsnyder timsnyder util/zoom::scale_range allows v_axis_only && h_axis_only
remove a TODO about possibly checking for v_axis_only && h_axis_only in util/zoom::scale_range.
The logic inside scale_range() will work (doing nothing) if both are true.  There is already
a use-case where the value of center can cause both to be true (if it is outside of the frame
boundary).  Seems like making that a hard error is unnecessary.
3c8a4d7
@timsnyder timsnyder add typings for new zoom buttons #925 e7ee20b
@timsnyder timsnyder rm superfluous get() and mget() per bev in #4841
Removing uses of get() and mget() that aren't needed now
that @define properties can be accessed via '.'
f8dbff8
@timsnyder timsnyder and 1 other commented on an outdated diff Aug 18, 2016
...c/coffee/models/tools/gestures/wheel_zoom_tool.coffee
@@ -19,20 +19,20 @@ class WheelZoomToolView extends GestureTool.View
@_scroll(e)
_scroll: (e) ->
- frame = @plot_model.get('frame')
+ frame = @plot_model.frame
hr = frame.get('h_range')
vr = frame.get('v_range')
@timsnyder
timsnyder Aug 18, 2016 Contributor

@bokeh/core, I removed quite a few uses of get() and mget() as @bryevdv requested in an earlier comment. I was surprised that CartesianFrame wouldn't let me access h_range and v_range without get().

Is this because @define_computed_property() and @define() properties are different somehow? Or, am I missing something else?

@bryevdv
bryevdv Aug 18, 2016 Member

Yes any computed properties will have to use get. Fortunately there are not many of them and the nominal plan is to get rid of computed properties entirely.

@timsnyder timsnyder commented on the diff Aug 18, 2016
bokehjs/src/coffee/core/properties.coffee
@@ -154,6 +154,10 @@ class Instance extends simple_prop("Instance", (x) -> x.properties?)
# TODO (bev) separate booleans?
class Number extends simple_prop("Number", (x) -> _.isNumber(x) or _.isBoolean(x))
+# TODO extend Number instead of copying it's predicate
+#class Percent extends Number("Percent", (x) -> 0 <= x <= 1.0)
+class Percent extends simple_prop("Number", (x) -> (_.isNumber(x) or _.isBoolean(x)) and (0 <= x <= 1.0) )
@timsnyder
timsnyder Aug 18, 2016 Contributor

Note the addition of a Percent property here. Any suggestions with the TODO would be appreciated. Or, if people think that the predicate is fine the way it is, I can delete the TODO comment.

@bryevdv
bryevdv Aug 18, 2016 Member

I think it looks fine, @mattpap is this the correct way to "extend" properties on the JS side?

@mattpap
mattpap Aug 18, 2016 Contributor
diff --git a/bokehjs/src/coffee/core/properties.coffee b/bokehjs/src/coffee/core/properties.coffee
index 0e3854f..f266b1f 100644
--- a/bokehjs/src/coffee/core/properties.coffee
+++ b/bokehjs/src/coffee/core/properties.coffee
@@ -133,11 +133,14 @@ class Property extends Backbone.Model

 simple_prop = (name, pred) ->
   class Prop extends Property
-    toString: () -> "#{name}(obj: #{@get(obj).id}, spec: #{JSON.stringify(@spec)})"
+    name: name
+    pred: pred
+
+    toString: () -> "#{@name}(obj: #{@get(obj).id}, spec: #{JSON.stringify(@spec)})"
     validate: (value) ->
-      if not pred(value)
+      if not @pred(value)
         attr = @get('attr')
-        throw new Error("#{name} property '#{attr}' given invalid value: #{value}")
+        throw new Error("#{@name} property '#{attr}' given invalid value: #{value}")

 class Any extends simple_prop("Any", (x) -> true)

@@ -154,6 +157,10 @@ class Instance extends simple_prop("Instance", (x) -> x.properties?)
 # TODO (bev) separate booleans?
 class Number extends simple_prop("Number", (x) -> _.isNumber(x) or _.isBoolean(x))

+class Percent extends Number
+  name: "Percent"
+  pred: (value) -> super(value) and (0 <= value <= 1)
+
 class String extends simple_prop("String", _.isString)

 # TODO (bev) don't think this exists python side
@@ -290,6 +297,7 @@ module.exports =
   Location: Location
   Number: Number
   Int: Number                     # TODO
+  Percent   Orientation: Orientation
   RenderLevel: RenderLevel
   RenderMode: RenderMode: Percent
@mattpap
mattpap Aug 18, 2016 Contributor

Alternatively, we could make simple_prop() smarter about inheritance.

@timsnyder timsnyder commented on the diff Aug 29, 2016
bokehjs/src/coffee/util/zoom.coffee
@@ -0,0 +1,80 @@
+scale_range = ({frame, factor, center, h_axis_only = false, v_axis_only = false}) ->
@timsnyder
timsnyder Aug 29, 2016 Contributor

@birdsarah, @bryevdv , @mattpap and anyone else who loves the tests...

I've been learning up on Mocha and sinon and looking at the tests that exist for things like CartesianFrame and Canvas. It seems like since the current implementation of scale_range depends on getting calculated properties like h_range and v_range and the mappers, testing this code is going to be challenging.

  1. Are there any JS tests that actually use a document with the solver and everything to test things like this? Would we need something like CasperJS/PhosphorJS in order to do that? Would it be worth adding those types of tests?
  2. Would it be better to stub CartesianFrame to not require that the
  3. Should I refactor scale_range so that it is a pure helper and doesn't depend on having a "solved" CartesianFrame? In other words, should I pass h_range, v_range, x_mappers and y_mappers instead of passing in frame?
@canavandl
canavandl Sep 14, 2016 edited Contributor

I think (always dangerous) that you shouldn't have to worry about stubbing any solver/canvas things because the computed ranges that you're dealing with are all model properties (and not view properties, which involve the solver and canvas stuff). So I'd be ok with passing in the frame (vs refactoring to pass in the ranges and mappers).

I added some tests to update the ranges for a wheel pan tool that you should be able to crib: https://github.com/bokeh/bokeh/blob/master/bokehjs/test/models/tools/gestures/wheel_pan_tool.coffee

@bryevdv
Member
bryevdv commented Aug 29, 2016

I think 3) is definitely a reasonable and good idea, that that function can be tested in isolation more simply. (which would be sufficient for me @birdsarah might have other thoughts)

@timsnyder
Contributor

mattpap answered my question about history cleanliness in #4907. So I won't worry about rebasing and rely on the final squash merge.

@canavandl
Contributor

hi @timsnyder

this PR is really close. All there's left is:

  • merging in master and dealing with whatever conflicts (probably related to set/get stuff)
  • writing a couple of tests JS tests
  • perhaps dealing with the new Percent property (I need to review the code more about that)

I would like to use this feature (in/out zoom tool) in a project. Do you have an idea of whether or not you'll have the availability to make the last couple of changes? If you're not going to be free in the next couple of days, would you mind adding me as a collaborator to your fork (https://github.com/blog/2247-improving-collaboration-with-forks) or letting me branch/supercede your PR?

Thanks again - this is going to be great!

@timsnyder
Contributor

@canavandl, I aim to definitely wrap it up this weekend. Might get to it before then but I went ahead and enabled the collabo checkbox. If you want to take it across the finish line, feel free.

@timsnyder timsnyder added a commit to timsnyder/bokeh that referenced this pull request Sep 19, 2016
@timsnyder timsnyder Percent property extends Number rather than copying for #4841 c8c0fdc
+ initialize: (attrs, options) ->
+ super(attrs, options)
+
+ @override_computed_property('tooltip', () ->
@birdsarah
birdsarah Sep 22, 2016 Contributor

unfortunately this is no longer available - @mattpap do you have tips for @timsnyder on how to fix.

@canavandl
canavandl Sep 22, 2016 Contributor

resolved

+ initialize: (attrs, options) ->
+ super(attrs, options)
+
+ @override_computed_property('tooltip', () ->
@birdsarah
birdsarah Sep 22, 2016 Contributor

as above

@canavandl
canavandl Sep 22, 2016 Contributor

resolved

bokehjs/src/coffee/util/zoom.coffee
+ # * v_axis_only - if true only scale the vertical axis
+ # * h_axis_only - if true only scale the horizontal axis
+
+ hr = frame.get('h_range')
@birdsarah
birdsarah Sep 22, 2016 Contributor

Any use of get also need replacing with plain frame.h_range etc.

@canavandl
canavandl Sep 22, 2016 Contributor

resolved

@birdsarah
Contributor

The following diff gets this PR working (and adds zoom_in, zoom_out tools to the color_scatter example):

diff --git a/bokehjs/src/coffee/models/tools/actions/zoom_in_tool.coffee b/bokehjs/src/coffee/models/tools/actions/zoom_in_tool.coffee
index c7c3af9..f6f1524 100644
--- a/bokehjs/src/coffee/models/tools/actions/zoom_in_tool.coffee
+++ b/bokehjs/src/coffee/models/tools/actions/zoom_in_tool.coffee
@@ -33,18 +33,9 @@ class ZoomInTool extends ActionTool.Model
   tool_name: "ZoomIn"
   icon: "bk-tool-icon-zoom-in"

-
-  initialize: (attrs, options) ->
-    super(attrs, options)
-
-    @override_computed_property('tooltip', () ->
-        @_get_dim_tooltip(
-          @tool_name,
-          @_check_dims(@dimensions, "zoom-in tool")
-        )
-      , false)
-    @add_dependencies('tooltip', this, ['dimensions'])
-
+  @getters {
+    tooltip: () -> @_get_dim_tooltip(@tool_name, @_check_dims(@dimensions, "zoom-in tool"))
+  }
   @define {
     factor: [ p.Percent, 0.1 ]
     dimensions: [ p.Array, ["width", "height"] ]
diff --git a/bokehjs/src/coffee/models/tools/actions/zoom_out_tool.coffee b/bokehjs/src/coffee/models/tools/actions/zoom_out_tool.coffee
index 30bc1a3..c66a41b 100644
--- a/bokehjs/src/coffee/models/tools/actions/zoom_out_tool.coffee
+++ b/bokehjs/src/coffee/models/tools/actions/zoom_out_tool.coffee
@@ -33,18 +33,9 @@ class ZoomOutTool extends ActionTool.Model
   tool_name: "ZoomOut"
   icon: "bk-tool-icon-zoom-out"

-
-  initialize: (attrs, options) ->
-    super(attrs, options)
-
-    @override_computed_property('tooltip', () ->
-        @_get_dim_tooltip(
-          @tool_name,
-          @_check_dims(@dimensions, "zoom-out tool")
-        )
-      , false)
-    @add_dependencies('tooltip', this, ['dimensions'])
-
+  @getters {
+    tooltip: () -> @_get_dim_tooltip(@tool_name, @_check_dims(@dimensions, "zoom-out tool"))
+  }
   @define {
     factor: [ p.Percent, 0.1 ]
     dimensions: [ p.Array, ["width", "height"] ]
diff --git a/bokehjs/src/coffee/util/zoom.coffee b/bokehjs/src/coffee/util/zoom.coffee
index bfc868b..b963b8d 100644
--- a/bokehjs/src/coffee/util/zoom.coffee
+++ b/bokehjs/src/coffee/util/zoom.coffee
@@ -8,8 +8,8 @@ scale_range = ({frame, factor, center, h_axis_only = false, v_axis_only = false}
   # * v_axis_only - if true only scale the vertical axis
   # * h_axis_only - if true only scale the horizontal axis

-  hr = frame.get('h_range')
-  vr = frame.get('v_range')
+  hr = frame.h_range
+  vr = frame.v_range


   # clamp the  magnitude of factor, if it is > 1 bad things happen
@@ -18,11 +18,11 @@ scale_range = ({frame, factor, center, h_axis_only = false, v_axis_only = false}
   else if factor < -0.9
     factor = -0.9

-  vx_low  = hr.get('start')
-  vx_high = hr.get('end')
+  vx_low  = hr.start
+  vx_high = hr.end

-  vy_low  = vr.get('start')
-  vy_high = vr.get('end')
+  vy_low  = vr.start
+  vy_high = vr.end

   frame_center = {
     x: (vx_high + vx_low) / 2.0
@@ -56,12 +56,12 @@ scale_range = ({frame, factor, center, h_axis_only = false, v_axis_only = false}
     sy1 = vy_high

   xrs = {}
-  for name, mapper of frame.get('x_mappers')
+  for name, mapper of frame.x_mappers
     [start, end] = mapper.v_map_from_target([sx0, sx1], true)
     xrs[name] = {start: start, end: end}

   yrs = {}
-  for name, mapper of frame.get('y_mappers')
+  for name, mapper of frame.y_mappers
     [start, end] = mapper.v_map_from_target([sy0, sy1], true)
     yrs[name] = {start: start, end: end}

diff --git a/examples/plotting/file/color_scatter.py b/examples/plotting/file/color_scatter.py
index f9b5cd9..08adfe8 100644
--- a/examples/plotting/file/color_scatter.py
+++ b/examples/plotting/file/color_scatter.py
@@ -10,7 +10,7 @@ colors = [
     "#%02x%02x%02x" % (int(r), int(g), 150) for r, g in zip(50+2*x, 30+2*y)
 ]

-TOOLS="hover,crosshair,pan,wheel_zoom,box_zoom,undo,redo,reset,tap,save,box_select,poly_select,lasso_select"
+TOOLS="hover,crosshair,pan,wheel_zoom,zoom_in,zoom_out,box_zoom,undo,redo,reset,tap,save,box_select,poly_select,lasso_select"

 p = figure(tools=TOOLS)

@birdsarah
Contributor

The fact that this PR passed the tests in-spite of not actually working in practice against master indicates that more tests are needed. A simple selenium test should work. Click on the zoom in button and test the range - you can see other examples of selenium tests that check the range.

@birdsarah
Contributor

I should add I'm really excited to see this feature finally come into Bokeh and am happy to help however I can especially with selenium tests.

@canavandl
Contributor

I have unit tests that I need to push.

bokehjs/src/coffee/util/zoom.coffee
[start, end] = mapper.v_map_from_target([sy0, sy1], true)
yrs[name] = {start: start, end: end}
+ debugger;
@canavandl
canavandl Sep 22, 2016 Contributor

wow. rookie mistake

@bryevdv
bryevdv Sep 22, 2016 Member

we could add a check for debugger anywhere in the code quality tests

@bryevdv
bryevdv Sep 22, 2016 Member

then mumble mumble pre-commit hook mumble mumble

@canavandl
canavandl Sep 22, 2016 Contributor

haha

@birdsarah
birdsarah Sep 22, 2016 Contributor

then mumble mumble pre-commit hook mumble mumble

I think the command is git mumble mumble pre-commit hook mumble mumble

@birdsarah
Contributor

Tested after luke's changes and looking good :)

@bryevdv bryevdv merged commit c0a42f9 into bokeh:master Sep 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bryevdv
Member
bryevdv commented Sep 22, 2016

Thanks for the PR @timsnyder !

@timsnyder
Contributor

You're welcome! Thanks everyone for the help wrapping it up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment