The Thread Search makes C::B crash when changing it to disabled (see ticket 777)
https://sourceforge.net/p/codeblocks/tickets/777/ (https://sourceforge.net/p/codeblocks/tickets/777/)
This is because it sends a cbEVT_REMOVE_LOG_WINDOW event (who calls DeletePage() from the infopane which in turn deletes the page) and then tries to call Reparent() of the deleted page.
So the cbEVT_REMOVE_LOG_WINDOW name is misleading, as it deletes the page instead of removing.
I suggest creating a cbEVT_DELETE_LOG_WINDOW event with the actual behaviour of cbEVT_REMOVE_LOG_WINDOW and change the latter so it calls RemoveNonLogger() (already existent but currently unused) instead of DeleteNonLogger().
There are 12 uses of cbEVT_REMOVE_LOG_WINDOW in assorted plugins that can be simply renamed, leaving Thread Search use as is.
This patch contains the proposed changes. ThreadSearch now works as it should and the other plugins are not affected because for them the change is only a rename.
I don't know if these changes affect the SDK API.
I'm not sure adding another event and modifying all plugins is worth it...But I don't have the time to really investigate how to properly fix this.
I'm not familiar with such part, but look at the patch:
@@ -443,6 +443,9 @@
// remove a log window
extern EVTIMPORT const wxEventType cbEVT_REMOVE_LOG_WINDOW;
#define EVT_REMOVE_LOG_WINDOW(fn) DECLARE_EVENT_TABLE_ENTRY( cbEVT_REMOVE_LOG_WINDOW, -1, -1, (wxObjectEventFunction)(wxEventFunction)(CodeBlocksLogEventFunction)&fn, (wxObject *) NULL ),
+// delete a log window
+extern EVTIMPORT const wxEventType cbEVT_DELETE_LOG_WINDOW;
+#define EVT_DELETE_LOG_WINDOW(fn) DECLARE_EVENT_TABLE_ENTRY( cbEVT_DELETE_LOG_WINDOW, -1, -1, (wxObjectEventFunction)(wxEventFunction)(CodeBlocksLogEventFunction)&fn, (wxObject *) NULL ),
People will get puzzled when they see "remove" and "delete" a log window. So, maybe a better event name?
I didn't invent them, the names are chosen to match wxNotebook's method names:
- cbEVT_REMOVE_LOG_WINDOW at the end calls Notebook::RemovePage()
- cbEVT_DELETE_LOG_WINDOW at the end calls Notebook::DeletePage()
Previously the cbEVT_REMOVE_LOG_WINDOW called DeletePage() and not RemovePage(), which is a lot confusing and fooled ThreadSearch's author to think the window was still usable after removing it.
Of course the comment is too terse, I'll change them to
REMOVE: // removes the specified window from the log but does not delete it
DELETE: // removes the specified window from the log and deletes it
A change that requires changes to multiple plugins is undesirable. Especially if building the plugin with the new sdk code results in calling the wrong event!
OK, the proposal then is:
- leave the original event untouched
- create a new event cbEVT_DETACH_LOG_WINDOW so ThreadSearch stops crashing
The modified patch is attached.
I have to take a look at why this reparenting is needed. I'm not sure this is really the case. But last time I've looked at it the code looked quite complex :(
Thank you for the comments, last patch version added to ticket 777.
Maybe a better name could be: cbEVT_DESTROY_LOG_WINDOW? ;)
I don't think so, because DESTROY suggests that Window->Destroy() will be called, and in fact the purpose of this event is removing it from the log pane without destroying the window.
Detach is the verb used in wxWidgets for this action, see for example wxSizer::Detach()
https://docs.wxwidgets.org/trunk/classwx_sizer.html#a362d7d556185fe9cd1b5d24004b86518 (https://docs.wxwidgets.org/trunk/classwx_sizer.html#a362d7d556185fe9cd1b5d24004b86518)
Quote from: Miguel Gimenez on January 25, 2019, 12:11:40 PM
OK, the proposal then is:
- leave the original event untouched
- create a new event cbEVT_DETACH_LOG_WINDOW so ThreadSearch stops crashing
The modified patch is attached.
I'm testing your patch, and it works OK.
BTW, I see when I close C::B(without your patch), the C::B process is still seen in the Windows Task manager, so I have to manually select the C::B and kill it. But I can't find the reason under GDB, when I halt the GDB, I see backtrace contains some function call inside the wx library and waiting for the gui objects.
With your patch, I don't see this issue. (EDIT: the hang issue still happens with your patch, so this is not related)
Quote from: Miguel Gimenez on January 30, 2019, 04:41:23 PM
I don't think so, because DESTROY suggests that Window->Destroy() will be called, and in fact the purpose of this event is removing it from the log pane without destroying the window.
Detach is the verb used in wxWidgets for this action, see for example wxSizer::Detach()
https://docs.wxwidgets.org/trunk/classwx_sizer.html#a362d7d556185fe9cd1b5d24004b86518 (https://docs.wxwidgets.org/trunk/classwx_sizer.html#a362d7d556185fe9cd1b5d24004b86518)
Thanks for the explanation, I think Detach is a good name.
@ollydbg: I still think that we don't need this new event, so I prefer if you don't add it yet.
The problem is not that critical anyway, yes it is bad, but it is not blocking the usage of C::B for many people.
This bug should be fix in master. Hopefully I've not broken something else. Testing is welcome.
Ticket 807 https://sourceforge.net/p/codeblocks/tickets/807 (https://sourceforge.net/p/codeblocks/tickets/807) has the same root causes and probably a similar solution.
Probably, but I don't have time for this at the moment.
Probably it will be good to introduce your patch for the new event anyway...
I'll revisit this someday...