Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new event wx.lib.agw.aui.EVT_AUI_PANE_CLOSED #1628

Merged
merged 3 commits into from
Jun 16, 2020

Conversation

topic2k
Copy link
Contributor

@topic2k topic2k commented May 10, 2020

I needed an event when a pane was closed, but the existing wx.lib.agw.aui.EVT_AUI_PANE_CLOSE is fired when a pane is about to be closed and that sometimes led to timing issues.
I added an extra event that is fired when the pane is really closed.
I was thinking about to rename EVT_AUI_PANE_CLOSE to EVT_AUI_PANE_CLOSING (which better fits), but that could break older code. So i just changed the docstring.

Fire an event when a pane has been closed.
Copy link
Member

@RobinD42 RobinD42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor issue and some questions.

e = AuiManagerEvent(wxEVT_AUI_PANE_CLOSED)
e.SetPane(pane_info)
e.SetCanVeto(False)
e.SetId(wxEVT_AUI_PANE_CLOSED)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An Event's id attribute is normally used for the id of the widget sending the event, not the event type. For example, for a EVT_BUTTON event the id is the value of the button's GetId(), not wxEVT_BUTTON. Also, it looks like the other AUI events are not setting the id (not great, but okay) so it's probably better to follow the existing pattern here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see why i set the id: to distingish from other events. But now i know i can use the event type instead.

e.SetPane(pane_info)
e.SetCanVeto(False)
e.SetId(wxEVT_AUI_PANE_CLOSED)
wx.CallAfter(self.ProcessMgrEvent, e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is wx.CallAfter necessary here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, would it make sense to just use the FireEvent method to create and send the event instead of doing it all here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure, but i think i thought to have the closing event be finished before fireing the closed event. Anyway, i switch to FireEvent which in also uses wx.CallLater.

@topic2k
Copy link
Contributor Author

topic2k commented May 21, 2020

@RobinD42 what is preferred: just adding a commit with changes or squashing the commits and force-push?

@RobinD42
Copy link
Member

Just adding the new commits is fine. It makes it easy to see what's changed. If a PR gets messy enough that a squash would be better, then I can do that when merging.

@RobinD42 RobinD42 merged commit 509a827 into wxWidgets:master Jun 16, 2020
@RobinD42
Copy link
Member

This pull request has been mentioned on Discuss wxPython. There might be relevant details there:

https://discuss.wxpython.org/t/wxpython-4-1-1-released/35043/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants