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

Add explicit Pingee connect and disconnect methods #255

Merged
merged 4 commits into from Nov 27, 2020

Conversation

mdickinson
Copy link
Member

This PR adds connect and disconnect methods to the Pingee (small-'i', implicit) interface. In particular, this gives the wx Pingee an opportunity to undo the binding that it made.

Closes #254.

@rahulporuri rahulporuri added this to In Progress in Enthought OSS Q4 2020 Nov 27, 2020
@rahulporuri rahulporuri removed this from In Progress in Enthought OSS Q4 2020 Nov 27, 2020
Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

LGTM with a question/comment. And I don't think there's any relevant docs so we don't need to worry about updating them.

it would be nice to put together dev docs on the topic though, while this is all still fresh in your head

@@ -76,12 +76,18 @@ class Pingee(wx.EvtHandler):

def __init__(self, on_ping):
wx.EvtHandler.__init__(self)
self._on_ping = on_ping
self._on_ping = lambda event: on_ping()
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like an orthogonal change - changing self._on_pingand replacingself._on_ping_eventwithself._on_pingin theself.Bind` call. I'm not sure why this is needed.

Is this just refactoring/making the code cleaner?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, yes, it's mostly orthogonal, except that with this change I can be sure that I'm binding and unbinding the exact same handler. (That would probably have been true with the method lookups too, assuming that wx is working with Python equality rather than identity, but with this change I don't have to make that assumption.) Call it superstition again, but this PR is mostly superstition anyway (which is what makes the changes hard to test).

@mdickinson
Copy link
Member Author

it would be nice to put together dev docs on the topic though, while this is all still fresh in your head

Agreed. I'll open an issue, and point to the existing PR description that contained a dump of the routing stuff.

@mdickinson
Copy link
Member Author

I'll open an issue

Opened #261

@mdickinson mdickinson merged commit df93b11 into master Nov 27, 2020
@mdickinson mdickinson deleted the ref/add-explicit-pingee-connection branch November 27, 2020 11:19
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.

Call Unbind in the wxPython Pingee implementation
3 participants