Skip to content

let Qt4 backend ignore extra mouse buttons (fixes exceptions) #890

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

Closed

Conversation

hmeine
Copy link
Contributor

@hmeine hmeine commented May 23, 2012

My mouse has more than three buttons; the current qt4 backend does not cope with mouse events on the canvas for these buttons.

@pelson
Copy link
Member

pelson commented May 23, 2012

Were you intending to support the extra mouse buttons, or simply avoid the backend blowing up?

buttond = {QtCore.Qt.LeftButton : 1,
QtCore.Qt.MidButton : 2,
QtCore.Qt.RightButton : 3,
# QtCore.Qt.XButton1 : None,
Copy link
Member

Choose a reason for hiding this comment

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

I don't see why we can't provide magic numbers (or maybe even strings) for these keys.
In my eyes, since you have used the {}.get() method further on, this commented out section should either be removed or the mouse buttons supported.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah -- we probably should use strings, but that would require changing all backends to be consistent and would require any third-party code using the event system to update. Should be on a list of good "backward incompatible" changes to make some day.

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 can see, strings would just work for the new values whilst keeping the integers for the old would avoid breaking any backwards compatibility?

None the less, this doesn't need to be done here for this pull request to be useful. In its current form this pull should be considered for merging into the 1.1.x branch.

@hmeine
Copy link
Contributor Author

hmeine commented May 23, 2012

First and foremost, I wanted to avoid the exceptions. (The first commit, introducing the .get, indeed fixed this already.)

Then, I introduced the constants for readability, and figured that I should also add the two missing constants in case someone wanted to pick this up later and introduce support for the extra buttons. Leaving the commented lines in would make life easier then (in particular for someone not familiar with Qt). I could have left away the comment characters (the effect would be exactly the same), but this way it looks like unfinished code (which it should, because it is).

I am no expert in mouse buttons, so I don’t know which new constants would make sense (e.g. 4/5 vs. "x1"/"x2"). Of course one could as well define them now and at least the Qt backend would support them.

@mdboom
Copy link
Member

mdboom commented May 23, 2012

+1

@mdboom
Copy link
Member

mdboom commented May 23, 2012

I agree. Could you close this PR and reopen it rebased against the v1.1.x branch?

@hmeine
Copy link
Contributor Author

hmeine commented May 23, 2012

Never did that before, but I'll do my very best. ;-)

@hmeine hmeine closed this May 23, 2012
@hmeine
Copy link
Contributor Author

hmeine commented May 23, 2012

I need to rebase my changed (probably locally) onto upstream/v1.1.x first, right?

@mdboom
Copy link
Member

mdboom commented May 23, 2012

Yes. And then create a new PR and edit the base revision so it points to matplotlib:v1.1.x

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.

3 participants