wxWidgets / wxWidgets

Cross-Platform C++ GUI Library
https://www.wxwidgets.org/
6.01k stars 1.76k forks source link

Document how to test for wxGLCanvas creation failure #13566

Open wxtrac opened 12 years ago

wxtrac commented 12 years ago

Issue migrated from trac ticket # 13566

component: documentation | priority: normal

2011-10-15 12:59:07: @ojwb created the issue


The attached patch fixes a segfault in wxGLCanvas when GLX isn't available. Instead the problem is reported more clearly.

The issue fixed is that the wxGLCanvas constructors call wxGLCanvas::Create() which in this case fails before it calls wxWindow::Create(), and returns false, but the return value is ignored and there seems to be no way for user code to check if the window is actually valid. So the patch moves the wxWindow::Create() call up to the start of wxGLCanvas::Create() (since all the parameters passed to wxWindow::Create() are parameters of wxGLCanvas::Create() so should be valid at this point).

(This is a fix for http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=575536 which contains more gory details...)

wxtrac commented 12 years ago

2011-10-15 12:59:33: @ojwb uploaded file wxglcanvas-avoid-segv-when-glx-not-available (0.9 KiB)

patch to fix this issue

wxtrac commented 12 years ago

2011-10-16 15:54:45: @vadz commented


Unfortunately I think this patch breaks the fix of 51481 as we don't hook parent-set signal before calling Create() any more. Maybe we should just call Create() even if InitVisual() fails but keep the code unchanged otherwise?

FWIW a workaround in user code would be to use 2 step construction, i.e. create wxGLCanvas using its default ctor and then call Create() and check its return code.

wxtrac commented 12 years ago

2011-10-16 16:37:44: @ojwb commented


It certainly seems reasonable to call Create() for all code paths, but at the same place in the code as it is currently called.

Regarding the workaround, neither the default ctor of wxGLCanvas nor its Create() method seem to be documented:

http://docs.wxwidgets.org/stable/wx_wxglcanvas.html

wxtrac commented 12 years ago

2011-10-16 20:02:29: @vadz changed title from SEGV in wxGLCanvas when GLX not available to Document how to test for wxGLCanvas creation failure

2011-10-16 20:02:29: @vadz changed status from new to confirmed

2011-10-16 20:02:29: @vadz changed component from OpenGL to documentation

2011-10-16 20:02:29: @vadz commented

And it's even not in the trunk docs neither. We definitely need to document this as this is the only way to safely test for OpenGL support.

Thinking more about this I actually believe that we really shouldn't create a dummy window here. You can still test if creation succeeded even when not using Create() by checking if GetHandle() returns 0 (yes, this is ugly...). So I think the problem is really that of documentation.

wxtrac commented 12 years ago

2011-10-17 02:09:49: @ojwb commented


OK, though a segfault doesn't seem a nice response if the user fails to check this.

This reminded me that the route we use to check for OpenGL support in survex is to derive the app class from wxGLApp and then call its InitGLVisual() method. However, I'm not sure how we came up with that approach, since wxGLApp seems to also be undocumented.

wxtrac commented 12 years ago

2012-02-08 00:53:11: @oneeyeman1 commented


Default constructor and Create() is still not documented.

wxtrac commented 11 years ago

2012-09-13 08:53:24: @oneeyeman1 commented


Can we make the default constructor private and make it impossible to create GLCanvas this way? So that the only way would be to use parametrized constructor...

wxtrac commented 11 years ago

2012-09-13 12:20:51: @vadz commented


Replying to [comment:6 oneeyeman]:

Can we make the default constructor private and make it impossible to create GLCanvas this way? So that the only way would be to use parametrized constructor...

I must be missing something but I really don't see how would it help with anything. Could you please explain?

wxtrac commented 11 years ago

2012-09-13 21:23:41: @oneeyeman1 commented


Vadim, Sorry I was too tired to think straight. ;-)

What I meant to say is: Make the code so that there is only one way to make GLCanvas. Does this sounds reasonable?

wxtrac commented 11 years ago

2012-09-13 22:13:11: @vadz commented


Replying to [comment:8 oneeyeman]:

What I meant to say is: Make the code so that there is only one way to make GLCanvas. Does this sounds reasonable?

I don't know. Personally I still don't see how would it help and it would make this class inconsistent with all the other wxWindow-derived classes that have both the default and non-default ctor.

wxtrac commented 9 years ago

2014-10-10 01:28:14: @ojwb commented


It seems that the wxGLCanvas default ctor and Create() method still aren't documented on trunk, and neither is wxGLApp:

http://docs.wxwidgets.org/trunk/classwx_g_l_canvas.html http://docs.wxwidgets.org/trunk/group__group__class__gl.html