-
Notifications
You must be signed in to change notification settings - Fork 55
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
Deprecate __init__
methods which create the widget
#993
Conversation
- proper calling of super() - make autocreation optional - populate traits correctly - invoke correctly from ProgressDialog - fix background color issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a couple of comments. I didn't test that the examples worked as expected with the wx toolkit though.
@@ -96,7 +109,7 @@ def __init__(self, parent, **traits): | |||
wx.SystemSettings.GetColour(wx.SYS_COLOUR_3DHIGHLIGHT), 1, wx.SOLID | |||
) | |||
|
|||
return | |||
return self.control |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit-picky comment - in an earlier case, we didn't self.control
in the _create_control
method and instead chose to simply return it instead - whereas in this method, we're setting and returning self.control
. Can i ask if there is a specific reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were some widgets with methods called in _create_control
that expect self.control
to not be populated. These widgets need their create methods to be untangled and have some code moved to _add_event_listeners
and other code moved to post-creation initialization.
For example, in this class, all of the binding methods should really be in _add_event_listenrs
.
Since this PR is all about deprecating auto-creation, I wanted to make the smallest changes to achieve the required results. I'll add an issue about untangling creation and initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue created: #995
self.parent = parent | ||
create = traits.pop("create", True) | ||
|
||
# XXX minimum is ignored - it either should be deprecated or supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have an issue please - to remove it outright for the next major release? I don't think we have the resources to figure out how to support minimum
at the moment - and we can implement that functionality from scratch if we ever need to in the future.
parent=parent, | ||
_max=maximum, | ||
direction=direction, | ||
size=size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like size
is ignored as well - it isn't defined on the IWidget
interface or on the wx Widget
class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue created for this and minimum
: #996
As discussed in #729 this does the discussed deprecation transforms in the remaining Wx widgets that use that approach.
Most of these widgets have not tests, but they are exercised by the examples.