Help tool. #11045

anntzer · 2018-04-13T09:35:00Z

PR Summary

Attn @fariza (@timhoffm perhaps as well).

This is an implementation of the help tool using native widgets. Note that only Qt can natively display tables (via html markup), for other backends I simply used

toolname: keys
    description

With that change, it is in fact shorter to provide 3 different native widget implementations (wx isn't there but I'm sure it would be just as easy to add once the general tool framework is merged) and add code to both generate plain text and html versions of the help text, than it is to set up the Table in #9022.

The relevant screenshots:

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

DietmarSchwertberger · 2018-04-13T19:31:40Z

I could take care for a wx implementation when this one is merged.
What is the relation to #9022?
Use the GUI implementation when embedded and the canvas implementation as fallback and for standalone windows?

anntzer · 2018-04-13T20:20:26Z

I would just always use the GUI implementation...

DietmarSchwertberger · 2018-04-13T21:11:24Z

Btw. there's something that I don't like with either implementation:
Usually keymaps are documented with upper case or mixed case like "F1", "Ctrl+W"

(Edit: point about Caps-Lock vs. Shift deleted; I got a wrong impression as my plot started with a full grid)

ImportanceOfBeingErnest · 2018-04-14T18:02:13Z

Would it make sense to provide a general purpose popup-window as suggested [here]:(#9022 (comment))

Maybe there should be a general pupose popup child window in the toolmanager [...]
This could then be used by the helper tool, but equally by all any other tool that needs some window.

Also, could the help tool display the currently used matplotlib version? And also the backend in use (if applicable)?

fariza · 2018-04-14T19:47:24Z

If this is the direction that we adopt I prefer the general purpose pop-up window. In MEP27 all that is already implemented https://matplotlib.org/devel/MEP/MEP27.html If we don't want for one reason or another go with MEP27 right now. We can always take some pieces

…

On Sat, Apr 14, 2018, 2:02 PM Importance of Being Ernest, < ***@***.***> wrote: Would it make sense to provide a general purpose popup-window as suggested [here]:(#9022 (comment) <#9022 (comment)> ) Maybe there should be a general pupose popup child window in the toolmanager [...] This could then be used by the helper tool, but equally by all any other tool that needs some window. Also, could the help tool display the currently used matplotlib version? And also the backend in use (if applicable)? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11045 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABa86Qa3KBUwRQDClFZiEC0dKFbcaxlbks5tojm3gaJpZM4TTJGX> .

anntzer · 2018-04-14T21:22:36Z

Would it make sense to provide a general purpose popup-window as suggested [here]:(#9022 (comment))

I believe that no, as the end user you should just pick a GUI backend and learn how to pop up a window with it. There's no point in implementing and exposing a full compatibility layer between qt/gtk/wx/tk (well perhaps there is, but that's not Matplotlib), only the parts that we "reasonably" need (but that doesn't mean Matplotlib shouldn't be using native widgets when necessary).

fariza · 2018-04-14T22:23:15Z

In this specific case the general pourpose pop-up is not for the user easy of use. It is for us, we have to support several backends. If we think that that belongs to the user, then we should not add this tool. I agree with you that we shouldn't need to provide full wrapper around qt,gtk,tk,etc... but we offer the backends and we add tools to them. So we are shooting ourselves in the foot. One very drastic idea that sometimes I fantasize with, is to get rid off all the backends and just keep one and make the best set of tools ever. But until we do that. We have to push for uniformity across what we have committed to support

…

On Sat, Apr 14, 2018, 5:22 PM Antony Lee, ***@***.***> wrote: Would it make sense to provide a general purpose popup-window as suggested [here]:(#9022 <#9022> (comment)) I believe that no, as the end user you should just pick a GUI backend and learn how to pop up a window with it. There's no point in implementing and exposing a full compatibility layer between qt/gtk/wx/tk (well perhaps there is, but that's not Matplotlib), only the parts that we "reasonably" need (but that doesn't mean Matplotlib shouldn't be using native widgets when necessary). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#11045 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABa86emrqpKhuNk0ZY8u0FXDDeUJnoeQks5tomisgaJpZM4TTJGX> .

anntzer · 2018-04-14T22:29:58Z

If the thing is for our own internal use (as I think it should), then we don't need to overengineer it. By the day we'll want to have two tools that both pop up a dialog message (if that ever happens), we can refactor out that relevant part (and I would prefer a subclassing API rather than a tool-based one).

QuLogic · 2018-04-15T04:53:26Z

On GTK 3.20, there's a (rather poorly documented) GtkShortcutsWindow that can be used. Unfortunately, there's a bit of an impedance mismatch in shortcut descriptions, so it requires a bit of adjustment to work, but it ends up looking like this:

It would be nice if we had some way of grouping things...

anntzer · 2018-04-15T05:16:18Z

That actually looks quite nice... (though not very space efficient).

QuLogic · 2018-04-15T06:01:04Z

I tried grouping things using the default_toolbar_tools, using GTK's groups, which looks a bit better for widescreens:

Or GTK has a concept of sections, where each one is a separate 'page', like this:

etc. But we probably need better groups.

ImportanceOfBeingErnest · 2018-04-15T09:03:44Z

the end user you should just pick a GUI backend and learn how to pop up a window with it

That's fine for the end user himself, although cumbersome. But what about the user that writes the program for the end user? What's great about the toolmanager approach is that it is really easy to add a some custom button in backend independent way. That leaves the end user the complete freedom of matplotlib.useing the backend of choice. In the case of the popup window, this would allow

class MyPopUp(ToolPopUp):
    text = """ My text to show """

fig.canvas.manager.toolmanager.add_tool('MyPopUp', MyPopUp)
fig.canvas.manager.toolbar.add_tool('MyPopUp', 'navigation', -1)

anntzer · 2018-04-15T10:37:55Z

That leaves the end user the complete freedom of matplotlib.useing the backend of choice.

Why exactly do you want that? At some point someone has to decide what toolkit to use. If I am writing a GUI app then the onus is on me to support all the toolkits I want to support.

Obviously matplotlib has cross-backend support, but I think a line can reasonably drawn at "cross-backend support for plotting-related activities", and popping an arbitrary message window doesn't qualify. (Otherwise, where do you draw the line? Do we support OK/Cancel buttons with callbacks? Arbitrary input dialogs? etc.)

Even if we were to go in this direction, I think the tool abstraction would be a pretty poor one for the action of popping up message dialogs.

ImportanceOfBeingErnest · 2018-04-15T11:11:05Z

Sure, it's not matplotlib's task to provide GUI wrappers.
My point is just that if matplotlib wraps certain things anyways, why not expose them to the user?

Anectotical reason for providing a pop up (hidden not to distract from the PR features)

Why exactly do you want that?

I have this program that me and collegues used to create reports about certain measurements. It would create a matplotlib figure and a text from some internally standarized data from a certain measurement device. The text would include some meta information like date/time of measurement, its duration, device's bandwidth setting etc.
Because the program's main aim is to create reports, it's not worth creating a complete GUI for it and I couldn't support all different GUIs that people have available anyways.
But people liked the fact that you may also show the figure interactively for zooming etc. (Essentially this just calls plt.show() at some point.) However you then don't have the meta information from the report text readily available. So what I did was to attach a callback such that when the mouse moves in the upper right corner of the figure, a matplotlib.widgets.Button appears that, if pressed, prints this meta information in the console. (This was necessary, not to have the button appear in the saved version of the figure.)
If writing this program again with matplotlib's toolmanager available, I would create a toolbutton to do this. But the much better solution would sure be to have this button showing a popup window with this text.

anntzer · 2018-04-15T11:22:48Z

why not expose them to the user?

Because that becomes public API with bug reports, feature requests, deprecation periods and all that, which we do not want to handle (or rather speaking for myself: which I do not think anyone here should be wasting their time handling).

If writing this program again with matplotlib's toolmanager available, I would create a toolbutton to do this. But the much better solution would sure be to have this button showing a popup window with this text.

Then make your program call matplotlib.use("tkagg") at the very beginning and learn how to use tkinter.simpledialog. Or, if you don't like tk, declare a dependency on {qt,gtk3,wx} (they are all pip/conda-installable nowadays, so if you somehow got your end user to install matplotlib you can likewise get it to install e.g. pyqt5), call use("qt5agg")/... instead, and learn how to pop up a message dialog with that toolkit (feel free to steal the implementations from this PR). That's a much more useful/reusable skill than popping up dialogs with a half-assed API provided by Matplotlib anyways...

timhoffm

Coming back to topic: I support @DietmarSchwertberger in the comment that printed shortcuts should be the standard mixed-case notation (F1, Ctrl+W, Home) for special keys and composed shortcuts. Single character shortcuts (l, L) are case sensitive and must be written as they are. Can we just change the notation in the rc config or would we need a converter to preprocess them for displaying?

timhoffm · 2018-04-16T09:51:29Z

lib/matplotlib/backend_tools.py

+
+    def _get_help_html(self):
+        fmt = "<tr><td>{}</td><td>{}</td><td>{}</td></tr>"
+        rows = [fmt.format("NAME", "KEYS", "DESCRIPTION")]


I'd use bold instead of uppercase, e.g. "<b>Name</b>" instead of "NAME"

Agreed that it looks better. Changed accordingly.

anntzer · 2018-04-16T16:32:40Z

We need to check whether making them Ctrl-D in the rc would only case them to trigger on Ctrl+Shift+D or not... See also http://doc.qt.io/qt-5/qkeysequence.html#keyboard-layout-issues for fun issues :)

timhoffm · 2018-04-16T23:15:22Z

Just checked with Qt5: The values in the rc are case sensitive: "home" works, "Home" does not.

Which leaves us with formatting the displayed strings.

Additional note: Not sure if this is a Qt issue or if this is within the matplotlib backend code (there's something going on with SPECIAL_KEYS, which may break with capitalization. Anyway, the formatting solution is just adding 4 lines of code, which is simple enough to not justify further research in the capitalization of rc shortcut settings.

timhoffm · 2018-04-16T23:19:23Z

lib/matplotlib/backend_tools.py

+            entries.append(
+                "{}: {}\n\t{}".format(
+                    name,
+                    ", ".join(self.toolmanager.get_tool_keymap(name)),


For proper capitalization

", ".join(format_shortcut(key) for key in self.toolmanager.get_tool_keymap(name))

with

def format_shortcut(s): def repl(match): return '+'.join(g.capitalize() for g in match.groups() if g is not None) return re.sub(r'(\w\w+)(?:\+(\w+))*', repl, s)

something like re.sub(r"\w+", lambda match: str.capitalize(match.group(0)), "ctrl+a") seems simpler.

But this fails on single characters (a -> A should not be replaced, however ctrl+a -> Ctrl+A should).

timhoffm · 2018-04-16T23:19:59Z

lib/matplotlib/backend_tools.py

+        for name, tool in sorted(self.toolmanager.tools.items()):
+            if not tool.description:
+                continue
+            keys = ", ".join(self.toolmanager.get_tool_keymap(name))


keys = ", ".join(format_shortcut(key) for key in self.toolmanager.get_tool_keymap(name))

timhoffm · 2018-04-16T23:21:56Z

lib/matplotlib/backend_tools.py

+
+    def _get_help_html(self):
+        fmt = "<tr><td>{}</td><td>{}</td><td>{}</td></tr>"
+        rows = [fmt.format("<b>Name</b>", "<b>Keys</b>", "<b>Description</b>")]


I'd use the terms "Action" and "Shortcuts" instead of "Name" and "Keys".

timhoffm · 2018-04-16T23:25:39Z

When testing I had to set rcParams['toolbar'] = 'toolmanager' for the default 'toolbar' the help button does not show. Is this intentional?

anntzer · 2018-04-16T23:28:37Z

yes (the idea is that the classic toolbar is going away... at some point)

timhoffm · 2018-04-16T23:59:41Z

Should we also format the names? name.capitalize().replace('_', ' ')

anntzer · 2018-04-17T00:04:44Z

nah, these are actually entries into a dict

timhoffm · 2018-04-17T12:36:00Z

Uh, oh! I just saw, there is cmd+w as well as cmd+W. So to keep standard shortcut notation, we also have to replace modifier+X with Modifier+Shift+X, where X is a single capital letter.

fariza · 2018-04-17T17:02:13Z

lib/matplotlib/backends/_backend_tk.py

@@ -963,10 +963,19 @@ def destroy(self, *args, **kwargs):
        self.window = None


+class HelpTk(backend_tools.ToolHelpBase):
+    def trigger(self, *args):
+        from tkinter.simpledialog import SimpleDialog


Do the import at the beginning of the file?

timhoffm · 2018-04-17T19:25:16Z

lib/matplotlib/backend_tools.py

+        keymaps = self.toolmanager.get_tool_keymap(name)
+        # Capitalize "ctrl+a" -> "Ctrl+A" but leave "a" as is.
+        return ", ".join(re.sub(r"\w{2,}|(?<=\+)\w",
+                                lambda m: m.group(0).capitalize(),


as mentioned, ctrl+a -> Ctrl+A but also ctrl+A -> Ctrl+Shift+A.

Please push the fix directly.

@anntzer I cannot push to your branch (permission denied). Either we have to resolve the permissions, or you can cherry pick from https://github.com/timhoffm/matplotlib/commits/anntzer/helptool

Dunno why, I ticked the "allow edits from maintainers". In any case, I pulled it (thanks for the test) and further simplified format_shortcut (str.title is handy here...).

QuLogic · 2018-04-18T03:05:46Z

Do you want to add the GTK+ native widgets here? (though I'd like if we had some better grouping available.)

anntzer · 2018-04-18T03:24:20Z

Perhaps (as a stopgap before we can bikeshed how to do goruping) just keep the order in which the tools are inserted (adjusting the order for the iconless ones appropriately) and in two columns?

fariza

I like the way it looks, and works in all the supported backends

QuLogic · 2018-04-18T23:15:49Z

lib/matplotlib/backend_tools.py

+        standard notation for displaying shortcuts, e.g. 'ctrl+a' -> 'Ctrl+A'.
+        """
+        return (key_sequence if len(key_sequence) == 1 else
+                re.sub(r"\+[A-Z]", r"+Shift\g<0>", key_sequence).title())


This doesn't add Shift for single letter shortcuts like 'L' for xscale; is that on purpose?

Good question. I've been implicitly assuming that we keep the case sensitivity for single letters, i.e.

xscale k,L yscale l

Now thinking about it, this is probably what we want to do. Otherwise, we'd have

xscale K, Shift+L yscale L

This could be slightly misleading, because "L" really means only typing the L-Key, which in a text field would result in a lowercase "l". An alternative way out (similar to what the GtkShortcutWindow is doing) would be to enclose the keys.

xscale <K>, <Shift>+<L> yscale <L>

Im +0.3 for sticking with the present solution.
Note: Shortcuts equal to single upper/lowercase letters are rarely used, because they clash with text input. I'm not aware of a standard notation for these cases.

vim just uses lowercase for lowercase and uppercase for uppercase (http://vimdoc.sourceforge.net/htmldoc/editing.html#CTRL-^), which seems just fine to me. ctrl+x are always documented as CTRL-X (UPPERCASE), but there are no shortcuts of the form ctrl+shift+x due to internal constraints.

anntzer added the MEP: MEP22 tool manager label Apr 13, 2018

timhoffm reviewed Apr 16, 2018

View reviewed changes

timhoffm mentioned this pull request Apr 16, 2018

Help tool #9022

Closed

anntzer force-pushed the helptool branch 2 times, most recently from 10c1ecc to f79940d Compare April 16, 2018 17:12

timhoffm requested changes Apr 16, 2018

View reviewed changes

anntzer force-pushed the helptool branch from f79940d to a8e66e5 Compare April 17, 2018 07:29

timhoffm approved these changes Apr 17, 2018

View reviewed changes

fariza reviewed Apr 17, 2018

View reviewed changes

timhoffm requested changes Apr 17, 2018

View reviewed changes

Help tool.

37d29df

anntzer force-pushed the helptool branch from a8e66e5 to 37d29df Compare April 17, 2018 23:10

fariza approved these changes Apr 18, 2018

View reviewed changes

timhoffm and others added 2 commits April 18, 2018 09:26

Fix short formatting for display

afbbdd6

Simpler format_shortcut.

59cb786

anntzer force-pushed the helptool branch from 8176f20 to 59cb786 Compare April 18, 2018 17:00

timhoffm approved these changes Apr 18, 2018

View reviewed changes

timhoffm merged commit dda9144 into matplotlib:master Apr 18, 2018

anntzer deleted the helptool branch April 18, 2018 18:08

DietmarSchwertberger mentioned this pull request Apr 18, 2018

Help tool for Wx backends #11081

Merged

6 tasks

QuLogic reviewed Apr 18, 2018

View reviewed changes

QuLogic mentioned this pull request Apr 19, 2018

Use GtkShortcutsWindow for Help tool. #11085

Merged

6 tasks

QuLogic added this to the v3.0.0 milestone Nov 27, 2018

Help tool. #11045

Help tool. #11045

Conversation

anntzer commented Apr 13, 2018

PR Summary

PR Checklist

DietmarSchwertberger commented Apr 13, 2018

anntzer commented Apr 13, 2018

DietmarSchwertberger commented Apr 13, 2018 • edited Loading

ImportanceOfBeingErnest commented Apr 14, 2018

fariza commented Apr 14, 2018 via email

anntzer commented Apr 14, 2018

fariza commented Apr 14, 2018 via email

anntzer commented Apr 14, 2018

QuLogic commented Apr 15, 2018

anntzer commented Apr 15, 2018 • edited Loading

QuLogic commented Apr 15, 2018 • edited Loading

ImportanceOfBeingErnest commented Apr 15, 2018

anntzer commented Apr 15, 2018

ImportanceOfBeingErnest commented Apr 15, 2018

anntzer commented Apr 15, 2018

timhoffm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Apr 16, 2018

timhoffm commented Apr 16, 2018 • edited Loading

timhoffm Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm commented Apr 16, 2018

anntzer commented Apr 16, 2018

timhoffm commented Apr 16, 2018

anntzer commented Apr 17, 2018

timhoffm commented Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuLogic commented Apr 18, 2018

anntzer commented Apr 18, 2018

fariza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DietmarSchwertberger commented Apr 13, 2018 •

edited

Loading

anntzer commented Apr 15, 2018 •

edited

Loading

QuLogic commented Apr 15, 2018 •

edited

Loading

timhoffm commented Apr 16, 2018 •

edited

Loading

timhoffm Apr 16, 2018 •

edited

Loading

timhoffm commented Apr 17, 2018 •

edited

Loading

timhoffm Apr 18, 2018 •

edited

Loading