-
-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Dedupe implementations of configure_subplots(). #16818
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
Conversation
This causes some kind of mainloop breakage, if you call |
Should be fixed now -- basically, because the toolfig is not pyplot-managed, I needed to call show() on it explicitly. |
I guess it's still a bit weird, because it starts the mainloop now, which shows the figure immediately instead of on Actually, it doesn't quite act like |
General comment/opinion: I'm not fond of trying to fake dialogs using figures. The main motivation is that we do not need to maintain dialogs for each backend. But:
I'd prefer native dialogs and for that:
This is code that will rarely change and once we have done the one-time effort of implementing it, it's not significantly more maintainance burden than the mpl widgets approach. |
@QuLogic Can you clarify what is the failing case? TBH I always have a bit of trouble with following ion()/ioff()... @timhoffm Just to be clear, I have a strong preference for native widgets too (after all I wrote #8683 :-)). However right now the matplotlib figure "is what we have", and this PR doesn't make things worse in that respect. |
@anntzer Correct, that's why I wrote "General comment". Concerning this PR, I cannot easily review the change because I don't follow the details of figure creation. Would need to dig into it a bit further. |
Well, it's not really Previously, if you ran With this PR, it's 'half-interactive', i.e., the tool shows itself immediately, but the figure only shows on Granted, it is a bit of an edge case to call |
I'm fairly tempted to claim that indeed opening a subplot_tool without having a figure first is simply not something we really need to support. (I don't think there's a simple way to register somewhere that a non-pyplot window needs to be show()ed when pyplot.show() is called.) |
manager.set_window_title("Subplot configuration tool") | ||
tool_fig = manager.canvas.figure | ||
tool_fig.subplots_adjust(top=0.9) | ||
manager.show() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The work around there is to put the show on a 0s timer on the event loop. That will make it show as soon as the event loop is allowed to run again (which is either on return of control to the user or when plt.show()
is called).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to put in that patch? given that you seem to know what to do...
I have a slight preference for using the zero time timeout to defer calling show until after the event loop gets a chance to spin. In addition to just being a funny edge case, it makes it more likely that someone could do def my_function():
plt.configure_subplots()
time.sleep(32) # or actual work and be left with a "frozen" window. However, if there is consensus that this a weird edge case I'm not going to block merging over this. |
Pushing to 3.4 as I don't think this is urgent. |
Rely on pyplot auto-backend-detection to pop up a window with the correct canvas class (technically this means one can end up with a gtk3agg subplot tool when the main figure is gtk3cairo, but that shouldn't be a real problem...). - Qt is excluded because it has its own (native) subplot config tool. - wx needs to restore a call to Destroy() because the subplot config figure is not pyplot-managed, so `Gcf.destroy(self.num)` is a noop. - Add a reference to the subplot config figure from the subplot tool widget.
This may be nice to get in at least before the big MEP22 work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should in the future we should add a hook on the Figure / manager to spawn a new window of the same type (maybe not registered with pyplot?), but this OK to go in for now.
Rely on pyplot auto-backend-detection to pop up a window with the
correct canvas class (technically this means one can end up with
a gtk3agg subplot tool when the main figure is gtk3cairo, but that
shouldn't be a real problem...).
figure is not pyplot-managed, so
Gcf.destroy(self.num)
is a noop.widget.
PR Summary
PR Checklist