fix wxAuiManager interfering with docview event propagation #25336

wsu-cb · 2025-04-17T03:46:13Z

No description provided.

wsu-cb · 2025-04-17T11:52:41Z

The failed check says that awk is missing from the build system. I'm pretty sure that is unrelated to this change.

vadz · 2025-04-17T12:14:06Z

The failed check says that awk is missing from the build system. I'm pretty sure that is unrelated to this change.

Yes, see #25337.

vadz

Thanks, but while I have just a minor comment about the first commit, I don't think the second one is correct.

src/common/event.cpp

include/wx/aui/framemanager.h

vadz · 2025-04-27T21:08:33Z

BTW, I don't know if I should apply the first commit (with its fixup) already?

wsu-cb · 2025-04-28T01:21:23Z

BTW, I don't know if I should apply the first commit (with its fixup) already?

I would be inclined to wait until we get the iteration cleanup fixed as well, since I'm pretty sure applying the Unbind() part of the first commit will cause more cases of iteration problems.

vadz

Thanks, this looks good to me now!

Ideal would be to add a test (to tests/events/propagation.cpp or some other file in this directory) verifying that this now works correctly, but I am not sure if there is a relatively simple way to do it. If you can think of anything, please consider adding it because this code is clearly rather fragile and having a test checking that this doesn't get broken again would be very useful.

TIA!

include/wx/event.h

src/common/event.cpp

wsu-cb · 2025-04-30T12:59:25Z

Ideal would be to add a test (to tests/events/propagation.cpp or some other file in this directory) verifying that this now works correctly, but I am not sure if there is a relatively simple way to do it.

It looks to me like I can create a DocViewAui test parallel to the existing DocView test that can test the original event propagation problem. I think that will also test the Unbind() cleanup once this PR is applied since that problem appeared once I implemented the first commit. I have converted this to a draft while I work on the tests.

wsu-cb · 2025-05-01T01:31:25Z

I have successfully created a DocViewAui test (3471668) that fails without the changes in this PR, and succeeds with them. The test fails with a CRT assert on wxMSW in a debug build (due to array bounds error) in the intermediate state where the wxAuiManager change is present but the wxEvtHandler change is not.

However, there are two questions I have about this test:

I have made no changes at all to the DocView test so far, but the DocViewAui test is largely a copy of the DocView one with just the code for creating windows changed. Should I modify the tests to share code, or keep them independent?
I have avoided force-pushing anything in this branch. However, I think it makes sense to make the test the first commit in the branch so that it can be seen to fail before the fixes are applied, and succeed after the fixes are applied. What do you think?

AliKet · 2025-05-01T04:49:30Z

FYI, the DocView test was recently changed (see b4d80a7) after fixing MDI issues in wxQt. So in my opinion, it's better to rebase this PR on master and try to refactor common code if possible.

tests/events/propagation.cpp

vadz · 2025-05-01T14:14:41Z

I have successfully created a DocViewAui test

Great, thanks!

However, there are two questions I have about this test:

1. I have made no changes at all to the `DocView` test so far, but the `DocViewAui` test is largely a copy of the `DocView` one with just the code for creating windows changed.  Should I modify the tests to share code, or keep them independent?

I am not sure if sharing the code is not going to make things more/too complicated. If I'm wrong, and the merged test remains relatively simple, then, sure, let's merge them. But I'm much less allergic to code duplication in the tests than in the main library, so I'm also fine with leaving this as is.

2. I have avoided force-pushing anything in this branch.  However, I think it makes sense to make the test the first commit in the branch so that it can be seen to fail before the fixes are applied, and succeed after the fixes are applied.  What do you think?

I don't think this is necessary but you can still rebase this branch on master, if only to squash the fixup commits and fix the CircleCI build (which was fixed on master since this PR was opened).

Thanks!

wsu-cb · 2025-05-03T22:37:40Z

FYI, the DocView test was recently changed (see b4d80a7) after fixing MDI issues in wxQt. So in my opinion, it's better to rebase this PR on master and try to refactor common code if possible.

Done. See 12a20d7

wsu-cb · 2025-05-03T22:52:26Z

I am not sure if sharing the code is not going to make things more/too complicated. If I'm wrong, and the merged test remains relatively simple, then, sure, let's merge them. But I'm much less allergic to code duplication in the tests than in the main library, so I'm also fine with leaving this as is.
...
I don't think this is necessary but you can still rebase this branch on master, if only to squash the fixup commits and fix the CircleCI build (which was fixed on master since this PR was opened).

I have rebased on master, and also added a new commit b303e22 that merges the tests. I will leave it to you to decide whether this is "relatively simple". If it is not, then feel free to drop that commit.

AliKet · 2025-05-04T15:23:28Z

Please apply this patch:

diff --git a/src/qt/frame.cpp b/src/qt/frame.cpp
index 7f670a6045..6f5ae97d71 100644
--- a/src/qt/frame.cpp
+++ b/src/qt/frame.cpp
@@ -226,7 +227,10 @@ void wxFrame::RemoveChild( wxWindowBase *child )
 // excluding any menubar and toolbar if any.
 wxPoint wxFrame::GetClientAreaOrigin() const
 {
-    return wxQtConvertPoint( GetQMainWindow()->centralWidget()->pos() );
+    if ( GetQMainWindow() )
+        return wxQtConvertPoint( GetQMainWindow()->centralWidget()->pos() );
+
+    return wxPoint();
 }
 
 QMainWindow *wxFrame::GetQMainWindow() const

which should fix the crash and should have been part of #25332

Edit:
Although the proposed patch above should fix the problem, I don't understand why we should do that! I need to debug this further...

AliKet · 2025-05-04T15:25:25Z

See c07c636

wsu-cb · 2025-05-04T21:32:49Z

Please apply this patch:

It looks like the patch fixes the problem on Ubuntu, but not Windows.

Edit:
From the WARN() logging I have temporarily added to the test, it looks like child->Activate() crashes when the child is wxDocMDIChildFrame.

AliKet · 2025-05-05T20:17:42Z

Please apply this patch:

It looks like the patch fixes the problem on Ubuntu, but not Windows.

Edit: From the WARN() logging I have temporarily added to the test, it looks like child->Activate() crashes when the child is wxDocMDIChildFrame.

#25368 should fix the crashes seen here.

BTW 495caaf is not needed.

and also whether Unbind() causes array bound errors

Implement recommendation in wxWidgets#25219 (comment) Fix wxWidgets#25219

SearchDynamicEventTable() tries to handle an event handler that calls Unbind(), but previously failed to account for SearchDynamicEventTable() being recursive when an event handler processes a new event. Use wxSharedPtr<> to ensure life of wxRecursionGuardFlag extends beyond wxRecursionGuard. This enables all recursive call to SearchDynamicEventTable() to prune empty elements from m_dynamicEvents, not just outermost recursive call. more detailed explanation of need for wxSharedPtr<> Co-authored-by: VZ <[email protected]>

wsu-cb · 2025-05-06T04:12:43Z

#25368 should fix the crashes seen here.

Yes, that fixed it. All the checks passed!

BTW 495caaf is not needed.

OK.

wsu-cb · 2025-05-07T04:09:28Z

I don't understand why the commit comment for cb59906 hasn't caused this PR to mark itself as closing #25219.

wxEvtHandler in master doesn't need to be ABI-compatible with 3.2, so don't bother checking that it is and remove the static asserts added in c8ef488 (SearchDynamicEventTable(): handle recursion, 2025-04-16) See wxWidgets#25336.

wsu-cb marked this pull request as ready for review April 17, 2025 11:52

vadz requested changes Apr 17, 2025

View reviewed changes

src/common/event.cpp Outdated Show resolved Hide resolved

include/wx/aui/framemanager.h Outdated Show resolved Hide resolved

vadz approved these changes Apr 29, 2025

View reviewed changes

include/wx/event.h Show resolved Hide resolved

src/common/event.cpp Outdated Show resolved Hide resolved

src/common/event.cpp Outdated Show resolved Hide resolved

src/common/event.cpp Outdated Show resolved Hide resolved

wsu-cb marked this pull request as draft April 30, 2025 12:59

vadz reviewed May 1, 2025

View reviewed changes

tests/events/propagation.cpp Outdated Show resolved Hide resolved

vadz reviewed May 1, 2025

View reviewed changes

tests/events/propagation.cpp Outdated Show resolved Hide resolved

wsu-cb force-pushed the auimgr-docview-fix branch from 3471668 to 447d3cf Compare May 3, 2025 22:34

wsu-cb force-pushed the auimgr-docview-fix branch 2 times, most recently from 881fa62 to e774291 Compare May 4, 2025 04:05

wsu-cb force-pushed the auimgr-docview-fix branch from 1b98f91 to 0dd9b4b Compare May 4, 2025 18:58

wsu-cb and others added 4 commits May 5, 2025 20:39

add DocViewAui test that checks event propagation with wxAuiManager

600c939

and also whether Unbind() causes array bound errors

reduce DocView()/DocViewAui() code duplication

6235357

wxAuiManager: use Bind() instead of PushEventHandler()

cb59906

Implement recommendation in wxWidgets#25219 (comment) Fix wxWidgets#25219

wsu-cb force-pushed the auimgr-docview-fix branch 2 times, most recently from b3d10ef to c8ef488 Compare May 6, 2025 04:12

wsu-cb marked this pull request as ready for review May 6, 2025 11:47

vadz merged commit 5a51e52 into wxWidgets:master May 7, 2025
41 checks passed

wsu-cb mentioned this pull request May 14, 2025

Auimgr docview fix backport #25378

Closed

wsu-cb deleted the auimgr-docview-fix branch June 1, 2025 23:49

fix wxAuiManager interfering with docview event propagation #25336

fix wxAuiManager interfering with docview event propagation #25336

Uh oh!

Conversation

wsu-cb commented Apr 17, 2025

Uh oh!

wsu-cb commented Apr 17, 2025

Uh oh!

vadz commented Apr 17, 2025

Uh oh!

vadz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vadz commented Apr 27, 2025

Uh oh!

wsu-cb commented Apr 28, 2025

Uh oh!

vadz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wsu-cb commented Apr 30, 2025

Uh oh!

wsu-cb commented May 1, 2025

Uh oh!

AliKet commented May 1, 2025

Uh oh!

Uh oh!

Uh oh!

vadz commented May 1, 2025

Uh oh!

wsu-cb commented May 3, 2025

Uh oh!

wsu-cb commented May 3, 2025

Uh oh!

AliKet commented May 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliKet commented May 4, 2025

Uh oh!

wsu-cb commented May 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliKet commented May 5, 2025

Uh oh!

wsu-cb commented May 6, 2025

Uh oh!

wsu-cb commented May 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AliKet commented May 4, 2025 •

edited

Loading

wsu-cb commented May 4, 2025 •

edited

Loading