Help tool for Wx backends#11081
Conversation
|
|
||
|
|
||
| class _TableDialog(wx.Dialog): | ||
| def __init__(self, parent, help, title="Help"): |
There was a problem hiding this comment.
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.
help_entries?
Also a missing space.
| "<tbody>".join(rows[1:]) + "</tbody></table>") | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
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.
That'll reset the class variable, but in HelpWx you're setting the instance variable.
There was a problem hiding this comment.
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.
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. |
| OK.Bind(wx.EVT_BUTTON, self.OnClose) | ||
|
|
||
| def OnClose(self, evt): | ||
| _HelpDialog.instance = None # remove global reference |
There was a problem hiding this comment.
Shouldn't this be _HelpDialog._instance?
There was a problem hiding this comment.
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_textand_get_help_htmlinto_get_help_entriesas this implementation needs a list of entries.PR Checklist