Opened 8 years ago

Closed 4 weeks ago

#14145 closed optimization (fixed)

wxAuiManager should take care about its own destruction

Reported by: Hanmac Owned by: Vadim Zeitlin <vadim@…>
Priority: low Milestone:
Component: wxAui Version: stable-latest
Keywords: Cc: bwilliams, atomic.alarm@…
Blocked By: Blocking:
Patch: yes

Description

i have maked an patch for AuiManager so it can detect the destruction of the Window with the EvtDestroy.

Attachments (1)

auimanager.patch download (1.7 KB) - added by Hanmac 8 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 8 years ago by vadz

What happens to the existing code calling UnInit() from the frame dtor? Wouldn't it result in asserts from RemoveEventHandler() now?

Also, why compare the frame IDs instead of comparing event.GetEventObject() with m_frame, this seems to be less error-prone.

Changed 8 years ago by Hanmac

comment:2 Changed 8 years ago by Hanmac

i changed the patch file with your suggestions

comment:3 Changed 8 years ago by vadz

  • Cc bwilliams added
  • Status changed from new to confirmed
  • Summary changed from AUIManager should take care about its own destruction to wxAuiManager should take care about its own destruction

Thanks!

Does anybody have a problem with doing this? I don't have much experience with wxAUI but doing it like this looks logical to me, why should the frame need UnInit() manually when it can be done automatically (I'd remove the code doing this from the sample too BTW if this patch is applied)?

Any objections?

comment:4 Changed 8 years ago by biwillia76

Patch looks good.

comment:5 Changed 8 years ago by rk

No objections here. But we could remove the UnInit() method from the public api now or at least deprecate it and mention in the documentation that it is no longer necessary to call it.

comment:6 Changed 8 years ago by mmacleod

Can't see any reason not to apply this.

However I disagree with UnInit() being either deprecated or removed from the public API, other then the fact that this forces people to update code immediately (where they don't really need too)
I also don't think it is a 100% given that there is no use case still out there where someone might need/want to UnInit() manually (I can't think of one currently but that does not mean one does not exist), this function is hardly a maintenance burden or anything like that (as it is needed internally anyway) so I simple don't see what the positive of doing this would be.

comment:7 Changed 4 weeks ago by GreenDragon

  • Cc atomic.alarm@… added

Can it be applied now?

comment:8 Changed 4 weeks ago by vadz

Unfortunately not in its current state because testing it shows that the patch doesn't work: if you apply it and remove UnInit() call from MyFrame dtor in the aui sample, the sample crashes on exit.

The reason is that wxAuiManager is destroyed in MyFrame dtor before wxEVT_DESTROY is generated in wxFrame dtor, so this event is dispatched to the already destroyed event handler.

But this looks simple to fix, i.e. I think we just need to also (and even most importantly) call UnInit() from the manager dtor. So I've created PR 1982 with this change, please test it and let me know if you see any problems with it. TIA!

comment:9 Changed 4 weeks ago by Vadim Zeitlin <vadim@…>

  • Owner set to Vadim Zeitlin <vadim@…>
  • Resolution set to fixed
  • Status changed from confirmed to closed

In 4dd009136/git-wxWidgets:

Merge branch 'aui-auto-uninit'

Call wxAuiManager::UnInit() automatically.

Closes #14145.

Note: See TracTickets for help on using tickets.