-
-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Make FigureCanvasWx ctor signature similar to FigureCanvasBase. #10606
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
Currently, `FigureCanvasWx` has a signature of `(parent, id, figure)`, unlike the Canvas classes of other backends, which only take `figure` as argument. Change `FigureCanvasWx` to be more similar to other backends. A similar fix will later be applied to `FigureManagerWx` to bring it in line with other Manager classes. The ultimate goal is to get rid of `new_figure_manager` and `new_figure_manager_given_figure` in the backend definitions, which can be replaced by `return FigureManager(FigureCanvas(Figure(...)), num)` and `return FigureManager(FigureCanvas(fig, num))` respectively. Also deprecate the aliases `FigureCanvasWx.frame` and `.tb` (which are named `.window` and `.toolbar` in all other backends).
I have not yet looked at the details. The flow for standalone windows:
The flow for embedding:
Maybe it would be the best to separate the canvas class for standalone windows and for embedding if it helps to keep the standalone code clean. |
The model I would like to stick to is the one of the Qt and Gtk backends, where the FigureManager class is passed in a canvas and responsible for instantiating a window, then reparents the window into the canvas. The benefit is the one mentioned above: making the setup code as similar as possible between backends. If that's so much against "normal" wx behavior, then perhaps separate classes for embedding and for standalone use could be a solution, indeed. |
I don't know about Gtk, but for Qt calling Really, re-parenting is anything else than standard. Things should be kept simple and straightforward (the Qt embedding example could be much more concise to make things clear). Btw. |
For Qt calling setParent explicitly is non-standard, but you can create widgets and windows without any parent and add them to a container; they will then be directly reparented to the containing widget/window (which is what we do via setCentralWidget). As far as I can tell this is fairly standard. The use of GetTopWindow in wx is because I could not find a way to create a window/widget on wx without any parent (like for Qt), so that was the only thing I could parent to. |
IMHO the Qt example should be updated accordingly. A wx canvas should follow the wx requirements. Reparenting is not a good idea. There's absolutely no point for FigureCanvasWx not accepting or requiring a parent as argument when being embedded. The id argument is not really useful with Python, but any other wx widget would accept it. |
The qt example already creates canvases without parents: https://github.com/matplotlib/matplotlib/blob/master/examples/user_interfaces/embedding_in_qt_sgskip.py#L34 The point is that with Qt, you more or less always need to 1) create the widget and 2) insert it into a container. Because step 2 will reparent the widget to the container, there's no need to indicate the parent in step 1). See e.g. the example given in the "official" docs: http://doc.qt.io/qt-5/qhboxlayout.html#details Again if that's so unnatural for wx then we should think of some other design. |
Haven't looked too much into this yet, but wx widgets always seemed to need
a parent widget, unlike other gui toolkits. We need to make sure that
people who do embedding still have a reasonable way of specifying their own
parent widget for the canvas.
Also, this will need a deprecation cycle or two as it is a public API.
…On Tue, Feb 27, 2018 at 4:48 PM, Antony Lee ***@***.***> wrote:
The qt example already creates canvases without parents:
https://github.com/matplotlib/matplotlib/blob/master/
examples/user_interfaces/embedding_in_qt_sgskip.py#L34
The point is that with Qt, you more or less always need to 1) create the
widget and 2) insert it into a container. Because step 2 will reparent the
widget to the container, there's no need to indicate the parent in step 1).
See e.g. the example given in th "official" docs: http://doc.qt.io/qt-5/
qhboxlayout.html#details
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#10606 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-HuMGtj3lF-Bp0aIA8905lGPmH-fks5tZHgqgaJpZM4SSvai>
.
|
The PR proposed something with a normal deprecation warning and indicated how to specify the parent (via Reparent), so @WeatherGod's points are taken into account. (But that doesn't help with the un-wx-ishness pointed out by @DietmarSchwertberger.) |
Your Qt example looks much clearer than what I've found on the current documentation page (https://matplotlib.org/examples/user_interfaces/embedding_in_qt5.html) For wx the widget will actually be created on the parent window. It could (in theory) already receive events which would then propagate to the wrong parent and if that is deleted at the same time a core dump could occur. Sure, most of the time there will be no problems, but I would very much prefer to keep the arguments and avoid re-parenting. For how a standalone window is created, I can't tell much, but also there the call It's a bit non-standard that FigureCanvasWx does not accept a size or position argument, but I would say that this is acceptable if it results in more standardization within Matplotlib. If FigureCanvasWx was written from scratch, it would probably be the best to use *args and **kwargs and accept e.g. size instead of figure. |
https://matplotlib.org/devdocs/gallery/user_interfaces/embedding_in_qt_sgskip.html for the new example. If you consider that even for the standalone case we should not rely (even purely internally) on reparenting then there's probably nothing that can be done to harmonize the backends. |
I've just checked. The I'm not sure about the toolbar handling in general. Currently the toolbar is added to the frame sizer instead of calling |
The entry points from mpl's pov into the backends are indeed |
Would it be an option to modify The use cases: Alternatively, would it be possible to replace It's complicated to make things simple... |
@DietmarSchwertberger Looking at this again I am a bit confused. Can you point to any docs online that argue against Reparent()ing widgets? Googling only gave me a couple of (fairly old) bug reports with Reparent() which were basically all resolved by patches to wxWidgets, and I didn't see a single instance where the wxWidgets maintainers said "don't use Reparent()" (e.g. https://trac.wxwidgets.org/ticket/2424 http://trac.wxwidgets.org/ticket/15237 https://trac.wxwidgets.org/ticket/17980). Of course I may have missed other references... On the other hand one could indeed make FigureCanvas nicer to use with wxWidgets (or other toolkits when embedding, in fact) by 1) making the |
@RobinD42 Any chance you could comment on this issue? Namely, is it OK to reparent wx widgets? This would help making the API more consistent across backends, as all other backends create their canvases with no parent and later reparent it as needed. |
In general using Another thing to watch out for is if the temporary parent is to be destroyed after the reparenting, it should be done only if there are not any pending events for the old parent or the child that was moved. Using |
Great, thanks for your input! |
To make it clear: Code as of now:
This PR will often require code like this:
The Freeze/Thaw calls are required to avoid flickering. See attached file which will create flickering, at least on Windows. IMHO that's a big mess. Please don't do it. It does not have any advantage. There's no cross-toolkit code anyway when it comes to widget creation. An improvement would look like that, i.e. without the figure argument:
The figure argument might make sense in an interactive environment, but not for embedding where plotting often starts a long time after creating the canvas. |
Thanks for providing a clarifying example. As I mentioned above, I was really unable to find any reference anywhere online warning against Reparent()ing in wx; at least now that gives me something to try (except that I don't have graphical access to a Windows machine these days due to the lockdown, but I'll try your code once I can). I'm also curious whether this flickering is/should be considered a bug? At least it doesn't occur locally (Arch+wxGTK). Also I guess I wonder how things work with AUI, given that @RobinD42 says it depends on Reparent()ing?
My goal is not to do things that people don't like, I am genuinely just trying to keep things as simple as possible, but not more.
Qt and GTK and Tk all share (basically) the same canvas and manager creation path ( |
Well, it's pefectly possible, but nobody would do this usually, so why should there be a warning? Btw. the Freeze/Thaw does not fix flickering in this example. This might be a bug. Usually, Freeze/Thaw is working, but on several occasions I had to remove it as it made things unstable.
AUI is for dragging e.g. palettes or subwindows around, so the window is moved from one to another. The flickering takes place where the user drags a window. So it's not an issue here.
This change does not make anything simpler. A signature compatible to normal wx widgets would be a simplification. Currently always glue code in the GUI creation is required which should not be required or better with the application logic (figure creation). E.g. here are wxGlade screenshots of how to embed a matplotlib canvas:
And all steps before and after are toolkit specific... |
Well, it's pefectly possible, but nobody would do this usually, so why should there be a warning? Btw. the Freeze/Thaw does not fix flickering in this example. This might be a bug. Usually, Freeze/Thaw is working, but on several occasions I had to remove it as it made things unstable.
AUI is for dragging e.g. palettes or subwindows around, so the window is moved from one to another. The flickering takes place where the user drags a window. So it's not an issue here.
This change does not make anything simpler. A signature compatible to normal wx widgets would be a simplification. Currently always glue code in the GUI creation is required which should not be required or better with the application logic (figure creation). E.g. here are wxGlade screenshots of how to embed a matplotlib canvas:
And also, all steps before and after are toolkit specific... Sorry, I'm not willing to spend more time on issues which are obvious. I have better things to do. |
If you look at the docs for wxWindow::Reparent() (https://docs.wxwidgets.org/3.0/classwx_window.html#a7977b749284e65aecfed2ce146799cb9) it doesn't say anything. Compare with e.g. the SafelyProcessEvent doc immediately below, which does warn that that function shouldn't be used. Or even with the corresponding function in Qt, QWidget::setParent() (https://doc.qt.io/qt-5/qwidget.html#setParent), which warns that it's unlikely the user needs to call this function. Certainly the lack of warning doesn't necessarily mean that a function is safe, but it is a strong hint in that direction, especially when other functions are explicitly documented as unsafe/unneeded. The fact that nowhere else online could I find a warning against reparenting is also a hint in that direction -- again, not a proof, but a hint. The fact that the author of wxPython considers Reparent()ing "okay" is also a hint in the same direction.
Again, I thank you for providing that example, which I will try on Windows ASAP (not right now for the practical reason explained above, which is beyond my control...). Before that, I simply had no practical example of case where Reparent()ing would be bad.
Again, the practical examples you give are useful. I have written a fair amount of Qt GUIs, but not used graphical GUI builders (whether for Qt or for wx), so it isn't obvious to me what they require.
I would like to believe that one of Matplotlib's strong points is its ability to push back GUI-specific code as far as possible. Obviously a barrier has to be drawn somewhere; the only question is where.
I can only hope that you realize I am making a genuine effort in trying to understand why certain patterns would not be suitable for wx, when most of the evidence available points in the other direction. |
Also, trying to keep this discussion constructive... an alternative API which may work for all cases and not rely at all on reparenting may be along the lines of (untested) diff --git i/lib/matplotlib/backends/backend_wx.py w/lib/matplotlib/backends/backend_wx.py
index 9d4b773e7..8ae427b06 100644
--- i/lib/matplotlib/backends/backend_wx.py
+++ w/lib/matplotlib/backends/backend_wx.py
@@ -7,6 +7,7 @@ Hunter (jdhunter@ace.bsd.uchicago.edu).
Copyright (C) Jeremy O'Donoghue & John Hunter, 2003-4.
"""
+from collections import namedtuple
import logging
import math
import pathlib
@@ -436,6 +437,9 @@ class GraphicsContextWx(GraphicsContextBase):
alpha=int(a))
+_DelayedFigureCanvasWx = namedtuple("_DelayedFigureCanvasWx", "cls figure")
+
+
class _FigureCanvasWxBase(FigureCanvasBase, wx.Panel):
"""
The FigureCanvas contains the figure and does event handling.
@@ -508,6 +512,13 @@ class _FigureCanvasWxBase(FigureCanvasBase, wx.Panel):
wx.WXK_NUMPAD_DELETE: 'delete',
}
+ def __new__(cls, arg0, *args, **kwargs):
+ if isinstance(arg0, wx.Object):
+ return super().__new__(cls, *args, **kwargs)
+ else:
+ assert isinstance(arg0, Figure) and not args and not kwargs
+ return _DelayedFigureCanvasWx(cls, arg0)
+
def __init__(self, parent, id, figure):
"""
Initialize a FigureWx instance.
@@ -1040,6 +1051,8 @@ class FigureManagerWx(FigureManagerBase):
"""
def __init__(self, canvas, num, frame):
+ if isinstance(canvas, _DelayedFigureCanvasWx):
+ canvas = canvas.cls(self, -1, canvas.figure)
_log.debug("%s - __init__()", type(self))
FigureManagerBase.__init__(self, canvas, num)
self.frame = frame Now you can still construct FigureCanvasWx using the wx-standard signature (or did you want to make other changes to it? e.g. support attaching the Figure instance later?), but internally Matplotlib can still call |
I finally had a chance to test @DietmarSchwertberger's example on Windows and can confirm the flickering problem there, so I agree that the approach here is not directly acceptable. @RobinD42 do you have any suggestions about this? |
IIUC, if the change allows the developer a way to create the UI object immediately without needing to reparent it later, then I think it sounds like a good idea and should satisfy @DietmarSchwertberger's concerns. One change I suggest, however, is to check if the |
The point is to allow Matplotlib to create some kind of canvas-like object (which is actually just a placeholder), which can be passed to the figure manager (which will actually be instantiating the correct canvas). The main reason to do so is that right now it is rather tricky to write new backends. For example, in mplcairo, I have on the one hand FigureCanvas = FigureCanvasWxCairo (which inherits from the base FigureCanvasWx for the UI parts and from my own FigureCanvasCairo for the rendering parts) -- that's fine, that's the point of the backend --, but I additionally need to write a couple of factory functions (new_figure_manager(), new_figure_manager_given_figure()) which abstract out how to create a correct FigureManager for that backend: for most backends these are just |
Superseded by #22925 (see discussion re: API design in linked issues). |
Currently,
FigureCanvasWx
has a signature of(parent, id, figure)
,unlike the Canvas classes of other backends, which only take
figure
asargument. Change
FigureCanvasWx
to be more similar to other backends.A similar fix will later be applied to
FigureManagerWx
to bring it inline with other Manager classes.
The ultimate goal is to get rid of
new_figure_manager
andnew_figure_manager_given_figure
in the backend definitions, which canbe replaced by
return FigureManager(FigureCanvas(Figure(...)), num)
andreturn FigureManager(FigureCanvas(fig, num))
respectively.Also deprecate the aliases
FigureCanvasWx.frame
and.tb
(which arenamed
.window
and.toolbar
in all other backends).PR Summary
PR Checklist