-
Notifications
You must be signed in to change notification settings - Fork 95
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
Enable the use of Schemas in Views #1827
Conversation
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
just a few comments / questions
def setUp(self): | ||
BaseTestMixin.setUp(self) | ||
|
||
def tearDown(self): | ||
BaseTestMixin.tearDown(self) |
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.
why are these needed?
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.
Probably not needed - I think I added them because I anticipated needing other mixins, but then didn't.
# instead | ||
|
||
qt_trigger_menu_action = partial( | ||
_qt_trigger_action, pyface.ui.qt4.action.menu_manager._Menu |
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.
doesn't need to change in this PR but I find some of this test code confusing as it calls both toolbars/menubars toolbar.
Eg in _qt_trigger_action
, toolbar = ui.control.findChild(container_class)
where here container_class
would be pyface.ui.qt4.action.menu_manager._Menu
.
|
||
self._test_actions(_wx_trigger_toolbar_action) | ||
|
||
# TODO: I couldn't find a way to press menu items programmatically for wx |
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.
I tested manually and it worked as expected
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.
I had to add DialogWithSchema().configure_traits()
in the if __name__ == "__main__"
below. Maybe it is worth adding this for more explicit manual testing since there isn't a unittest?
def _wx_trigger_toolbar_action(ui): | ||
# long road to get at the Id of the toolbar button | ||
toolbar_control = ui.control.GetToolBar() | ||
toolbar_item = toolbar_control.tool_bar_manager.groups[0].items[0] | ||
toolbar_item_wrapper = toolbar_item._wrappers[0] | ||
control_id = toolbar_item_wrapper.control_id | ||
|
||
# build event that clicks the button | ||
click_event = wx.CommandEvent( | ||
wx.wxEVT_COMMAND_TOOL_CLICKED, control_id | ||
) | ||
|
||
# send the event to the toolbar | ||
toolbar_control.ProcessEvent(click_event) |
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.
given the duplication maybe this can be pulled out to sit next to _qt_trigger_action
?
traitsui/tests/test_actions.py
Outdated
not is_mac_os, | ||
"Problem with triggering toolbar actions on Linux and Windows. Issue #428.", # noqa: E501 | ||
) | ||
@unittest.skip("Problem with triggering toolbar actions. Issue #428.") |
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.
why did we remove macOS testing here?
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.
The issue that we were seeing in #428 started manifesting on OS X as well. I'll update that as well.
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.
Digging into this a little to see what is going on, and looking at the UI generated, there is a toolbar, but no toolbar buttons show.
# ----- wx tests | ||
|
||
@unittest.skipIf( | ||
not is_mac_os, |
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.
Just to be explicit - so for wx on mac, using schemas will work, but the old way will fail
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.
That appears to be the case (my suspicion was that the old way had been broken for some time) - but let me just double check that I didn't accidentally break the old way somehow.
Co-authored-by: aaronayres35 <36972686+aaronayres35@users.noreply.github.com>
Digging into what was going on with toolbars in Wx ended up with some weird issues where the buttons aren't displaying. This is likely related to the tests failing. A days worth of digging hasn't resolved, or even indicated what the problem might be. I have opened #1843 and I think at this point the plan is:
|
Looked at his a bit more: the issue with Wx toolbars in #1843 is independent of this PR. |
This allows users to use the Pyface
Schema
system instead of usingMenuBarManager
,ToolBarManager
and similar objects directly. This permits declarative use of these without needing to have a toolkit available. Additionally, there are hooks on theActionManagerBuilder
to permit extensions, should they be desired.This is experimental. There is perhaps an argument that the
ActionManagerBuilder
shouldn't be directly exposed, but instead a factory function expected, as perTasksActionManagerBuilder
, potentially permitting other controllers.Fixes #1826.
Checklist