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#9022

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

Closed
fariza wants to merge6 commits intomatplotlib:masterfromfariza:help-tool
Closed

Help tool#9022

fariza wants to merge6 commits intomatplotlib:masterfromfariza:help-tool

Conversation

fariza
Copy link
Member

This PR adds a tool to show the usable tools and it's shortcuts (keymap).

As most people are used to pressF1 to bring up some help menu or informationf1 was selected as shortcut

This tool creates a new axes that expands to the whole figure (to hide everything), and add text with the current tool information, toggling again removes the axes
screenshot from 2017-08-11 18-03-23

Currently there is a problem with alignment.
Does anybody has a better idea on how to generate the text to avoid this misalignment?

@afvincent
Copy link
Contributor

@fariza (Very) Dummy ideas about the text misalignement: could forcing a monospace font be of any help (but yes it is kind of a nuke...) or maybe have a lookax.table (I never used it so I am not sure how relevant it could be)?

@WeatherGod
Copy link
Member

WeatherGod commentedAug 12, 2017 via email

I love the idea, but it does need refinement. May I suggest a question markicon?
On Aug 11, 2017 6:26 PM, "Adrien F. Vincent" ***@***.***> wrote:@fariza <https://github.com/fariza> (Very) Dummy ideas about the text misalignement: could forcing a monospace font be of any help (but yes it is kind of a nuke...) or maybe have a look ax.table <https://matplotlib.org/api/_as_gen/matplotlib.axes.Axes.table.html> (I never used it so I am not sure how relevant it could be)? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9022 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-AVjcmg-DghKZm0aDxA31Yn8dUJwks5sXNT-gaJpZM4O1L_t> .

@fariza
Copy link
MemberAuthor

@WeatherGod question mark in the toolbar? or as shortcut?

@fariza
Copy link
MemberAuthor

Using monospace as suggested (thanks@afvincent ), the alignment problem is gone

Before pressingf1
screenshot from 2017-08-12 14-25-45

After pressingf1
screenshot from 2017-08-12 14-26-10

@fariza
Copy link
MemberAuthor

Here is with the help icon
screenshot from 2017-08-12 14-46-03

@WeatherGod
Copy link
Member

WeatherGod commentedAug 13, 2017 via email

Thanks for the icon.Now, would it make sense to put the keymap column after the name column? Iwould usually look for a tool by name and want to know their shortcut, nottheir full description.
On Aug 12, 2017 2:50 PM, "Federico Ariza" ***@***.***> wrote: Here is with the help icon [image: screenshot from 2017-08-12 14-46-03] <https://user-images.githubusercontent.com/1490153/29243462-9de13a98-7f6d-11e7-8884-5ad9aac89d86.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9022 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-GvmfuYCh-Q3vBjPN8YVnSFTD4k-ks5sXfQOgaJpZM4O1L_t> .

@tacaswelltacaswell added this to the2.1 (next point release) milestoneAug 13, 2017
@story645
Copy link
Member

I think this is a great idea, but I'm a little confused byName(id) and theActive Toggle Tools header. I'm unsure if Name is the same as id and if there's a reason I need to know that, and I wonder if `Active Toggle Tools can be dropped since right underneath it says "Group" and "Active"-and what sort of group membership is that?

@fariza
Copy link
MemberAuthor

@story645 This tool is for the new toolmanager.
The printout is the same from the toolmanager example, that is why it has the active toggle tools, remember toggle tools can belong to a group where they are mutually exclusive (radio button), that is why it shows the current toggled tool for each group.
I will remove that group part as it is not really relevant to the "Help"

@fariza
Copy link
MemberAuthor

Removing the toggle groups and keymap before description
screenshot from 2017-08-15 15-32-12

@anntzer
Copy link
Contributor

What are the "List" and "Show" tools that appear at the top?

@fariza
Copy link
MemberAuthor

@anntzer The screenshot is from the ToolManager example. That is why those two tools are there (that particular example custom tools)

@anntzer
Copy link
Contributor

anntzer commentedAug 19, 2017
edited
Loading

That example should probably be updated to use another keymap than G, which is now used by default by grid_minor.

Edit: Also this kind of collision should probably be detected and trigger an error or at least a warning from toolmanager.

anntzer
anntzer previously requested changesAug 19, 2017
def get_text(self):
sep = '_' * 80 + '\n'
t = sep
t += "{0:15} {1:25} {2}\n".format(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The column and separator widths should get computed from the strings that will actually be used, instead of being hardcoded. Same comment applies below.
Or the descriptions and keys should get textwrapped (https://docs.python.org/3/library/textwrap.html#textwrap.wrap).

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm pretty bad with fonts, if I keep using monospace, how can I know what is the "length" (in characters) of the canvas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I mean replace 15 and 25 by whatever number is necessary (just count the characters of the longest string).

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I understand, but wouldn't it be better to make it to adjust to the number of characters that fit into the "width"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It's monospace so by definition all characters have the same width.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, that I know. But don't size 20 what does it mean in terms of width?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I am no expert of fonts, but I think there is no way toprecisely know that in asimple way (i.e. outside of the "font engine"). However someSO sources (here is an archive of the mentioned original source) suggest sthat a 60 % width-to-height ratio may be a quite good approximation for monospace fonts.

So if I am not too wrong, a formula like:

Nmax of chars = width in inches / ((0.6 + epsilon) * fontsize in points * 1/72)

may be a rather easy way to estimate the number of characters that should fit in a given width. (PS: in case it is not clear, the PostScript point is 1/72 of international inch)

Anyway, AFAIU@anntzer´s suggestion, his idea was more to use columns of minimal width and that do not wrap the text (which may be easier to implement ^^)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, my point was as described by@afvincent.
If you really care about physical text size I think you need to use backend.get_text_width_height_descent but then you may as well look into how text wrapping is implemented (and not use a monospace font, but a plt.table).

@fariza
Copy link
MemberAuthor

@anntzer the warning is in place from the begining.
If you run the exampleexamples/user_interfaces/toolmanager_sgskip.py

you will get a warning
workspace/matplotlib/lib/matplotlib/backend_managers.py:209: UserWarning: Key G changed from grid_minor to Show (k, self._keys[k], name))

@fariza
Copy link
MemberAuthor

It took me some time, but I decided to try to guess the number of characters per line (monospace) using the approx 60% (thanks for the tip).

Here is how it looks now, using this approach

screenshot from 2017-09-06 19-42-40

@anntzeranntzer dismissed theirstale reviewSeptember 7, 2017 00:02

issues addressed

@anntzer
Copy link
Contributor

Looks like the first column is too wide and the second one not wide enough? (the nav keys could definitely fit on a single line)

As a side note, the description for allnav and nav should be changed to "Enable" instead of "Enables",
and the inner caps in Toggle Fullscreen mode and Toggle Scale X/Y removed (for consistency with the other descriptions). There's also a typo Toogle -> Toggle (multiple times).

@fariza
Copy link
MemberAuthor

@anntzer I changed to a 6 column setup 1,2,3 widths so, the second is the double of the first and the third is half the total.
Nav keys don't fit into a sinlge line, but it is exceptionally long

screenshot from 2017-09-07 14-31-00

@@ -14,6 +14,8 @@

from matplotlib import rcParams
from matplotlib._pylab_helpers import Gcf
from matplotlib.table import Table
import textwrap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

move the import down per PEP8

self.text_axes = None

def enable(self, *args):
self.text_axes = self.figure.add_axes((0, 0, 1, 1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think you should create the axes with a custom (internal) label (with a comment) to workaround#9024 (although that bad behavior is now deprecated), just in case.

@anntzer
Copy link
Contributor

few minor things, but LGTM otherwise.

@tacaswelltacaswell modified the milestones:2.1 (next point release),2.2 (next next feature release)Sep 24, 2017
@fariza
Copy link
MemberAuthor

Is there something left to do for this PR?

@tacaswelltacaswell modified the milestones:needs sorting,v3.0Feb 26, 2018
@fariza
Copy link
MemberAuthor

@anntzer@afvincent Is there something missing?


def disable(self, *args):
self.text_axes.remove()
self.figure.canvas.draw_idle()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Does this mean that after having pressed that button 10 times, you have 10 unusedAxes in memory? Should it maybe rather reuse an existing axes, or comletely delete the unused axes?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

when creating the axes we specify the label, so if I understand it correctly it should reuse it if available

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

That behavior (of reusing axes) is deprecated.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

But's it's not available, because you removed it from the figure. And if you didn't remove it, it would cause a warning, telling you that creating an instance with the exact same arguments will return a new instance in the future.

Simple test case:

fig = plt.figure()ax1 = fig.add_axes((0, 0, 1, 1), label='help_tool_axes')ax1.remove()ax2 = fig.add_axes((0, 0, 1, 1), label='help_tool_axes')print(ax1==ax2)    # This will print False.

I think it would indeed be a good idea to reuse the axes, but I currently would not know how to do that. I.e. how would one add an axes to a figure which has previously been removed? (fig.add_axes(ax) does not seem to work.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Create it once, callset_visible(True), callset_visible(False).

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done, now it creates it once, set the visibility to change.
I clear the axes before filling it because the tools are dynamic, so the content might change between different calls

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Setting the visibility lets the axes stay infig.axes. This might change any program's complete logic depending on whether or not you are using the ToolManager to show the figure and whether or not you have previously toggled the help or not.

Simple example:

for ax in fig.axes:    # do something.

@anntzer
Copy link
Contributor

See#10851 (comment).

@ImportanceOfBeingErnest
Copy link
Member

There might be another problem with this approach of adding an axes. I think if you happen to have setfig.set_tight_layout(True) and then enter help mode, it would trigger a warning about the axes not being compatible with tight_layout.

@timhoffm
Copy link
Member

Not quite sure if it is generally a good idea to do this within the figure.

Advantage: Works for all backends.
Disadvantage: May mess up an existing plot.

Alternatives:

  • Backend-native popup window. More work (i.e. per backend), but completely decoupled from the drawing logic.
  • Compromise: Show this in a separate figure.

@anntzer
Copy link
Contributor

fwiw I am pretty strongly in favor of using backend-native stuff whenever possible (and yes, I know that@fariza thinks the opposite :-))...

@@ -1020,6 +1021,75 @@ def _mouse_move(self, event):
self.toolmanager.canvas.draw_idle()


class HelpTool(ToolToggleBase):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should be ToolHelp for consistency (or rather all others should be FooTool, but heh).

@ImportanceOfBeingErnest
Copy link
Member

Creating a separate figure is really easy if one can rely on using pyplot for that,

self.auxfig = pyplot.figure()# ....self.auxfig.show()

Would there be a reason for this not to be the case?

For the other proposal, maybe there should be a general pupose popup child window in the toolmanager which provides aset_text andget_figure method.
This could then be used by the helper tool, but equally by all any other tool that needs some window.
Of course it would need to be implemented by all backends and individual backends could implement further methods if needed.
Once such a window exists, the creation of new tools which need a separate figure or just some popup window would be really easy.

@anntzeranntzer mentioned this pull requestApr 13, 2018
6 tasks
@fariza
Copy link
MemberAuthor

fariza commentedApr 13, 2018 via email

My problem with specific backend stuff is that we have to support it. Itlooks better I agree.It is easy to create one window, set the title, set the size plug thecallbacks, etc.. but then doing it for 5 or 6 backends becomes cumbersome.And then if you want to do it for 1, 2 or 10 different tools you will endwith too many windows creatio/control to do it efficiently.That is why MEP27 exists and why we have been trying to get it properlyreviewed for years. We want to do that window creation process for eachbackend once AND only once. Then implement generic tools (as much aspossible) that can be shared and maintained easily
On Fri, Apr 13, 2018, 3:55 AM Antony Lee, ***@***.***> wrote: fwiw I am pretty strongly in favor of using backend-native stuff whenever possible (and yes, I know that@fariza <https://github.com/fariza> thinks the opposite :-))... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9022 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABa86akUWkz6WzYgyyGwJJC_ado_rlpZks5toFnggaJpZM4O1L_t> .

@fariza
Copy link
MemberAuthor

@ImportanceOfBeingErnest we don't want to use pyplot, that is something that we want removed somewhere in the future.

@timhoffm
Copy link
Member

Not having read through the details of MEP27, but that seems to only target the plot window, not general purpose windows for displaying help.

I see two possible strategies:

  • Either use this code but display it in a separate figure. For the time being, one can rely on pyplot for handling the backend-specific stuff for figure creation. Once MEP27 gets accepted, we can migrate the help from pyplot to the new API.
  • Or, use the native widgetsHelp tool. #11045. As long as there is not more than the help window, that's fine in terms of complexity. If you want to do it for 10 different things, one will have to think about internal backend abstractions for simple general-purpose windows. I assume that you would want some interactive elements like buttons etc. as well, which is also not easy when drawing it all on a figure.

I tend to go with the native widgets.

@farizafariza closed thisApr 18, 2018
@farizafariza deleted the help-tool branchApril 18, 2018 18:21
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer left review comments

@afvincentafvincentafvincent left review comments

@ImportanceOfBeingErnestImportanceOfBeingErnestImportanceOfBeingErnest left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.

9 participants
@fariza@afvincent@WeatherGod@story645@anntzer@ImportanceOfBeingErnest@timhoffm@tacaswell@jklymak

[8]ページ先頭

©2009-2025 Movatter.jp