MultiCursor with additionnal optionnal horizontal bar #1811

Merged
merged 3 commits into from Mar 11, 2013

4 participants

@davidtrem

This pull request proposed an improved MultiCursor widget that allows to select either vertical (default) and/or horizontal cursor lines. Previous Multicursor was vertical bar only.
The default behavior of Multicursor is fully preserved by this pull request.

@pelson pelson commented on the diff Mar 6, 2013
lib/matplotlib/widgets.py
@@ -943,8 +954,18 @@ def __init__(self, canvas, axes, useblit=True, **lineprops):
if useblit:
lineprops['animated'] = True
- self.lines = [ax.axvline(xmid, visible=False, **lineprops)
@pelson
Matplotlib Developers member
pelson added a note Mar 6, 2013

Obviously this breaks backwards compatibility with self.lines - I don't think that's a problem, but it should be documented in doc/api/api_changes.rst. I wonder if it's worth providing a property which returns the union of self.vlines and self.hlines - on second thoughts, that doesn't really help if users want to do self.lines.append(...) so scrap the property idea.

I see...
May we keep self.lines pointing to self.vlines ?
Or replace all occurrence of self.vlines by self.lines
Both solution prevent api changes...
What would you prefer ?

@pelson
Matplotlib Developers member
pelson added a note Mar 6, 2013

I think we can just document the change. I think this is a small change which I can't see having a major impact to many codes - I'd sooner have the consistency of what you have implemented, than the inconsistency that not breaking compatibility would bring.

OK. A new commit document this small API change in doc/api/api_changes.rst

@WeatherGod
Matplotlib Developers member

For backwards compatibility, why don't we set up a property "self.lines" that would point to self.vlines, but puts out a deprecation message?

@pelson
Matplotlib Developers member
pelson added a note Mar 7, 2013

Yep. That works, nice idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson commented on the diff Mar 6, 2013
lib/matplotlib/widgets.py
show()
"""
- def __init__(self, canvas, axes, useblit=True, **lineprops):
+ def __init__(self, canvas, axes, useblit=True, horizOn=False, vertOn=True,
@pelson
Matplotlib Developers member
pelson added a note Mar 6, 2013

I'm not a fan of the horizOn and vertOn keywords - is the casing for consistency reasons with other signatures? If not, I'd be happier with horiz_lines & vert_lines or even just horizontal and vertical...

@pelson
Matplotlib Developers member
pelson added a note Mar 6, 2013

To answer myself - horizOn and vertOn are consistent with Cursor. Scrap my comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson commented on an outdated diff Mar 6, 2013
lib/matplotlib/widgets.py
@@ -926,15 +929,23 @@ class MultiCursor(Widget):
ax2.plot(t, s2)
multi = MultiCursor(fig.canvas, (ax1, ax2), color='r', lw=1)
+ #multi = MultiCursor(fig.canvas, (ax1, ax2), color='r', lw=1,
@pelson
Matplotlib Developers member
pelson added a note Mar 6, 2013

The comment needs to go - you could use your example as the primary one though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson
Matplotlib Developers member

I've not tried it out, but 👍 from me. Anybody else want to comment? @WeatherGod - I know you've done some work with the widgets in the past.

Thanks for this @davidtrem .

@NelleV

LGTM 👍

@mdboom mdboom merged commit 91eb1a9 into matplotlib:master Mar 11, 2013

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment