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

Widget context menus #1020

Merged
merged 4 commits into from
Sep 20, 2021
Merged

Widget context menus #1020

merged 4 commits into from
Sep 20, 2021

Conversation

corranwebster
Copy link
Contributor

This is doing essentially the same as #1019 but for context menus, so that now any widget can have a context menu attached to it.

One consequence of this is that Field and the testing code for field have become very thin with no toolkit-specific code at all. This PR is leaving them in that state, but I will open an issue to either do a clean-up or add some meat to them.

This is a straightforward move of code from Fields to Widgets, which is
possible now the _add_event_notifiers, etc. has been standardized,
@rahulporuri
Copy link
Contributor

@corranwebster CI is failing on this PR as well on the wx toolkit

@corranwebster
Copy link
Contributor Author

corranwebster commented Sep 9, 2021

Looks like a genuine conflict in the DataView code. Nope, it's a bad test that's only exposed because DataView uses HasStrictTraits.

@rahulporuri rahulporuri self-requested a review September 17, 2021 08:32
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 few comments

@@ -35,6 +35,9 @@ class IWidget(Interface):
#: A tooltip for the widget.
tooltip = Str()

#: An optional context menu for the widget.
context_menu = Instance("pyface.action.menu_manager.MenuManager")
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the show_context_menu method that is defined on the IField interface? Update : It looks like an undefined/unused part of the IField interface which we're choosing not to carry over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I realized it wasn't hooked up to anything, so no reason to carry it over.

Comment on lines -43 to -46
wx.EVT_CONTEXT_MENU, handler=self._handle_context_menu
)
else:
self.control.Bind(wx.EVT_CONTEXT_MENU, self._handle_context_menu)
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like there was a bug here which we coincidentally fixed

@corranwebster corranwebster merged commit 47191e1 into main Sep 20, 2021
@corranwebster corranwebster deleted the enh/widget-context-menus branch September 20, 2021 13:18
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