FigureCanvasQT key auto repeat

[JGoutin]

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.

[tacaswell]

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]

attn @mfitzp

[JGoutin]

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]

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]

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]

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.

[mfitzp]

@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]

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]

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]

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

[JGoutin]

@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]

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]

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)