Skip to content

FIX: port the key handling changes from mpl #310

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

Merged
merged 1 commit into from
Mar 19, 2021

Conversation

tacaswell
Copy link
Member

ports changes from matplotlib/matplotlib#19146

closes #309

@tacaswell
Copy link
Member Author

Style checkers: 2 Tom: nil

@@ -241,6 +242,68 @@ def send_binary(self, data):
def new_timer(self, *args, **kwargs):
return TimerTornado(*args, **kwargs)

if matplotlib.__version__ < '3.4':
Copy link
Collaborator

Choose a reason for hiding this comment

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

woah neat! I had no idea this was a thing you can do

@ianhi
Copy link
Collaborator

ianhi commented Mar 18, 2021

👍 Makes sense to me.

It would also be great to have somewhere to put your test case #309 (comment) somewhere (especially for whenever a proper widget testing framework solidifies - which I think will be based on https://github.com/jupyterlab/galata ???)

A good place would be the UAT notebook from #305. But if this gets merged first lets just make sure to open a follow up issue for adding to the UAT.

@martinRenou
Copy link
Member

which I think will be based on https://github.com/jupyterlab/galata ???

That would be great indeed.

@tacaswell
Copy link
Member Author

I'm not sure what the current ipympl release schedule is, but can this go out as soon as possible? We are probably going to do mpl 3.4 in the next few days.

@ianhi
Copy link
Collaborator

ianhi commented Mar 18, 2021

I'm not sure what the current ipympl release schedule is, but can this go out as soon as possible? We are probably going to do mpl 3.4 in the next few days.

I think the release schedule has been: Something gets merged, then we wait a few days until someone remembers to make a new release. So lets release right after merging this. (Or maybe after this + #305)

@ianhi
Copy link
Collaborator

ianhi commented Mar 18, 2021

I tried this out, but it's not actually clear to me what this fixes? Is it only possible to see the bug in a non-qwerty layout?

Overall I trust that this fixes the issue though

@tacaswell
Copy link
Member Author

Without this patch, but with mpl3.4.0rcX / v3.4.x / master when you type keys you get back the key code as a string. mpl3.3 and before we shipped back the key codes and then converted to keys a (private) method on the Python side. We overhauled that code for mpl34 which required changes to both the py and js sides. ipympl re-uses the Python side logic (did not trace it through fully, but it is via inheritance) but has its own version of the js side.

This PR back-ports the js side, so that we feed mpl34 the values it expects, but that would break ipypmpl with any, ah, currently released version of Matplotlib which seems bad. So this also backports the conversion function and hides it behind a mpl version-gated at class-definition time check to selectively over-ride the private method. Thus if we do change the private function again (without changing the protocol to with the js side), then ipympl will get the improvements and old versions of mpl still work :)

@martinRenou
Copy link
Member

Thanks!

@martinRenou martinRenou merged commit e364c99 into matplotlib:master Mar 19, 2021
@tacaswell tacaswell deleted the fix_mpl34 branch March 19, 2021 14:27
@ianhi
Copy link
Collaborator

ianhi commented Mar 22, 2021

This should be in version 0.7.0 on pypi. @martinRenou does anything extra need to happen to update the conda-forge version?

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.

incompatibility with mpl 3.4
3 participants