more samples are working

[seandepagnier]

No description provided.

[reingart]

@seandepagnier: great work!

I was in doubt too regarding the base destructor (see my comments in your previous merge)
Sorry about the virtual method there, I'm used to write python code that doesn't have this kind of issues.

Just cosmetic: according wxQT naming conventions, specific methods should be prefixed with Qt to maintain consistency with the rest of the code -and like in other ports- (specially with QtGetScrollBarsContainer, QtGetPicture, QtGetPainter, QtHandlePaintEvent, QtStoreWindowPointer, etc.)
GetHandle() was the exception because it is defined in the public interface
So, for example, I think GetQPushButton should be QtGetPushButton

Anyway, if m_qtWindow is used in derived clases, the GetQWhatever methods aren't needed, the base dtor could use the pointer directly, not calling GetHandle (avoiding future confusions/problems about this):

wxWindow::~wxWindow()
{
    ...
    // delete QWidget when control return to event loop (safer)
    if (m_qtWindow)                               // replaced GetHandle()
    {
        m_qtWindow->deleteLater();       // replaced GetHandle()
    }
}

Also, there is code already using GetHandle() if I remember correctly, and returning the correct type could be more useful.
And it is not always the case where there is just one subclass QWidget per wxWindow, for example, wxTextCtrl has two different widgets:

    QLineEdit *m_qtLineEdit;
    QTextEdit *m_qtTextEdit;

It depends on which one is NULL to determine if it is a multiline or not (see wxTextCtrl::QtGetScrollBarsContainer)

Even wxWindow could be a QWidget (generic panels) or QScrollArea (scrollable paneles), so the static casts looks a bit scarier to me and the function call a bit more verbose.

Maybe the GetQWhatever could be replaced by a macro like in M_PIXDATA in bitmaps (to avoid the function calls)

Another solution is that each subclass could implement it own destructor (like you did in wxFrame and wxMessageDialog), so the resource is destroyed explicitly where it is created, avoiding the function call and the conversions / casts.

@vadz, what do you think?

[seandepagnier]

As far as the casts.. what is wrong with them? The result is a lot more verbose than GetHandle().

Having an instance in each class, yes you can do this, and also have the virtual GetHandle. Then you need to delete them in every class. This ends up creating more lines of code to do the same thing and I see no real advantage. In any case it would do the same thing.

I'm more interested in finishing the port than reworking things that might have to change again later anyway. I got wxGLCanvas working but there is something wrong and some widget or something draws a small rectangle in the corner. Most of the dialogs work but there are some bugs.

1. listctrl -- not implemented, very important widget
2. sizers are not working correctly
3. FD monitoring not supported
4. painting has many many bugs.
5. aui not supported, and some higher level widgets crash because they were not intended to work in our paradigm. This could possibly mean changing how the pointers are handled, again

[seandepagnier]

I also want to note, there is zero performance cost for the conversions
since they are inline static casts.

On Wed, Jul 30, 2014 at 12:49 AM, Mariano Reingart <notifications@github.com

wrote:

@seandepagnier https://github.com/seandepagnier: great work!

I was in doubt too regarding the base destructor (see my comments
7bb0e11#diff-a3c307e14cc916ab52b533c6295aa2daR137
in your previous merge)
Sorry about the virtual method there, I'm used to write python code that
doesn't have this kind of issues.

Just cosmetic: according wxQT naming conventions
http://wiki.wxwidgets.org/WxQt#Naming_conventions, specific methods
should be prefixed with Qt to maintain consistency with the rest of the
code -and like in other ports- (specially with QtGetScrollBarsContainer,
QtGetPicture, QtGetPainter, QtHandlePaintEvent, QtStoreWindowPointer,
etc.)
GetHandle() was the exception because it is defined in the public
interface
So, for example, I think GetQPushButton should be QtGetPushButton

Anyway, if m_qtWindow is used in derived clases, the GetQWhatever methods
aren't needed, the base dtor could use the pointer directly, not calling
GetHandle (avoiding future confusions/problems about this):

wxWindow::~wxWindow()
{
...
// delete QWidget when control return to event loop (safer)
if (m_qtWindow) // replaced GetHandle()
{
m_qtWindow->deleteLater(); // replaced GetHandle()
}
}

Also, there is code already using GetHandle() if I remember correctly,
and returning the correct type could be more useful.
And it is not always the case where there is just one subclass QWidget
per wxWindow, for example, wxTextCtrl has two different widgets:

QLineEdit *m_qtLineEdit;
QTextEdit *m_qtTextEdit;

It depends on which one is NULL to determine if it is a multiline or not
(see wxTextCtrl::QtGetScrollBarsContainer)

Even wxWindow could be a QWidget (generic panels) or QScrollArea
(scrollable paneles), so the static casts looks a bit scarier to me and the
function call a bit more verbose.

Maybe the GetQWhatever could be replaced by a macro like in M_PIXDATA in
bitmaps (to avoid the function calls)

Another solution is that each subclass could implement it own destructor
(like you did in wxFrame and wxMessageDialog), so the resource is
destroyed explicitly where it is created, avoiding the function call and
the conversions / casts.

@vadz https://github.com/VADZ, what do you think?

—
Reply to this email directly or view it on GitHub
#43 (comment).

[reingart]

Static cast is unsafe at least in the wxTextCtrl case, as I commented prior (please correct me if I'm wrong), one of the following two functions will return a undefined / wrong pointer (QLineEdit and QTextEdit are quite different, they don't share a common base like wxTextEntryBase):

    QLineEdit *GetQLineEdit() const { return static_cast<QLineEdit *>(m_qtWindow); }
    QTextEdit *GetQTextEdit() const { return static_cast<QTextEdit *>(m_qtWindow); }

Also, GetHandle() should not be used in the dtor (m_qtWindow should be used instead), or it shouldn't not be virtual:

• to avoid other beginners do the same mistake I did, IMHO this in fact should be stated in wxWidget coding guideline
• to avoid being criticized by poor code quality / c++ bad practices (see some comments in stack overflow about this)

Also, GetQWhatever exposes too much implementation details, there is not always the case for 1:1 matching QWidget with a wxWindow, and you'll have to start moving logic outside the correct class (for example, in the text control case, or in your code, GetQScrollBar() in wxWindow::Create()). QtGetScrollBarsContainer forces something like this (for example in the paint event), and I don't know if adding more like that is a good pattern.

Finally, your're using delete again, instead of deleteLater(), this is causing segmentation fault even in the controls sample (I've compiled your branch):

Program received signal SIGSEGV, Segmentation fault.
QObject::~QObject (this=0x91ec20, __in_chrg=<optimised out>)
    at kernel/qobject.cpp:789
789 kernel/qobject.cpp: No existe el archivo o el directorio.
(gdb) bt
#0  QObject::~QObject (this=0x91ec20, __in_chrg=<optimised out>)
    at kernel/qobject.cpp:789
#1  0x00007ffff78b22de in wxQtShortcutHandler::~wxQtShortcutHandler (
    this=0x91ec20, __in_chrg=<optimised out>) at ../src/qt/window.cpp:59
#2  0x00007ffff78b230e in wxQtShortcutHandler::~wxQtShortcutHandler (

I fixed that in your last PRs (and this one) going back to deleteLater() and it is no longer reproducible, so I prefer my way, at least until I finish the GSOC.
I need something stabilized to present to Google at the end, so I cannot keep experimenting and introducing news bugs.

About your priorities:

1. listctrl -- not implemented, very important widget: yes, I planned to implement it (see issue Implement wxListCtrl #32) but I'm running out of time and I need to finish the GSOC properly. BTW, when I started to work on wxQT, the only control barely implemented was wxButton..., the others where just stubs.
2. sizers are not working correctly: I don't really know in which case, but they should be working so far (in general, as long I could test). There are still some issues with Qt sizing hints that maybe has to be solved with Qt layouts (see some discussions in wx-dev and issues)
3. FD monitoring not supported: yes, this is out of the scope of my GSOC project proposal, and indeed they seemed quite advanced for me (at least when I first reviewed the code). Anyway, the event loop should be "modernized" as that was one of the first things I did to merge the 2.9 code with the latest trunk changes
4. painting has many many bugs: see the wiki WxQtClippingProblems. I fixed most problems but I faced some apparent Qt quirks (including some "painter is not active" situations), that honestly after reading docs and debugging, I don't understand why they happen (they should be working ok in my understanding). The painting issue you mention with OpenGL could be related to the background style (see the erasing code in QtHandlePaintEvent, adapted from wxGTK but surely it needs more debugging)
5. aui not supported, and some higher level widgets crash because they were not intended to work in our paradigm. This could possibly mean changing how the pointers are handled, again: I don't really know what you mean, but this definitively is out of the scope of my GSOC project proposal

As I commented @seandepagnier (by private mail) and @vadz (in wx-dev), I'm a bit overwhelm (starting to be no comfortable at all due the GSOC deadline), and anyway, I cannot decide due my limited c++ / wx / qt experience.

Again, thanks for your contributions, waiting for @vadz response, I'll keep thinking how this can be merged / solved (specially for dd22866)

PS: please, pardon if there is any misunderstanding due my English, I'm doing my best effort

[reingart]

Please see 6ab2ff3 with my solution to remove the virtual call in wxWindow dtor (maybe it is not optimal but I think it is less disruptive at this stage).

@seandepagnie: if you could rollback dd22866 changes and use m_qtWindow pointer directly, I think we could advance and merge this PR sooner.

I've created the issue #44 to work on the SIGSEGV caused by signal handled after deletion, I think a solution like using GTKDisconnect could be doable.

Also, I splitted and commited some of your changes (b17f9e5 and f3efa9c), thanks again!

[seandepagnier]

We should not be using deleteLater. The Qt objects are created after the
wx ones, so they must be destroyed before them. Using deleteLater is
wrong, the qt widgets will send signals and events, and the handler will
not exist anymore.

In this case deleteLater causes a lot more problems than it solves. Try running the toolbar sample, and
showing and hiding the toolbar. It cannot work at all with deleteLater here, but works fine with delete.

The cases that still crash, are using helper classes which don't map to wxObjects wxQtShortcutHandler:: these need to be cleaned up differently, perhaps changing the qt parent. There are also cases with wxRichToolTips, whatever they are.

The alternative using deleteLater would be to set the handler pointer in
the signalhandler classes to null and test for it in every case, and also
manually disconnect all signals to slots. There are probably other things
needed as well. The result would be a program more confusing because first
the wxObject is created which creates a QObject that outlives it.

It's a case of a this library having a more lines of code than it needs to
do the same thing.

The static cast is safe for the textctrl, because I only call the helper I
need if you see the code. I am not using it in a way that is unsafe.
Perhaps it would be better to split this into two different helper classes.

On Wed, Jul 30, 2014 at 8:51 AM, Mariano Reingart notifications@github.com
wrote:

Please see 6ab2ff3
6ab2ff3
with my solution to remove the virtual call in wxWindow dtor (maybe it is
not optimal but I think it is less disruptive at this stage).

@seandepagnie: if you could rollback dd22866
dd22866
changes and use m_qtWindow pointer directly, I think we could advance and
merge this PR sooner.

I've created the issue #44
#44 to work on the SIGSEGV
caused by signal handled after deletion, I think a solution like using
GTKDisconnect could be doable.

Also, I splitted and commited some of your changes (b17f9e5
b17f9e5
and f3efa9c
f3efa9c),
thanks again!

—
Reply to this email directly or view it on GitHub
#43 (comment).

[seandepagnier]

By the way, great work so far.

I don't think I would have much interest in this project if it wasn't so
close to working.

Don't worry about your english, it's better than most.

On Wed, Jul 30, 2014 at 3:37 PM, sean d'epagnier seandepagnier@gmail.com
wrote:

We should not be using deleteLater. The Qt objects are created after the
wx ones, so they must be destroyed before them. Using deleteLater is
wrong, the qt widgets will send signals and events, and the handler will
not exist anymore.

It is important to understand when to use deleteLater, and when not to. In
this case it cases a lot of problems. Try running the toolbar sample, and
showing and hiding the toolbar. It cannot work at all if you use
deleteLater here, but works fine with delete.

The case of wxQtShortcutHandler:: these need to be cleaned up
differently, perhaps changing the qt parent. There are also cases with
RichToolTips, whatever they are, that cause problems.

The alternative using deleteLater would be to set the handler pointer in
the signalhandler classes to null and test for it in every case, and also
manually disconnect all signals to slots. There are probably other things
needed as well. The result would be a program more confusing because first
the wxObject is created which creates a QObject that outlives it.

It's a case of a this library having a more lines of code than it needs to
do the same thing.

The static cast is safe for the textctrl, because I only call the helper I
need if you see the code. I am not using it in a way that is unsafe.
Perhaps it would be better to split this into two different helper classes.

On Wed, Jul 30, 2014 at 8:51 AM, Mariano Reingart <
notifications@github.com> wrote:

Please see 6ab2ff3
6ab2ff3
with my solution to remove the virtual call in wxWindow dtor (maybe it is
not optimal but I think it is less disruptive at this stage).

@seandepagnie: if you could rollback dd22866
dd22866
changes and use m_qtWindow pointer directly, I think we could advance and
merge this PR sooner.

I've created the issue #44
#44 to work on the SIGSEGV
caused by signal handled after deletion, I think a solution like using
GTKDisconnect could be doable.

Also, I splitted and commited some of your changes (b17f9e5
b17f9e5
and f3efa9c
f3efa9c),
thanks again!

—
Reply to this email directly or view it on GitHub
#43 (comment).

[vadz]

@reingart I appreciate your desire for stability but we also need to be sure that the chosen approach does basically work. If @seandepagnier is right and deleteLater() really can't work, then you need to spend time on reworking this, even if it means that other stuff remains unimplemented -- it's important that we build on solid foundations.

Personally I just don't know enough about Qt to know who is right here, but I do admit feeling very suspicious about deleteLater(), it looks like a hack to me and none of the existing ports need to delay the destruction of the native windows, so it seems to me that wxQt shouldn't need to do it neither.

[seandepagnier]

We managed to fix the issue so far that it can run without crashing using
deleteLater.

I still think delete is better, and the code is also simpler, but this
allows us to experiment.

On Thu, Jul 31, 2014 at 3:37 AM, VZ notifications@github.com wrote:

@reingart https://github.com/reingart I appreciate your desire for
stability but we also need to be sure that the chosen approach does
basically work. If @seandepagnier https://github.com/seandepagnier is
right and deleteLater() really can't work, then you need to spend time on
reworking this, even if it means that other stuff remains unimplemented --
it's important that we build on solid foundations.

Personally I just don't know enough about Qt to know who is right here,
but I do admit feeling very suspicious about deleteLater(), it looks like
a hack to me and none of the existing ports need to delay the destruction
of the native windows, so it seems to me that wxQt shouldn't need to do it
neither.

—
Reply to this email directly or view it on GitHub
#43 (comment).

[reingart]

@vadz deleteLate() don't seems to be the main culprit here, as @seandepagnier said, we were chatting about this and found a bug in main window central widget handling.
Anyway, we could change to using delete with my solution easier, and it should be more safer (now I added debug warnings, but we could implement assertions like in wxGTK and other ports instead of just crashing)
I think we should experiment more before switching, as deleteLater resolved some nasty bugs in the beginning of my work with this project (including the shortcut handler one, that may persist with delete), that I want to understand better.
I'm working too on cleaning the m_qtWindow and other code duplication with a PostCreate method like in wxGTK, so wxQT get more stabilized and less error prone.

[reingart]

BTW, @vadz, the opossite is true, no other major port seems to be using delete directly -or similar- (please correct me if I'm wrong):

• wxGTK: calls gtk_widget_destroy (see bellow)
• wxMSW: DestroyWindow that sends sends WM_DESTROY message ...
• wxOSX: CFRelease in cocoa (although I don't quite understand how ~wxWidgetImpl works)
• wxMotif: XtDestroyWidget(w); that uses a two phase destroy-process to avoid dangling references

The wxGTK comment is more descriptive:

        // Note that gtk_widget_destroy() does not destroy the widget, it just
        // emits the "destroy" signal. The widget is not actually destroyed
        // until its reference count drops to zero.

Of course, then it release the reference that should be the last one.
In Qt, we could have references in the event loop, and deleteLater will remove all pending events from the queue, as it says the docs, that's why I still think it is safer.

[seandepagnier]

Vaz, I really need your opinion because I am stuck.

I tried to merge because in my tree, all the dialogs in the dialogs demo
work, the help demo, opengl, clipboard, are working. At this rate I think
the port can actually run OpenCPN in a few weeks.

In reingart's branch, none of these things, many of the same sample
programs like dialogs crash at start, and he rather use a pointer in every
class instead of a helper function which eliminated this. He made a few
changes, but left the virtual GetHelper function so there are still many
changes needed to fix, and several classes he forgot to set the base class
pointer. I don't know how to deal with this because I already fixed it
once.

I am guessing he refuses to merge because he would rather write m_qtLabel
instead of GetQtLabel() I don't care what it looks like, use a macro
or something. The problem gets worse! I got to the FileDialog, I have
a problem, and using the new "style" I must do:

m_qtWindow = m_qtDialog = m_qtDirDialog = new wxQtDirDialog( parent, this,
message, defaultPath, style);

Because m_qtWindow is used to delete, m_qtDialog is needed for exec from
ShowModel (since it's created on the stack) and m_qtDirDialog is used in
the class. Why do we do this nonsense? Every level of derived class adds
another pointer to set. Or are we supposed to keep all the extra the
virtual helper functions which are only needed in the case of 3 or more
pointers, and leave the middle pointers NULL?

If you ask any Qt developer you should nearly always using delete, and
using deleteLater in the case where you need to delete an object from
within a slot which was triggered by a signal from the object being deleted
if there are additional pending signals on that object.

The question is, can wxWidgets programs delete the widget from the event
handler triggered by the same widget? If so, we can queue the event
rather than handling it directly which is just changing a function call not
adding logic. It is probably better anyway to avoid stacking the two event
loops of qt and wx too deep in contrived cases which is probably bad.
Then go on and use delete. This is a lot simpler than these commits:

9f3e771
Add sanity checks for Qt event/signal handlers
154cde4
Avoid crash if a control is deleted and a signal is received
4386ed4
Block Qt signals if widget is being deleted

I don't think it makes sense for the QObject to outlive the wxObject that
created it, and all of this is has been a hack, and so far I think it
doesn't handle all cases anyway.

I am waiting for feedback.

On Thu, Jul 31, 2014 at 3:54 AM, Mariano Reingart notifications@github.com
wrote:

@vadz https://github.com/vadz deleteLate() don't seems to be the main
culprit here, as @seandepagnier https://github.com/seandepagnier said,
we were chatting about this and found a bug in main window central widget
handling.
Anyway, we could change to using delete with my solution easier, and it
should be more safer (now I added debug warnings, but we could implement
assertions like in wxGTK and other ports instead of just crashing)
I think we should experiment more before switching, as deleteLater
resolved some nasty bugs in the beginning of my work with this project
(including the shortcut handler one), that I want to understand better.
I'm working too on cleaning the m_qtWindow and other code duplication with
a PostCreate method like in wxGTK, so wxQT get more stabilized and less
error prone.

—
Reply to this email directly or view it on GitHub
#43 (comment).

[reingart]

@seandepagnier :

There where nothing related to delete vs deleteLater in the problems your mention with the samples in my branch:

• toolbar sample didn't work because it was deleting itself twice, removing the delete you added to it work fine: 4d3cc0e, also I added the helper that was missing to complete and handle events, see d1440fb (screenshot)
• help sample didn't work because the logic to find the main window was incorrect, adopting your patch to get parent recursively solved it 0febaa7 (screenshot)
• opengl samples didn't work because glcanvas wasn't implemented at all, applying your patches that add glcanvas.cpp and glcanvas.h now works too 60b736e, 61c2fb3 (screenshot)
• dialog samples didn't run because a invalid pixmap pointer in wxBitmap 30a72bf, again, not related to object destruction, and now it works too.

File, fonts and color dialogs don't work because they are just stubs mostly unimplemented, and I didn't applied your patches that add them.

So far I skipped your commit "Fix major issues with memory destruction, now the help sample works!" dd22866 and everything works correctly in my branch too, no crashes and even no debug messages I added to trace this destruction issues.

About my commits, sanity checks are all over wxGTK, and that port also disconnect the signals prior calling the destoy function (BTW, it is just a line), so I don't see any major issue.

About the m_qtWindow, you don't need to assign it directly anymore, you could call PostCreation() (idea taken from wxGTK too).
You will not need to call it directly either, as it is called by default from well-known base pseudo constructor and helpers (controls, frames, etc.). If there is some place not handling it correctly, please tell me, I couldn't found any, but maybe I'm missing something.
Also, I don't have problem to use GetHandle() instead of m_qtWhatever, what I don't like is adding a bunch of new GetQWhatever() methods and replacing a lot of places (I just changed 1 line in some files and reverted most of them as that seemed error prone). The other reasons are explained in previous comments.
Finally, with my code, you could change delete from deleteLater when you wish if I that proves to be really nasty (I didn't see yet a traceback or side effect that confirm that).

I could continue, but I need to go to sleep (here it is 5 AM), sorry.
Today I have to close this milestone and I really don't have more time to keep the discussion going on (in just a couple of week to GSOC ends, and I still need to do the documentation, packaging, etc.)

I wonder if you can adapt the rest of your changes not applied yet to conform to my branch, that is, skip dd22866 and rollback any change to GetHandle / GetQWhatever, that would be great.
Please, in that case, make new pull request(s) or rebase this one, as the commits will not merge cleanly since a couple days ago.

We could also left this discussion for after the GSOC, the conflict is clearly identified and your solution do not involves rewriting everything anyway.

Thanks again

[seandepagnier]

The issue is, we are both fixing the same bugs, like the invalid pixmap I
had 2 days ago, you have duplicated the effort.

Maybe I am wrong, but I always try to write the fewest number of lines of
code to make a program. I also try to make the program not allocate more
memory than it needs to. Rather than adding extra statements for sanity
checks that should never be hit (the extra 200 lines of checks to allow
using deleteLater, which is incorrect to be used for our purposes), and
storing duplicate pointers (in some cases 3 or more) for the same object
just makes the program more difficult to maintain even if it runs the same.
Also every class must have an extra virtual function accessor that I had
eliminated which uses more memory and is just more work to maintain if all
of this can be eliminated.

I am open to any solutions that use less lines of code and are easier to
maintain.

On Thu, Jul 31, 2014 at 4:26 PM, Mariano Reingart notifications@github.com
wrote:

@seandepagnier https://github.com/seandepagnier :

There where nothing related to delete vs deleteLater in the problems your
mention with the samples in my branch:

• toolbar sample didn't work because it was deleting itself twice,
  removing the delete you added to it work fine: 4d3cc0e
  4d3cc0e,
  also I added the helper that was missing to complete and handle events, see
  d1440fb
  d1440fb
  (screenshot)
• help sample didn't work because the logic to find the main window
  was incorrect, adopting your patch to get parent recursively solved it
  0febaa7
  0febaa7
  (screenshot)
• opengl samples didn't work because glcanvas wasn't implemented at
  all, applying your patches that add glcanvas.cpp and glcanvas.h now works
  too 60b736e
  60b736e,
  61c2fb3
  61c2fb3
  (screenshot)
• dialog samples didn't run because a invalid pixmap pointer in
  wxBitmap 30a72bf
  30a72bf,
  again, not related to object destruction.

File, fonts and color dialogs don't work because they are just stubs
mostly unimplemented, and I didn't applied your patches that add them.

So far I skipped your commit "Fix major issues with memory destruction,
now the help sample works!" dd22866
dd22866
and everything works correctly in my branch too, no crashes and even no
debug messages I added to trace this destruction issues.

About my commits, sanity checks are all over wxGTK, and that port also
disconnect the signals prior calling the destoy function (BTW, it is just a
line), so I don't see any major issue.

About the m_qtWindow, you don't need to assign it directly anymore, you
could call PostCreation() (idea taken from wxGTK too).
You will not need to call it directly either, as it is called by default
from well-known base pseudo constructor and helpers (controls, frames,
etc.). If there is some place not handling it correctly, please tell me, I
couldn't found any, but maybe I'm missing something.
Also, I don't have problem to use GetHandle() instead of m_qtWhatever,
what I don't like is adding a bunch of new GetQWhatever() methods and
replacing a lot of places (I just changed 1 line in some files and reverted
most of them as that seemed error prone). The other reasons are explained
in previous comments.
Finally, with my code, you could change delete from deleteLater when you
wish if I that proves to be really nasty (I didn't see yet a traceback or
side effect that confirm that).

I could continue, but I need to go to sleep (here it is 5 AM), sorry.
Today I have to close this milestone and I really don't have more time to
keep the discussion going on (in just a couple of week to GSOC ends, and I
still need to do the documentation, packaging, etc.)

I wonder if you can adapt the rest of your changes not applied yet to
conform to my branch, that is, skip dd22866
dd22866
and rollback any change to GetHandle / GetQWhatever, that would be great.
Please, in that case, make new pull request(s) or rebase this one, as the
commits will not merge cleanly since a couple days ago.

We could also left this discussion for after the GSOC, the conflict is
clearly identified and your solution do not involves rewriting everything
anyway.

Thanks again

—
Reply to this email directly or view it on GitHub
#43 (comment).

[vadz]

If things work right now in @reingart's branch, let's keep them like this until the end of the GSoC to avoid any disruptive changes at this late stage.

However I am globally still in favour of @seandepagnier's approach:

1. Having multiple pointers to the same object which must always be maintained in sync is error prone and I'm sure this will result in problems. We should definitely avoid this.
2. Using deleteLater() remains suspicious to me (and no, I disagree that the other ports do it like this, DestroyWindow() does delete the window immediately, the fact that it sends WM_DESTROY has nothing to do with this, what counts is that by the time it returns the native HWND doesn't exist any more), but it's less clear cut. In wx API you can delete a window (e.g. a button) from its own event handler. You can't directly delete a TLW though, you need to call Destroy() which implements delayed destruction internally. So perhaps we need deleteLater() for child windows. But almost certainly not for the TLWs which are already destroyed during the idle time.

[seandepagnier]

On Fri, Aug 1, 2014 at 6:26 AM, VZ notifications@github.com wrote:

If things work right now in @reingart https://github.com/reingart's
branch, let's keep them like this until the end of the GSoC to avoid any
disruptive changes at this late stage.

It is working the same now that the guards are in place. There are still
some strange crashes which unreliably occur, and never in a debugger, only
at exit in a thread qt spawns, and I believe the bug is reported to be in
xkb not qt.

However I am globally still in favour of @seandepagnier
https://github.com/seandepagnier's approach:

1. Having multiple pointers to the same object which must always be
   maintained in sync is error prone and I'm sure this will result in
   problems. We should definitely avoid this.
2. Using deleteLater() remains suspicious to me (and no, I disagree
   that the other ports do it like this, DestroyWindow() does delete the
   window immediately, the fact that it sends WM_DESTROY has nothing to
   do with this, what counts is that by the time it returns the native
   HWND doesn't exist any more), but it's less clear cut. In wx API you
   can delete a window (e.g. a button) from its own event handler. You
   can't directly delete a TLW though, you need to call Destroy() which
   implements delayed destruction internally. So perhaps we need
   deleteLater() for child windows. But almost certainly not for the TLWs
   which are already destroyed during the idle time.

Would it be a bad idea to queue the event in the qt slot rather than
calling it directly?

I'm now having issues running an application that calls Yeild() at startup
to handle events before entering the event loop. This works in the other
ports of wx, but crashes the qt port.

[vadz]

Sorry, queue which event? I'm starting to get slightly lost here, I think it would be more productive to discuss this via email on wx-dev, could you please post there?

[seandepagnier]

I tried a few times to post to wx-dev and nothing seems to appear

[reingart]

Just for reference: the pending changes (except for the conflict one) where adopted to the current coding approach and merged manually in bed605f (color, font, file and dir dialogs) by me.

Then, the following commits where necessary to stabilize dialogs: 76940df & 4e1fd1e (finally removed m_qtDialog pointer)

As with OpenGL support previously merged, this dialogs now use a mixture of the current coding approach (GetHandle and private pointer), and @seandepagnier approach (writing directly the wxWindow base protected pointer).
IMHO I think this is acceptable, but the special cases and considerations mentioned earlier in this discussion should be addressed in order to extend the new approach to all the other classes.

Thanks again!