-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Help tool for Wx backends #11081
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
Help tool for Wx backends #11081
Conversation
@@ -1826,9 +1826,49 @@ def remove_rubberband(self, dc=None): | |||
self._rect = None | |||
|
|||
|
|||
class _TableDialog(wx.Dialog): | |||
def __init__(self, parent, help, title="Help"): |
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.
help
is built-in function. While technically you can use that variable name, I would not advise to do so. Also, it's slightly imprecise. Maybe rename to help_entries
.
|
||
class HelpWx(backend_tools.ToolHelpBase): | ||
def trigger(self, *args): | ||
help = [("Action","Shortcuts", "Description")] |
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.
help_entries
?
Also a missing space.
lib/matplotlib/backend_tools.py
Outdated
return ("<style>td {padding: 0px 4px}</style>" | ||
"<table><thead>" + rows[0] + "</thead>" | ||
"<tbody>".join(rows[1:]) + "</tbody></table>") | ||
|
||
|
||
|
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.
Remove. PEP8 will complain about too many empty lines.
Thanks, I probably should install a PEP 8 checker...
|
OK.Bind(wx.EVT_BUTTON, self.OnClose) | ||
|
||
def OnClose(self, evt): | ||
HelpWx.dlg = None |
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'll reset the class variable, but in HelpWx you're setting the instance variable.
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.
Oh, yes, you're right.
I don't want to pass and store a reference to the HelpWx instance in the dialog.
On the other hand, self.__class__.dlg = _HelpDialog(...)
is not too nice, so a reference might be better.
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.
Ah, HelpWx.dlg = _HelpDialog(...)
would be an option.
This plus a comment would probably be the the best solution.
Don't know how these things are usually handled in wx. It's not particularly nice, but ok for me. Maybe add a short comment that makes this dependency handling clear. |
Shouldn't the dialog just be made modal, and this way you don't have to handle lifetime problems? (it is modal in gtk3 and qt5, not in tk right now) |
That would have been the easiest option, but that's not nice from a user perspective. |
Does wx require you to keep an explicit reference to the dialog to avoid it getting garbage collected? |
wx itself does not need the reference, but for re-use always a reference is needed. I'm not sure whether a closed dialog would be collected if If the help dialog typically was used again and again during a session, then I wouldn't destroy it, but I think that's not the typical use case for most users. Also, in theory, tools could be added and so destroy/re-created is probably the best here. Things should now be clearer with the latest commit. |
Inline comments must have the # preceded by two spaces. |
The two spaces are there, so the checker should not complain. At first, I did not re-use, but then I sometimes ended with multiple dialogs open. |
You could make the dialog sort of a singleton: Store the reference in the Dialog class and use a class method to instantiate the dialog. That way you can keep an internal reference for reuse. |
As @DietmarSchwertberger says we're very close to overengineering now :) |
The current solution would work in that case, if the dialog was closed inbetween. Update on-the-fly would definitively be overengineering. Usually, on F1 I'm just launching a browswer with the HTML documentation. For other things I'm using modal dialogs or notebook tabs if anything should be displayed permanently. |
Oh I didn't mean update on the fly, just update after close/open indeed. |
I like the singleton plus classmethod. |
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.
Conditional on rebasing and merging #10851 first.
OK.Bind(wx.EVT_BUTTON, self.OnClose) | ||
|
||
def OnClose(self, evt): | ||
_HelpDialog.instance = None # remove global reference |
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.
Shouldn't this be _HelpDialog._instance?
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.
Oh, yes. You're right. Thanks.
(Worked anyway due to the bool value of a destroyed window, but kept an unneccesary Python reference.)
Thanks @DietmarSchwertberger , sorry these sat so long. |
PR Summary
wx implementation of the ToolManager Help tool (see #11045)
Re-factored part of
ToolHelpBase._get_help_text
and_get_help_html
into_get_help_entries
as this implementation needs a list of entries.PR Checklist