Use standard subplot window in macosx backend #21663

QuLogic · 2021-11-18T05:44:54Z

PR Summary

This seems to compile and work, but the test mac is kind of slow, so not sure if it'll break anything. It also doesn't have a retina display, so I can't actually confirm that the content is not quarter sized.

Fixes #21648

PR Checklist

Tests and Styling

[n/a] Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

[n/a] New features are documented, with examples if plot related.
[n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
[n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
[n/a] Documentation is sphinx and numpydoc compliant (the docs should build without error).

dstansby · 2021-11-18T14:49:00Z

Works for me (on a retina screen), but looks like some tests are failing on CI.

greglucas

There is some weird interaction going on with dual monitors here, as if the window stays a part of the parent window and can't span both.

When I move the subplots window to my other display it immediately hides itself from view. When I go back and click on the original figure window, the subplots window reappears at half transparency when I drag it around.
When I close the main figure window, the subplot window stays around and keeps the process alive.

I will try and look into this a little more in the next day or two, but writing this down in case anyone else has a look before that.

greglucas

This currently opens a new window every time I click the subplots_param button. On qt5agg clicking the button closes the old window and brings up a new one for me. So, it might be nice to try and add that in here as well if possible.

Of course, I'm only finding other bugs in the review here ;) this does indeed solve the HiDPI scaling issue.

greglucas · 2021-11-19T00:17:20Z

src/_macosx.m

-    PyGILState_Release(gstate);
+    NSWindow* parent = [((FigureCanvas*)canvas)->view window];
+    NSWindow* child = [((FigureCanvas*)subplot_canvas)->view window];
+    [parent addChildWindow: child ordered: NSWindowAbove];


I would remove the addChildWindow here. When it is attached to the parent the window will move around relative to it when dragged. This causes issues on dual monitors, where it is hidden from view when I drag the subplots window onto a new monitor. Removing this makes it behave as expected still I think, so I'm not sure there is a real benefit to attaching them.

But does it close when you close the figure window? Even with multiple figures open?

Ah, I see Qt doesn't handle that properly either.

Yeah, this is a tricky one. I suggested the easy way out here ;) I think you could go down a rabbit hole that probably isn't super critical.

As it currently is, closing the figure window does close the subplot window as well, but the event-loop keeps running for some reason, so my Python process doesn't end.

Then I go and click on the still-running Python process to bring it back up and all of a sudden two windows appear again, with a phantom figure window that I can't do anything with and the subplots window that does work. I can then close the subplots window and it closes the entire session then.

OK, I changed it to only set a utility window style, though I'm not sure it really had much effect.

I'll open a separate PR for the subplot window lifetime.

I'm not sure you even need that?

I get some logs output saying it isn't supported. (It didn't fail, just logged)

python[68679:18018136] NSWindow does not support utility styleMask 0x10

Oh, you're right; I missed that when testing. The docs don't seem to say that's a requirement, but okay.

Looks good to me now. I'll open some new issues with the other things we found here.

Dismissing based on this PR actually solving the HiDPI issue.

anntzer · 2021-11-20T10:23:33Z

You can also deprecate prepare_configure_subplots on the Python side.

anntzer · 2021-11-20T10:44:03Z

src/_macosx.m

-    NSRect rect;
-    int width, height;
+{
+    PyObject *mod = NULL, *base_class = NULL, *ret = NULL;


Sorry for the way-too-complicated original patch; actually, because the method has not been overridden on Python's side, you can even directly call it, I think:

{ PyObject* result; PyGILState_STATE gstate; gstate = PyGILState_Ensure(); result = PyObject_CallMethod(toolbar, "configure_subplots", ""); if(result) Py_DECREF(result); else PyErr_Print(); PyGILState_Release(gstate); }

i.e. the same implementation as home, back, etc. (I plan to merge all of these together in a single helper, in a later PR.)

I believe you're right; it seems to work on the test Mac anyway.

QuLogic · 2021-11-23T09:02:51Z

You can also deprecate prepare_configure_subplots on the Python side.

Yes perhaps, but not for 3.5.1

Fixes matplotlib#21648

anntzer

Approving on general grounds, but it would be good if a mac user checked (even if it's @greglucas again).

greglucas

Confirmed that it works as expected for me still with multi-dpi monitors.

lumberbot-app · 2021-11-23T14:25:42Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout v3.5.x
git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -m1 c19cca75e54fc4df21559c656261242e0029ca0d

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #21663: Use standard subplot window in macosx backend'

Push to a named branch:

git push YOURFORK v3.5.x:auto-backport-of-pr-21663-on-v3.5.x

Create a PR against branch v3.5.x, I would have named this PR:

"Backport PR #21663 on branch v3.5.x (Use standard subplot window in macosx backend)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

…ackend

Backport PR #21663 on branch v3.5.x (Use standard subplot window in macosx backend)

QuLogic added the OS: Apple label Nov 18, 2021

QuLogic added this to the v3.5.1 milestone Nov 18, 2021

QuLogic added GUI: MacOSX and removed OS: Apple labels Nov 18, 2021

greglucas previously requested changes Nov 18, 2021

View reviewed changes

greglucas reviewed Nov 19, 2021

View reviewed changes

QuLogic force-pushed the macosx-subplot branch 2 times, most recently from 4896d83 to d1017e0 Compare November 19, 2021 03:58

greglucas approved these changes Nov 19, 2021

View reviewed changes

greglucas mentioned this pull request Nov 19, 2021

[MNT]: macosx subplot parameters multiple windows #21680

Closed

anntzer reviewed Nov 20, 2021

View reviewed changes

Use standard subplot window in macosx backend

b4392d3

Fixes matplotlib#21648

QuLogic force-pushed the macosx-subplot branch from d1017e0 to b4392d3 Compare November 23, 2021 09:33

anntzer approved these changes Nov 23, 2021

View reviewed changes

greglucas approved these changes Nov 23, 2021

View reviewed changes

greglucas merged commit c19cca7 into matplotlib:main Nov 23, 2021

lumberbot-app bot added the Still Needs Manual Backport label Nov 23, 2021

anntzer mentioned this pull request Nov 23, 2021

Various cleanups to macosx backend. #17702

Closed

6 tasks

QuLogic deleted the macosx-subplot branch November 29, 2021 22:07

QuLogic pushed a commit to QuLogic/matplotlib that referenced this pull request Nov 29, 2021

Backport PR matplotlib#21663: Use standard subplot window in macosx b…

382bf96

…ackend

QuLogic mentioned this pull request Nov 29, 2021

Backport PR #21663 on branch v3.5.x (Use standard subplot window in macosx backend) #21806

Merged

QuLogic removed the Still Needs Manual Backport label Nov 29, 2021

QuLogic added a commit that referenced this pull request Nov 30, 2021

Merge pull request #21806 from QuLogic/macos-backport

43dad62

Backport PR #21663 on branch v3.5.x (Use standard subplot window in macosx backend)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use standard subplot window in macosx backend #21663

Use standard subplot window in macosx backend #21663

QuLogic commented Nov 18, 2021

dstansby commented Nov 18, 2021

greglucas left a comment

greglucas left a comment

greglucas Nov 19, 2021

QuLogic Nov 19, 2021

QuLogic Nov 19, 2021

greglucas Nov 19, 2021

QuLogic Nov 19, 2021 •

edited

Loading

greglucas Nov 19, 2021

QuLogic Nov 19, 2021

greglucas Nov 19, 2021

anntzer commented Nov 20, 2021

anntzer Nov 20, 2021

QuLogic Nov 23, 2021

QuLogic commented Nov 23, 2021

anntzer left a comment

greglucas left a comment

lumberbot-app bot commented Nov 23, 2021

Use standard subplot window in macosx backend #21663

Use standard subplot window in macosx backend #21663

Conversation

QuLogic commented Nov 18, 2021

PR Summary

PR Checklist

dstansby commented Nov 18, 2021

greglucas left a comment

Choose a reason for hiding this comment

greglucas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuLogic Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Nov 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuLogic commented Nov 23, 2021

anntzer left a comment

Choose a reason for hiding this comment

greglucas left a comment

Choose a reason for hiding this comment

lumberbot-app bot commented Nov 23, 2021

QuLogic Nov 19, 2021 •

edited

Loading