Add a quit_all key to the default keymap

[thisch]

In this pull request, I have:

• Added a new key ("W") which closes all open figures
• Added "q" to the list of default keys for closing figures (if you still think that this should be opt-in, I'll remove it - see #851)

I have tested this on GNU/Linux with the mpl Qt4 backend.

[mdboom]

Sorry to have let this sit here for so long. It seems like a good idea to me, but I'd like a little more feedback first in case there's something I'm missing.

@fariza, @efiring, @tacaswell

[WeatherGod]

One thing I worry about with adding more default keys is possible
collisions with existing applications.

On Tue, Oct 6, 2015 at 12:01 PM, Michael Droettboom <
notifications@github.com> wrote:

Sorry to have let this sit here for so long. It seems like a good idea to
me, but I'd like a little more feedback first in case there's something I'm
missing.

@fariza https://github.com/fariza, @efiring https://github.com/efiring,
@tacaswell https://github.com/tacaswell

—
Reply to this email directly or view it on GitHub
#4921 (comment)
.

[thisch]

I think the 2.0 release is a perfect release for new default keys, as there are already a lot of changes of the defaults planned. If not now for the 2.0 release, when?

[WeatherGod]

The problem I see is that 2.0 has been advertised as a style changing
release, not an API changing release. We have stated that code that runs on
1.5 will also run on 2.0 with the only difference being the appearance of
the results.

On Tue, Oct 6, 2015 at 1:35 PM, Thomas Hisch notifications@github.com
wrote:

I think the 2.0 release is a perfect release for new default keys, as
there are already a lot of changes of the defaults planned. If not now for
the 2.0 release, when?

—
Reply to this email directly or view it on GitHub
#4921 (comment)
.

[WeatherGod]

Note, with the new tool manager system coming down the pike, it will be
easier to report key collisions and handle them better, assuming that the
user's application is also using the Tool manager system. This will make it
possible to make changes to default key bindings in the future.

On Tue, Oct 6, 2015 at 1:41 PM, Benjamin Root ben.v.root@gmail.com wrote:

The problem I see is that 2.0 has been advertised as a style changing
release, not an API changing release. We have stated that code that runs on
1.5 will also run on 2.0 with the only difference being the appearance of
the results.

On Tue, Oct 6, 2015 at 1:35 PM, Thomas Hisch notifications@github.com
wrote:

I think the 2.0 release is a perfect release for new default keys, as
there are already a lot of changes of the defaults planned. If not now for
the 2.0 release, when?

—
Reply to this email directly or view it on GitHub
#4921 (comment)
.

[fariza]

I don't see any problems other than adding more Gcf stuff (we are trying to get rid of it with MEP27).
But in any case that functionality has to be there.

What about adding it only in toolmanager? with that we avoid adding new functionality and no collision problem with existing keys.

[tacaswell]

While I am normally a fan of foot-cannons, this strikes me as a potentially destructive one without a huge upside (like the sometimes default binding of 'ctrl-alt-backspace' to KILL MY X11 SESSION).

[WeatherGod]

I have zero problem with adding it only to toolmanager. Gives more reason
to get people to port their applications over to use it.

On Tue, Oct 6, 2015 at 1:46 PM, Federico Ariza notifications@github.com
wrote:

I don't see any problems other than adding more Gcf stuff (we are trying
to get rid of it with MEP27).
But in any case that functionality has to be there.

What about adding it only in toolmanager? with that we avoid adding new
functionality and no collision problem with existing keys.

—
Reply to this email directly or view it on GitHub
#4921 (comment)
.

[thisch]

So I'll try to add it to the new toolmanager, if you are fine with it.

Edit: To test it I have to use the GTK3 or the Tk backend, is this correct?

[fariza]

yes, for the time being only those two backends are supported

[efiring]

On 2015/10/06 6:01 AM, Michael Droettboom wrote:

Sorry to have let this sit here for so long. It seems like a good idea
to me, but I'd like a little more feedback first in case there's
something I'm missing.

@fariza https://github.com/fariza, @efiring
https://github.com/efiring, @tacaswell https://github.com/tacaswell

I'm not a fan of extensive default key mappings. I was annoyed when
they were first introduced into mpl without my having noticed. Every
now and then a plot would jump to full screen. It took a while before I
figured out that this was because I had bumped "f".
Nevertheless, I think the mappings proposed here make sense. They are
consistent with what we have, they don't look too easy to hit
accidentally, and they address a reasonably common use case.

[thisch]

@fariza I can't test the new toolbarmanager (with the tk backend) due to the following exception

  File "/home/thomas/miniconda/envs/py3k/lib/python3.5/site-packages/matplotlib-1.5.0+199.g45d1c34-py3.5-linux-x86_64.egg/matplotlib/pyplot.py", line 527, in figure
    **kwargs)
  File "/home/thomas/miniconda/envs/py3k/lib/python3.5/site-packages/matplotlib-1.5.0+199.g45d1c34-py3.5-linux-x86_64.egg/matplotlib/backends/backend_tkagg.py", line 84, in new_figure_manager
    return new_figure_manager_given_figure(num, figure)
  File "/home/thomas/miniconda/envs/py3k/lib/python3.5/site-packages/matplotlib-1.5.0+199.g45d1c34-py3.5-linux-x86_64.egg/matplotlib/backends/backend_tkagg.py", line 112, in new_figure_manager_given_figure
    figManager = FigureManagerTkAgg(canvas, num, window)
  File "/home/thomas/miniconda/envs/py3k/lib/python3.5/site-packages/matplotlib-1.5.0+199.g45d1c34-py3.5-linux-x86_64.egg/matplotlib/backends/backend_tkagg.py", line 549, in __init__
    backend_tools.add_tools_to_container(self.toolbar)
  File "/home/thomas/miniconda/envs/py3k/lib/python3.5/site-packages/matplotlib-1.5.0+199.g45d1c34-py3.5-linux-x86_64.egg/matplotlib/backend_tools.py", line 924, in add_tools_to_container
    container.add_tool(tool, group, position)
  File "/home/thomas/miniconda/envs/py3k/lib/python3.5/site-packages/matplotlib-1.5.0+199.g45d1c34-py3.5-linux-x86_64.egg/matplotlib/backend_bases.py", line 3223, in add_tool
    image, tool.description, toggle)
  File "/home/thomas/miniconda/envs/py3k/lib/python3.5/site-packages/matplotlib-1.5.0+199.g45d1c34-py3.5-linux-x86_64.egg/matplotlib/backends/backend_tkagg.py", line 952, in add_toolitem
    button = self._Button(name, image_file, toggle, frame)
  File "/home/thomas/miniconda/envs/py3k/lib/python3.5/site-packages/matplotlib-1.5.0+199.g45d1c34-py3.5-linux-x86_64.egg/matplotlib/backends/backend_tkagg.py", line 973, in _Button
    im = Tk.PhotoImage(master=self, file=image_file)
  File "/home/thomas/miniconda/envs/py3k/lib/python3.5/tkinter/__init__.py", line 3393, in __init__
    Image.__init__(self, 'photo', name, cnf, master, **kw)
  File "/home/thomas/miniconda/envs/py3k/lib/python3.5/tkinter/__init__.py", line 3349, in __init__
    self.tk.call(('image', 'create', imgtype, name,) + options)
_tkinter.TclError: couldn't recognize data in image file "/home/thomas/miniconda/envs/py3k/lib/python3.5/site-packages/matplotlib-1.5.0+199.g45d1c34-py3.5-linux-x86_64.egg/matplotlib/mpl-data/images/home.png"

It doesn't seem to be a problem with the tk package, because everything works fine with the old toolbar.

[fariza]

@thisch how are you testing it? I can't reproduce the problem

[thisch]

I use conda and installed matplotlib from the latest git version with python setup.py install.

This simple command triggeres the exception

python -c 'import matplotlib.pyplot as plt; plt.figure()'

[thisch]

@fariza does the following example work for you?

from tkinter import *          

root = Tk()                    

#  adapt it
fname = '/home/thomas/miniconda/envs/py3k/lib/python3.5/site-packages/matplotlib-1.5.0+199.g45d1c34-py3.5-linux-x86_64.egg/matplotlib/mpl-data/images/home.png'
photo = PhotoImage(file=fname)
photo_label = Label(image=photo)
photo_label.grid()             
photo_label.image = photo      

text = Label(text="Text") # included to show background color
text.grid()    

root.mainloop()

edit: saving the png file as a gif seems to work ... hmm
see http://stackoverflow.com/questions/15718663

[fariza]

Your example works for me but I'm using 3.4 not 3.5.

[thisch]

It even does not work with 3.4. I guess this is yet another problem with a conda package.

Edit: I've just verified that it works with the tkinter version shipped with fedora.

[fariza]

Maybe is the tkinter version.

@tacaswell according to http://effbot.org/tkinterbook/photoimage.htm PhotoImage supports only GIF, PNG/PPM.
In your experience, is this correct?

[thisch]

Isn't the tkinter version always tied to the python version?

[thisch]

Maybe the conda tk package was not compiled with png support (@asmeurer)

Edit:

http://www.muonics.com/FreeStuff/TkPNG/

Prior to version 8.6, Tk does not have native support for any image formats that allow for alpha (partial-> transparency) channels, although it does have support for alpha blending internally.

[fariza]

I think the time has come to create an "image selector" to load the appropriate image for each backend/resolution combination

@tacaswell how are we handling the image selection for hpdi displays?
As I can see the images/*pdf files are only used for mac. is this correct?

Is there any ongoing effort to standardize this?

[thisch]

@fariza can you reproduce the _tkinter.TclError exception within a conda environment, where the tk version is 8.5?

[fariza]

@thisch sorry for the delay, I just tried with a windows7 with anaconda version 3.16.0 but tk.TkVersion is 8.6 and it works.

[thisch]

@fariza, since the tk 8.6 package is neither available in the official anaconda repo nor in the 'anaconda cloud', I guess that your installed tk package is from a different source. Is this possible?

Independently, this PR already contains the necessary class to be used with the 'toolmanager', right?

[fariza]

I think we are going to leave it only inside toolmanager, please remove it from backend_bases

[thisch]

Is it ok to leave this change as it is?

[fariza]

I don't see mayor problem with that @tacaswell what do you think?

[pelson]

I don't think we need 3 keys for the same command. How about removing the "W" and "Q" and just having cmd+W?

[fariza]

@thisch I think this is almost ready to go, just remove the backend_bases changes, we will keep this addition only for new toolmanager

[thisch]

@fariza done. Could you please test it, since my toolbarmanager setup still does not work.

[thisch]

@fariza, what needs to be done to have a working toolmanager implementation for the qt[45] backend? I'm interested in helping out.

[fariza]

@thisch we are waiting for MEP27 to be reviewed/merged before porting more backends, why you may ask? this is because MEP27 simplifies many things regarding window handling that is required for each backend.
The work on MEP27 is #4143
There you can take a look at what it takes for each backend, at the moment there are only GTK3 so the QT work is welcome.

For the specific QT side of things, take a look at https://github.com/OceanWolf/matplotlib/tree/backend-refactor-qt I would recommend to contact @OceanWolf before going any further, maybe some of that code is outdated or maybe is done and just need review....

[OceanWolf]

Hmm, I really don't like the usage of Gcf here, I specifically removed the import of Gcf from backend_tools.py here in https://github.com/matplotlib/matplotlib/pull/4143/files#diff-61876e8eb555e63c6c387e2bf83761c4.

If we really want this here, may I propose we import Gcf within in the tool, but I presume a better way exists to do this than through Gcf.

How does this work for embeded applications that want to use the default toolbar and shortcut keys? It feels like something that they should have to take away... I think I would rather have the FigureManager add the FigureManager specific tools (i.e. the ones to do with the window).

[fariza]

I don't like the idea of importing Gcf inside the tool.
@OceanWolf I know we are going to remove Gcf and I agree with that. Is there any other way to get the list of figures?

[WeatherGod]

I think this is a really good point, and it points out a breakage in the
conceptual model of the tools. The implicit assumption is that the instance
of the tools apply to the contents of a single figure window, not globally.
I am still really uneasy about adding such a feature.

On Wed, Jan 13, 2016 at 11:47 AM, OceanWolf notifications@github.com
wrote:

Hmm, I really don't like the usage of Gcf here, I specifically removed the
import of Gcf from backend_tools.py here in
https://github.com/matplotlib/matplotlib/pull/4143/files#diff-61876e8eb555e63c6c387e2bf83761c4
.

If we really want this here, may I propose we import Gcf within in the
tool, but I presume a better way exists to do this than through Gcf.

How does this work for embeded applications that want to use the default
toolbar and shortcut keys? It feels like something that they should have to
take away... I think I would rather have the FigureManager add the
FigureManager specific tools (i.e. the ones to do with the window).

—
Reply to this email directly or view it on GitHub
#4921 (comment)
.

[OceanWolf]

@fariza conceptually the whole point of Gcf acts as a global figure manager, another way shouldn't exist otherwise it just does the same job as Gcf.

I have come to this conclusion while looking at the two backends where I can't see a way to get rid of Gcf, namely Pdf which compiles all the figures into a single pdf document; and the WebAgg backend which allows one to view a specific figure by figure_number in the url (these two remaining issues can get resolved later and not part of the current refactors).

[tacaswell]

The important thing is to get Gcf out of the backend files so that the managers and company can be used with out having to touch the global state.

However, when you use pyplot to create the windows you have access to both Gcf (because you are in pyplot) and the manager (because pyplot just created it and told Gcf about it) so can tack on the key bindings to close all figures at that point.

I think this sort of dependency injection gets what everyone wants with minimal extra complications.

exactly how we pick apart Pdf and WebAgg can be dealt with later.