8000 FigureCanvasQT key auto repeat by JGoutin · Pull Request #6302 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

FigureCanvasQT key auto repeat #6302

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 10, 2016
Merged

FigureCanvasQT key auto repeat #6302

merged 6 commits into from
May 10, 2016

Conversation

JGoutin
Copy link
Contributor
@JGoutin JGoutin commented Apr 14, 2016

Actually, with QtBackend, auto-repeat is disabled. But auto-repeat is sometime useful (By example for implementing moving an artist with arrow keys).

This add "keyPressAutoRepeat" and "keyReleaseAutoRepeat" properties to FigureCanvasQT.

If True, theses properties enable key auto-repeat for press and release events respectively.

With this auto-repeat is still disabled by default, but can be re-enabled now.

JGoutin added 2 commits April 14, 2016 18:59
@JGoutin
Option to enable/disable key auto-repeat for QtAgg
aa66dfe 
Add to "keyPressAutoRepeat" and "keyReleaseAutoRepeat" properties to FigureCanvasQT.

If True, enable key auto-repeat for press and release events respectively.
@JGoutin
Update backend_qt5.py
e683c8c
@tacaswell tacaswell added this to the 2.1 (next point release) milestone Apr 14, 2016
@tacaswell
Copy link
Member

Shouldn't the press and release events always have the same 'auto' status?

Seems reasonable enough other wise.

Why have the _key???auto as class attributes? It seem simpler to just expose them as instance attributes. The pattern is to use rcParams for most of the global configuration state.

@tacaswell
Copy link
Member

attn @mfitzp

@JGoutin
Copy link
Contributor Author
JGoutin commented Apr 14, 2016

I split press and release events in 2 status to have more flexibility. I don't know why auto-repeat was disabled in Qt-back-end, but I think it is for a good reason. Therefore, I change only the press status when really needed in my application, and reset it to default with the final release event.

An alternative could be to have only one status, but have a "isAutoRepeat" property on Matplotlib events. But I don't know if other backends can return this information.

Previously, _key???auto was an instance attribute, but I finally make it a class attribute to be compatible with Qt4 Backend also (And any other subclass who overload __init__). But it's maybe not the best method for do it, so I can change this (and add theses attributes to backend_qt4.py to).

@mfitzp
Copy link
Member
mfitzp commented Apr 15, 2016

Thanks for your contribution @JGoutin

I'm also not entirely sure why auto-repeat was disabled on the backend but I suspect it may have been to avoid generating a large number of events (e.g. when holding down zoom). But I wonder if the recent changes to redrawing (draw_idle) may have rendered that obsolete. I'll check this.

For the PR I have similar questions:

  1. Is there a use-case for enabling auto on press, but not on release? Do you see any negative effects from changing both in your current usage? Having two options to change one feature would be less than ideal, but I may be missing something here.
  2. I also agree that exposing these as normal instance attributes would be cleaner. The Qt4 backend actually inherits from the Qt5 so you should not need to worry about compatibility (anything you add to FigureCanvasQT for Qt5 will also be in FigureCanvasQT for Qt4. I don't think you need to worry about any other backends as this is a Qt-only feature.

@tacaswell I'm not sure rcParams is the place for this as I can imagine a case of wanting to enable this feature on a per-canvas basis.

@efiring
Copy link
Member
efiring commented Apr 15, 2016

On 2016/04/14 11:11 PM, Martin Fitzpatrick wrote:

I'm also not entirely sure why auto-repeat was disabled on the backend
but I suspect it may have been to avoid generating a large number of
events (e.g. when holding down zoom).

To clarify: you are referring to the zoom/pan behavior in which holding
down the X key restricts the changes to the first axis, and Y to the
second. Correct?

JGoutin added 2 commits April 16, 2016 10:47
@JGoutin
Update backend_qt5.py
cd59be6 
Key auto-repeat for push and release in one attribute.
Auto-repeat On by default for QT5.
@JGoutin
Update backend_qt4.py
b396140 
Key auto-repeat Off by default for QT4.
@JGoutin
Copy link
Contributor Author
JGoutin commented Apr 16, 2016 •
edited
Loading

I changed status to be an instance attribute and for work on press and release events at the same time.

After some tests, I didn't find any problem for enabling auto-repeat permanently. This is maybe because Qt4 is not really efficient on drawing (Auto repeat event drawing was slow and unresponsive on my test). But it is not the case for Qt5. So, I make auto-repeat ON by default for Qt5 and OFF by default for Qt4.

@mfitzp :

  1. Not really, I did this only for have possibility to only temporary change the status for avoid possible unknown problems... But if there is no real problem, splitting it is not useful. I agree it is more clear to have only one attribute for this.

@JGoutin JGoutin changed the title Figure canvas qt key auto repeat FigureCanvasQT key auto repeat Apr 16, 2016
@mfitzp
Copy link
Member
mfitzp commented Apr 27, 2016

@efiring I don't think this affects the Pan/Zoom — it's related to catching keyboard events for custom handling of plot interaction (e.g. if you bound right-arrow to pan right, without the auto-repeat you would need to keep jabbing the key). The following snippet shows the effect on the events:

import matplotlib
matplotlib.use('Qt5Agg')
import matplotlib.pyplot as plt

fig = plt.figure()
ax = fig.add_subplot(1,1,1)
ax.plot([1,2,3],[1,2,3])

def on_key(event):
    print('you pressed', event.key, event.xdata, event.ydata)

cid = fig.canvas.mpl_connect('key_press_event', on_key)
plt.show()

With this patch, pressing a key results in "you pressed" being printed repeatedly the console. On current master, you just get the one notice.

@JGoutin Thanks again for making the changes, testing it here everything looks perfectly fine! The only outstanding question I have is with the difference in behaviour between Qt4 and Qt5. The two Qt backends are designed to function as exact drop-in replacements so a difference in behaviour isn't ideal. My vote would be to have it turned off by default in both Qt4 and Qt5. Thoughts @tacaswell @efiring?

Having tested this with Qt4 (change Qt5Agg for Qt4Agg in the snippet above) and found no problem there either I suggest that if/when it's enabled it should be enabled in both backends simultaneously. I would feel more comfortable having it out in the wild as default=off for a while before making that change in case there is some breakage here I'm not hitting.

@JGoutin
Copy link
Contributor Author
JGoutin commented Apr 27, 2016 •
edited
Loading

For some (all ?) other backends, behavior is auto-repeat "ON" because there is no difference between Repeated or first key press. Is it maybe more ideal to have the same default behavior on Qt ("ON" also for Qt5 and Qt4) ?

@tacaswell
Copy link
Member

If everything else defaults to 'on' qt should also default to 'on' (unless we find a compelling reason otherwise).

@JGoutin If you triggering draws, always use draw_idle which only triggers a mpl re-rendering when the screen really repaints and there is always blitting.

attn @pwuertz due to your work on qt performance recently.

@pwuertz
Copy link
Contributor
pwuertz commented May 1, 2016

Seems reasonable, and I'm not seeing any regressions when zooming + key press.

@JGoutin
Copy link
Contributor Author
JGoutin commented May 1, 2016

@tacaswell Yes, it's exactly for this (move an artist, with arrows). Thanks for the tips!

I set True for PyQt4 also on my commit.

@mfitzp
Copy link
Member
mfitzp commented May 8, 2016

I've now tested this on PyQt 4 & 5 and PySide (the former two on Mac, Linux, Windows, the latter on Mac) and can't see any problems resulting from this change with default set to on.

My vote is to merge. @tacaswell remind me, does this need to be rebased to a single commit, or can I merge as-is?

@tacaswell
Copy link
Member

Merge as is

On Sun, May 8, 2016, 04:00 Martin Fitzpatrick notifications@github.com
wrote:

I've now tested this on PyQt 4 & 5 and PySide (the former two on Mac,
Linux, Windows, the latter on Mac) and can't see any problems resulting
from this change with default set to on.

My vote is to merge. @tacaswell https://github.com/tacaswell remind me,
does this need to be rebased to a single commit, or can I merge as-is?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6302 (comment)

@mfitzp mfitzp merged commit b16e6f3 into matplotlib:master May 10, 2016
@JGoutin JGoutin deleted the FigureCanvasQT-KeyAutoRepeat branch May 10, 2016 16:44
@vidartf vidartf mentioned this pull request Feb 9, 2017
Closed
@anntzer anntzer mentioned this pull request Feb 17, 2018
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
GUI: Qt
Projects
None yet
Milestone
v2.1
Development

Successfully merging this pull request may close these issues.
6 participants
@JGoutin @tacaswell @mfitzp @efiring @pwuertz @mdboom
0