Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

DietmarSchwertberger
Copy link
Contributor

PR Summary

wx implementation of the ToolManager Help tool (see#11045)

Re-factored part ofToolHelpBase._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

@tacaswelltacaswell added this to thev3.0 milestoneApr 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 tohelp_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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

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

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
ContributorAuthor

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

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
ContributorAuthor

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 ifDestroyLater 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

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
ContributorAuthor

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 commentedApr 19, 2018
edited
Loading

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

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
ContributorAuthor

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

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

@DietmarSchwertberger
Copy link
ContributorAuthor

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

@anntzeranntzer 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
ContributorAuthor

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

@tacaswelltacaswell merged commitec14df9 intomatplotlib:masterJul 9, 2018
@tacaswell
Copy link
Member

Thanks@DietmarSchwertberger , sorry these sat so long.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@timhoffmtimhoffmtimhoffm left review comments

@maddox32maddox32maddox32 left review comments

@anntzeranntzeranntzer approved these changes

Assignees
No one assigned
Labels
GUI: wxMEP: MEP22tool manager
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.

5 participants
@DietmarSchwertberger@timhoffm@anntzer@tacaswell@maddox32

[8]ページ先頭

©2009-2025 Movatter.jp