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

Miscellaneous stream improvements #1436

Merged
merged 6 commits into from
May 15, 2017
Merged

Miscellaneous stream improvements #1436

merged 6 commits into from
May 15, 2017

Conversation

jlstevens
Copy link
Contributor

@jlstevens jlstevens commented May 14, 2017

This PR aims to implement the suggestions in issue #1426.

The first commit distinguishes SingleTap from Tap and DoubleTap where Tap is supposed to capture all tap events.

@bryevdv Unfortunately I can't quite implement the intended semantics for Tap as a double tap event should resolve into two taps. As far as I can tell, the way hammer is used in bokeh does not allow for this.

Is this correct? Would it be reasonable to add a bokeh event that reflects all taps, whether single or part of a double tap?

@jlstevens jlstevens added this to the v1.7.1 milestone May 14, 2017
@jlstevens
Copy link
Contributor Author

Here is an example (including all the code!) of a little visualization using the new Draw stream:

draw

@jlstevens
Copy link
Contributor Author

Note that even after having updated the skip conditions, odd things happen when using %%opts Path [toolbar=None] in the example shown above. Needs investigating.

@jbednar
Copy link
Member

jbednar commented May 15, 2017

Looks super useful, thanks! Is the three-pronged "if" statement needed because a Curve cannot be empty? Avoiding tricky logic like that seems like a good reason to allow empty Curves...

@philippjfr
Copy link
Member

philippjfr commented May 15, 2017

Is the three-pronged "if" statement needed because a Curve cannot be empty?

I think in this case it's to start a new curve every time a new drag even is started. That said in most cases we do support empty Elements, e.g. you can do hv.Curve([]).

@jlstevens
Copy link
Contributor Author

I think in this case it's to start a new curve every time a new drag even is started.

That's right! If you can suggest a way to shorten the example, I would be very happy to try it out.

@jlstevens
Copy link
Contributor Author

Anyway, in my last commit I added the JS to clip the x,y values to the axis ranges for PointerXY. The JS is pretty horrible so I want to clean it up before this PR is merged. Otherwise, I have implemented all the proposal I made in the issue.

@philippjfr
Copy link
Member

The JS is pretty horrible so I want to clean it up before this PR is merged.

It's not pretty but I'm not sure how it can be improved.

@jlstevens
Copy link
Contributor Author

It's not pretty but I'm not sure how it can be improved.

If nothing else, I can improve the formatting. After that, I think this PR is ready to merge though it might be worth investigating the odd behavior when toolbar=None...

@jlstevens
Copy link
Contributor Author

jlstevens commented May 15, 2017

To make it easier to test, here is the text of the gif example above:

from holoviews.streams import Draw
vals, curves = [], {}

def callback(x,y, stroke_count):
    global vals, curves
    curve = hv.Curve(vals)
    if len(curves)==0:
        curves[stroke_count] = hv.Curve([(x,y)])
    elif stroke_count != (len(curves)-1):
        curves[stroke_count]= curve
        vals = []
    else:
        vals.append((x,y))
        curves[stroke_count]= curve

    return hv.Path([curve.data for curve in curves.values()])

hv.DynamicMap(callback, streams=[Draw(x=0,y=0)])

It is also worth noting that we decided to call the future stream used for actually dragging a glyph/element around (i.e drag + selection) Drag.

@jlstevens
Copy link
Contributor Author

@philippjfr Other than the odd behavior with toolbar=None (which I don't know what to make of) I think this PR is ready for review.

@philippjfr
Copy link
Member

Should I go ahead and merge? Toolbar option is fixed in #1442.

@jlstevens
Copy link
Contributor Author

Should I go ahead and merge?

Given that the toolbar issue is fixed, yes please!

@philippjfr philippjfr merged commit 92a909b into master May 15, 2017
@philippjfr philippjfr deleted the stream_improvements branch May 25, 2017 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants