Skip to content

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

Merged
merged 8 commits into from
Jul 9, 2018

Conversation

DietmarSchwertberger
Copy link
Contributor

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.

wxhelptool

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@tacaswell tacaswell added this to the v3.0 milestone Apr 18, 2018
@@ -1826,9 +1826,49 @@ def remove_rubberband(self, dc=None):
self._rect = None


class _TableDialog(wx.Dialog):
def __init__(self, parent, help, title="Help"):
Copy link
Member

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")]
Copy link
Member

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.

return ("<style>td {padding: 0px 4px}</style>"
"<table><thead>" + rows[0] + "</thead>"
"<tbody>".join(rows[1:]) + "</tbody></table>")



Copy link
Member

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.

@DietmarSchwertberger
Copy link
Contributor Author

Thanks, I probably should install a PEP 8 checker...
Yes, help_entries is much better.
I have modified the code to avoid multiple dialogs being created shown.
I hope it is OK, to keep a reference to the dialog as class variable and reset it from the dialog when it's closed and destroyed:

class _HelpDialog(wx.Dialog):
    def OnClose(self, evt):
        HelpWx.dlg = None
        self.DestroyLater()
        ...

class HelpWx(backend_tools.ToolHelpBase):
    dlg = None

OK.Bind(wx.EVT_BUTTON, self.OnClose)

def OnClose(self, evt):
HelpWx.dlg = None
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@timhoffm
Copy link
Member

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.

@anntzer
Copy link
Contributor

anntzer commented Apr 19, 2018

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)

@DietmarSchwertberger
Copy link
Contributor Author

That would have been the easiest option, but that's not nice from a user perspective.
With this implementation the user can keep it open as long as he wants or just close it with Enter/ESC or the mouse.

@anntzer
Copy link
Contributor

anntzer commented Apr 19, 2018

Does wx require you to keep an explicit reference to the dialog to avoid it getting garbage collected?
If that's the case, my solution (for qt, but the idea is the same) is to just have a global _keep_alive set and stash everything there (as there's typically no point of reimplementing that again and again).

@DietmarSchwertberger
Copy link
Contributor Author

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 DestroyLater is not called. The docs are not clear here except that explicit destruction is always recommended if a dialog is not used any more. For modal dialogs, things are easier and better documented (there, a context manager can be used).

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.
Let's see whether the PEP 8 checker accepts the inline comments.

@anntzer
Copy link
Contributor

anntzer commented Apr 19, 2018

Inline comments must have the # preceded by two spaces.
Why do you want to reuse the dialog? It seems cheap enough to just recreate one if needed (as you mention yourself that's unlikely anyways).
If we're OK recreating a dialog everytime and wx doesn't need a reference, then I'll just not store it anywhere?
(Again, I don't know anything about wx best practices, just trying to map my qt knowledge to that...)

@DietmarSchwertberger
Copy link
Contributor Author

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.
It`s almost over-engineering...

@timhoffm
Copy link
Member

timhoffm commented Apr 19, 2018

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.

@anntzer
Copy link
Contributor

anntzer commented Apr 19, 2018

As @DietmarSchwertberger says we're very close to overengineering now :)
Would prefer a solution that works even if tools get added or removed (not necessarily common, but given that it shouldn't be hard to implement that...).

@DietmarSchwertberger
Copy link
Contributor Author

The current solution would work in that case, if the dialog was closed inbetween. Update on-the-fly would definitively be overengineering.
I will check for the singleton solution and come back in an hour or so. If it's more readable, I will commit it.

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.
So that's actually my first floating non-modal dialog after having used wxPython for 18 years now. So sorry for the confusion...

@anntzer
Copy link
Contributor

anntzer commented Apr 19, 2018

Oh I didn't mean update on the fly, just update after close/open indeed.

@DietmarSchwertberger
Copy link
Contributor Author

I like the singleton plus classmethod.
Updates on the fly would now be easy, as _get_help_entries() is called anyway, but without it's more readable.

Copy link
Contributor

@anntzer anntzer left a 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

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?

Copy link
Contributor Author

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.)

@tacaswell tacaswell merged commit ec14df9 into matplotlib:master Jul 9, 2018
@tacaswell
Copy link
Member

Thanks @DietmarSchwertberger , sorry these sat so long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants