Qt4 keys #2273

mrterry · 2013-08-02T04:31:48Z

Adds support for tab, backtab, backspace, enter, insert, delete, pause, sysreq, and clear key_press_event for QT backend. Also, enables unicode support.

closes #2264

mdboom · 2013-08-02T14:12:59Z

I the interest of trying to make sure as much as possible has a test, I wonder what we can do here. We don't really have any GUI tests at the moment, because GUI testing is hard (let's go shopping).

I wonder as a half measure if it might make sense to add a demo (if there isn't one already) that displays the matplotlib-generic key name when a key is pressed. Then at least it would be easy to test whether a particular keycode is getting passed through. We can worry about automating it later when we have a good story for GUI testing.

mrterry · 2013-08-02T14:30:04Z

I was thinking about this. For the python-implemented backends, we should be able to test some of this. You could pass mock-qt-event objects to FigureCanvasQT.keyPressEvent() and make sure the correct characters pop out the other end. It doesn't solve the issue of promising that events will get fired in the first place (a la #2262), but it is better than nothing.

Would we be be ok with adding Mock as a testing dependency?

mdboom · 2013-08-02T14:44:26Z

I'm not opposed to adding Mock as a testing dependency (it's pure Python, right?) -- but I have no experience with it. How would that look with and without Mock?

mrterry · 2013-08-02T16:19:01Z

Pure python and part of the stdlib in 3.3 (unittest.mock). I'll write the tests (hopefully this afternoon) and post mock and non-mock variants.

tacaswell · 2013-08-02T19:35:02Z

If you are going to be adding qt-based tests please fold in the one test in https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/test_backend_qt4.py

mrterry · 2013-08-02T21:18:50Z

will do.

While I'm at it, may I remove the unused and DEBUG code. pep8 it too.

mdboom · 2013-08-02T21:57:45Z

By all means.

pelson · 2013-08-12T10:19:25Z

While I'm at it, may I remove the unused and DEBUG code. pep8 it too.

This is really desirable, but I cannot stress enough how valuable it is to do these in separate PRs - it will mean we can move more quickly to merge the non controversial changes and generally results in a much more fun experience for all.

mrterry · 2013-08-14T01:31:18Z

I ended up rewriting the key handling logic to make it shorter and simpler. Also added are a broad range of test for the key handling: upper/lower ascii chars, upper/lower unicode, alt+control, ctrl+alt, and testing the order of modifiers. Tests pass on Mac. The Travis test failure is not mine. @pelson pep8 modifications will happen in a separate PR after this is merged.

I think this is ready.

mdboom · 2013-08-14T12:08:06Z

lib/matplotlib/backends/backend_qt4.py

-                        int(event.modifiers()) & modifier == modifier):
-                    key = u'{0}+{1}'.format(prefix, key)
+        mods = [p for p, m, k in MODIFIER_KEYS
+                if event_key != k and event_mods & m == m]


Minor point: is the == m part redundant?

I'm cautious of making a statement on operator precedence, but I think a pair of parentheses would be helpful here:

[event_key != k and (event_mods & m) == m]

if I'm not mistaken?

bit-wise operators have very high precedence, but the parens do make it easier to read. i'll add them.

mrterry · 2013-08-14T17:27:06Z

I went ahead and added mock as a requirement. The commits can be backed out if needed.

I also made the possibly controversial change to remove Tests.check(). It told you if nose wasn't installed (but installed it anyway), checked if nose was too old (but continued on its merry way even if ancient). Installation of Tests only depended on Test.get_config().

The new version keeps the expectation that Test.get_config() decides whether to install Tests() and requires sufficiently modern versions of the dependencies.

mrterry · 2013-08-14T18:24:35Z

Whoops. python>=3.3

I added chattier Tests (modeled on Tornado), which returns a message with which versions are found/being installed.

mrterry · 2013-09-05T01:53:54Z

My plan is to march through backends to make sure they all have roughly comparable feature sets wrt key_press_events, and add tests as I go. (Yoink and TextBoxWidget both need backspace to work). Once this gets merged, I'll move on to the next.

Is there anything more this needs? The integration with six looks like it introduced a conflict. Should I rebase off master to make the merge easy?

WeatherGod · 2013-09-05T19:19:01Z

On Wed, Sep 4, 2013 at 9:53 PM, Matt Terry notifications@github.com wrote:

My plan is to march through backends to make sure they all have roughly
comparable feature sets wrt key_press_events, and add tests as I go. (Yoink
and TextBoxWidget both need backspace to work). Once this gets merged, I'll
move on to the next.

Is there anything more this needs? The integration with six looks like it
introduced a conflict. Should I rebase off master to make the merge easy?

yes, do rebase as best as you can. The integration of six is important.

mrterry · 2013-09-05T20:23:34Z

rebased on ff227d3

WeatherGod · 2013-09-06T13:43:08Z

lib/matplotlib/tests/test_backend_qt4.py

+    qt_canvas.keyPressEvent(event)
+
+
+@cleanup


I think the cleanup decorator is only for when we are plotting

assert_correct_key makes a figure and sends it events. Does that qualify as "plotting"? All the same, I'll document this better to clear up an future issues.

ah, I missed that. Yes, that does count as "plotting".

Would it be easier to go with a test class that has a setup and teardown so we don't have to do cleanup for each test function here? Don't know if it would make a difference or not.

I think this way is more robust. A test class would presumably have a single, shared figure that all the callback test use. I don't fully trust key_press_events cough_macos_cough to actually get fired. In the event that one doesn't fire, the test will eventually time out. I don't know what cascade gets launched if I'm using a class based test with a shared figure and one of the tests times out.

That and I like functions : )

mrterry · 2013-09-11T17:42:15Z

Those failures are not mine. It looks like I rebased on a master with issues. Pre-rebase this passed cleanly. I have a local branch rebased on b4c9ced that tests cleanly. I can rerebase if that would make things clearer.

mrterry · 2013-09-11T17:42:50Z

fat fingered the close button. my appologies.

WeatherGod · 2013-09-11T17:50:12Z

Ok, LGTM, but note I have not tested this as my work machine does not have QT.

tacaswell · 2013-09-11T19:58:57Z

lib/matplotlib/backends/backend_qt4.py

-from matplotlib.backend_bases import RendererBase
-from matplotlib.backend_bases import IdleEvent
-from matplotlib.mathtext import MathTextParser
-#######


I didn't delete these when I pep8'd the file because I was worried something else would re-import them from this module.

They are not used internally within MPL. I can put them back in if needed.

Just checking that you checked before removing them, that is all.

tacaswell · 2013-09-11T20:04:57Z

I am currently using this branch re-based on master (to get the pyside fixes), I will let you know if I hit any issues today.

tacaswell · 2013-09-19T04:10:40Z

I now get interesting errors in the console if I use the media keys (XF86* family)

Traceback (most recent call last):
  File "/home/tcaswell/local_installs/lib/python2.7/site-packages/matplotlib-1.4.x-py2.7-linux-x86_64.egg/matplotlib/backends/backend_qt4.py", line 306, in keyReleaseEvent
    key = self._get_key(event)
  File "/home/tcaswell/local_installs/lib/python2.7/site-packages/matplotlib-1.4.x-py2.7-linux-x86_64.egg/matplotlib/backends/backend_qt4.py", line 352, in _get_key
    key = unichr(event_key)
ValueError: unichr() arg not in range(0x110000) (wide Python build)

mrterry · 2013-09-19T05:22:47Z

Hmm. That is very interesting. I was pretty sure that QT used utf-8 key codes. I perhaps am using unichr(key) when I should be using unicode(key, 'utf-8'). I'll take a look at this in the morning. I'll also add a test. Do you remember which key caused the error?

tacaswell · 2013-09-19T15:08:17Z

I only tested play/pause and forward.

I am also not sure how this is interacting with the OS as I thought that kde should have been snarfing those keys (as they are short cuts to control media) and they don't show up in the key history as emacs sees it.

Sorry for the terse report, I might have time this weekend to dig into it a bit deeper.

mrterry · 2013-09-19T15:59:20Z

No worries. I think I understand what is going on now. Quoth the wikipedia:

Unicode defines a codespace of 1,114,112 code points in the range 0hex to 10FFFFhex.

That's 6*4 = 24 bits per code point. If you're using 32 bit ints like QT, you have a few bits left for keyboard keys that are not unicode characters. The play button, for example. Furthermore, if you go look at the QT key codes, you'll see that Key_Play (way down at the bottom) is indeed outside the unicode codespace 0x01020004 > 0x0010ffff. The bug is that I'm blindly trying to interpret the QT key code as unicode, but QT will return non-unicode code points.

I see two ways to more forward.

drop all non-unicode code-points.
return a backend-specific key-like-objects.

I think the best answer is to do 1), but make a note of what is going on in the source. I can image cases where someone really wants to use the play button. Right now I'm not sure of the most sensible way to expose what QT is doing, and I will resist the urge to guess.

mrterry · 2013-09-19T17:16:46Z

This rebases on master to get the pyside fixes.

tacaswell · 2013-10-26T21:34:07Z

👍 LGTM

The failures on this travis are related pep8 issues that were in master when it was tested.

tacaswell · 2013-11-26T18:48:18Z

lib/matplotlib/backends/backend_qt4.py

+                         QtCore.Qt.Key_Meta: 'control',
+                         })
+    MODIFIER_KEYS[0] = ('super', QtCore.Qt.ControlModifier, QtCore.Qt.Key_Control)
+    MODIFIER_KEYS[2] = ('ctrl', QtCore.Qt.MetaModifier, QtCore.Qt.Key_Meta)


It claims there is a pep8 issuer around here.

pelson · 2013-11-27T15:57:46Z

👍 for merge once the pep8 issue is addressed. Thanks @mrterry

tacaswell · 2014-01-05T18:54:20Z

@mrterry Can you fix up the pep8 issues so we can get this into 1.4?

mdboom reviewed Aug 14, 2013
View reviewed changes

mrterry mentioned this pull request Aug 23, 2013

mpl_connect event.key has 'alt' prepended in matplotlib 1.2 on windows #2302

Closed

WeatherGod reviewed Sep 6, 2013
View reviewed changes

mrterry closed this Sep 11, 2013

mrterry reopened this Sep 11, 2013

tacaswell reviewed Sep 11, 2013
View reviewed changes

mrterry added 19 commits September 19, 2013 10:10

add extra keys to qt4 backend

26e8b9f

support for unicode key_press_event

62f564e

comments

db86c40

pull qt keys into module variables

1226094

add tests for Qt Key handling

b454f32

simplify key getting logic

b80a8e7

expose modifier key indexes

41fdfae

fix tests

b22193b

remove unused text() from qt key testing

16ed6e8

backspace tests

60407dc

parens for clarity

bf4e43d

require mock

32ddf1e

compare python versions correctly

c9721fe

Tests reports found version of nose/mock

5cfbeb4

stray paren

6770646

remove unused imports

2bea38e

whitespace

8f68252

describe origin of modifier list

ec40639

document assert_correct_key

989ce39

filter out non-unicode codepoints and add test

2359f09

tacaswell reviewed Nov 26, 2013
View reviewed changes

tacaswell mentioned this pull request Jan 22, 2014

Qt4 keys #2749

Merged

tacaswell closed this Jan 22, 2014

		qt_canvas.keyPressEvent(event)


		@cleanup

Uh oh!

Qt4 keys #2273

Qt4 keys #2273

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!