use Qt window title as default savefig filename #908

Merged
merged 9 commits into from Jul 9, 2012

Conversation

Projects
None yet
4 participants
Contributor

mspacek commented May 29, 2012

When in IPython, I tend to set my figure window title to the last command entered in IPython, ostensibly the command used to generate the figure. When I save the figure, I like to use the same string for the filename, but I've always had to punch that in manually. This patch automates this, but only for the Qt backend, since I'm not familiar with any others. For the typical figure where the user doesn't change the window title, the default filename is now the slightly more descriptive "Figure_x" instead of just "image".

Member

pelson commented May 29, 2012

A very pertinent mailinglist post: http://old.nabble.com/How-to-give-a-name-to-a-figure--td19498850.html .

I think this pull request should include the definition of an interface to get the window title (as suggested in the mailing list) in a general way in the backend_bases.py, and that your changes make qt the first implementer of the new interface (hence, other backends could then follow suit in time).

@pelson pelson commented on an outdated diff May 29, 2012

lib/matplotlib/backends/backend_qt.py
@@ -431,8 +431,10 @@ def save_figure(self, *args):
sorted_filetypes = filetypes.items()
sorted_filetypes.sort()
default_filetype = self.canvas.get_default_filetype()
+ default_filename = self.canvas.window().windowTitle() or 'image'
+ default_filename.replace(' ', '_')
@pelson

pelson May 29, 2012

Member

Careful here. Replace is not done in place:

>>> a = 'foo bar'
>>> a.replace(' ', '_')
'foo_bar'
>>> a
'foo bar'

@pelson pelson commented on an outdated diff May 29, 2012

lib/matplotlib/backends/backend_qt4.py
@@ -555,8 +555,10 @@ def save_figure(self, *args):
sorted_filetypes = filetypes.items()
sorted_filetypes.sort()
default_filetype = self.canvas.get_default_filetype()
+ default_filename = self.canvas.window().windowTitle() or 'image'
+ default_filename.replace(' ', '_')
@pelson

pelson May 29, 2012

Member

Ditto.

Contributor

mspacek commented May 30, 2012

Whoops, I've made that string in-place error before. Strange thing was it didn't show up in testing. That was because it was actually a QString, for which replace() is in-place apparently :)

I agree, there should be a canvas.get_window_title() method. Should be easy enough to add, at least for qt. I'll get to it shortly.

Member

pelson commented May 30, 2012

That was because it was actually a QString, for which replace() is in-place apparently :)

Ouch. Thanks for the clarification.

Contributor

mspacek commented May 30, 2012

OK, FigureCanvasBase and FigureManagerBase now both have a get_window_title() method. The qt4 backend's FigureCanvasQT overrides it. I've left it undefined for the qt backend, since I don't really know how to get the window title (I presume the qt backend is for qt3), and a quick google search didn't bring up a caption() or getCaption() method analogous to qt3's setCaption() method.

My only concern is that perhaps get_window_title() and set_window_title() should raise NotImplementedErrors in FigureCanvasBase, instead of just using pass. That way, users won't be confused by silent errors on backends for which these methods aren't defined.

Owner

mdboom commented May 30, 2012

Yes -- I think the base class should raise NotImplementedError. It would be great to implement this in at least the major GUI backends (wx, gtk, gtk3, tk, qt4) if possible -- historically, the backends tended to lose feature parity when we don't take care of these things right away. I can probably find some time in the next few days to look at that and extend this PR if you don't have a chance.

Contributor

mspacek commented May 30, 2012

OK, done. Sure, if you (or anyone else) with access to the other backends could implement them, that would be great.

Member

pelson commented May 31, 2012

@mspacek I have sent you a pull request, against this branch, which implements the title in the remaining backends. I have been able to test on a Linux (standard Ubuntu 12.04) machine.

@pelson pelson and 2 others commented on an outdated diff Jun 1, 2012

lib/matplotlib/backend_bases.py
@@ -2077,6 +2088,15 @@ def set_window_title(self, title):
if hasattr(self, "manager"):
self.manager.set_window_title(title)
+ def get_default_filename(self):
+ """
+ Return a string, which includes extension, suitable for use as
+ a default filename.
+ """
+ default_filename = self.get_window_title() or 'image'
+ default_filename = default_filename.replace(' ', '_')
@pelson

pelson Jun 1, 2012

Member

As you probably guessed, I would prefer the filename to be lower case for the same reason that the spaces have been removed. Having said that, I'm not overly fussed if you would prefer to keep mixed case.

@mdboom

mdboom Jun 1, 2012

Owner

I think I agree with the lowercase thing. It's more of a gut feeling though -- and part of me feels like it should be lower case on Unix and mixed case on Windows, but that's probably a bad idea ;)

@mspacek

mspacek Jun 1, 2012

Contributor

I guess I disagree with going lowercase because it's a loss of information, but I'm outnumbered, so I'll change it back :)

Owner

mdboom commented Jun 1, 2012

This looks great. Just tested all GUI backends on Linux and seems to work as advertised.

@pelson pelson commented on an outdated diff Jun 3, 2012

lib/matplotlib/backend_bases.py
def set_window_title(self, title):
"""
Set the title text of the window containing the figure. Note that
this has no effect if there is no window (eg, a PS backend).
"""
- pass
+ raise NotImplementedError
@pelson

pelson Jun 3, 2012

Member

One thing I am not able to test is the impact of this line. For instance, does the MacOS backend blow up now before any plot is produced? Are there other backends which we don't have sight of which will have this method called, with the obvious impact of having an unexpected exception raised?

Contributor

mspacek commented Jun 3, 2012

@pelson, I guess the point is we want an exception to be raised, so that it doesn't fail silently. And then if reports of failure come back, they can be dealt with as needed?

It seems there isn't a get_window_title or set_window_title in FigureManagerMac. Maybe I should add them both and print a warning? I suppose set_window_title was never implemented, and I don't see it hidden anywhere in _macosx.FigureManager in _macosx.m.

Owner

mdboom commented Jun 4, 2012

I think @pelson raises a good point about raising NotImplementedError. Maybe get_window_title should instead return some reasonable default in the base class so backends don't add them will continue to work -- with effort of course being put into bringing all the backends in line. I'm unfortunately not a regular Mac user (I have a shared work machine I can fire up on occasion). We should try to get the Mac OS-X backend updated if possible. (It's always a tricky one to get done, since it's the only non-cross-platform backend).

Contributor

mspacek commented Jun 4, 2012

OK. What would be a reasonable default in the base class? A warning? Or should I just restore it to pass?

Owner

mdboom commented Jun 4, 2012

I think get_window_title should return "image" to maintain the old behavior, which was to use "image.png" as the default filename. set_window_title should probably emit a warning, but otherwise do nothing. That's just my 2c -- there might be an even better way.

Contributor

mspacek commented Jun 4, 2012

Returning "image" on get_window_title in the base class seems reasonable to me.

The other thing to consider is that we probably want to keep the previous behaviour of nothing happening when set_window_title is called on a non-GUI backend, which is sort of a vote for calling pass instead of raising a warning. I suppose there should really be a subclass for non-GUI backends that would override the warning in the base class with a pass.

Owner

mdboom commented Jun 4, 2012

Good point about the warning. Let's just make it "pass".

Anyone have OS-X and want to tackle implementing this in the mac osx backend?

Member

jkseppan commented Jun 9, 2012

There's a (very lightly tested) attempt for OSX in jkseppan:default_filename: jkseppan/matplotlib@8046c39

Member

pelson commented Jun 9, 2012

@jkseppan: Great stuff! What setup have you been able to test it on?

Member

jkseppan commented Jun 9, 2012

(I updated the commit: jkseppan/matplotlib@08dd29. I submitted it as a pull request to your branch, so you can pull it and this request should automatically update.)

Mac OS X version 10.6.8, Python 2.7 installed from a python.org package:

Python 2.7.2 (v2.7.2:8527427914a2, Jun 11 2011, 15:22:34)
[GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin

As a test, I run:

from matplotlib import pyplot
fig=pyplot.figure()
fig.canvas.set_window_title(u'Hääyöaie')
pyplot.show()

and then click on the save icon. The save window now defaults to "hääyöaie.png" as the filename (though the extension is hidden by default).

Contributor

mspacek commented Jun 9, 2012

Great, thanks for that! I don't understand much in the .m file, but trust that it works.

Owner

mdboom commented Jun 11, 2012

Great! As this now includes all of the major backends, I think this is probably ready to merge. @mspacek: Any remaining issues you're aware of? Can you do a fresh rebase on master?

Member

pelson commented Jun 11, 2012

@mdboom: We've not had any confirmation that this works on windows yet - but we might just have to bite the bullet and merge.
@jkseppan: Were you able to test any of the other backends on your Mac?

Member

jkseppan commented Jun 11, 2012

Not so far, because I had trouble installing the various backend requirements on a Mac - but I'm installing some of them via MacPorts now, so I should get to testing the backends soon.

Member

jkseppan commented Jun 11, 2012

I was able to test with TkAgg and GtkAgg (Python 2.7.3 installed via MacPorts). Both work; in GtkAgg I can't set the title to a unicode string.

MacPorts is still installing the dependencies for Qt, but I couldn't get WxPython to work via MacPorts.

Member

jkseppan commented Jun 11, 2012

With the Qt4Agg backend, the save dialog simply does not appear (Python 2.7.3 via MacPorts).

Owner

mdboom commented Jun 11, 2012

@jkseppan: Interesting. Is the Qt4Agg problem specific to this branch, or is that true on master as well?

Contributor

mspacek commented Jun 11, 2012

@mdboom, nope, no issues that I know of, at least on Qt4Agg in Linux. I hope I've rebased correctly.

Member

jkseppan commented Jun 12, 2012

@mdboom: The same problem occurs on latest master without this change. Getting the dependencies right on OSX is notoriously difficult, so I suspect I have some problem with how I've installed Qt.

Member

pelson commented Jun 13, 2012

@jkseppan: Thanks for all your work on this, its been really valuable to test at least one other OS. The Qt issue sounds like a separate issue to this PR, so unless we can get a windows tester on board, this change has my +1.

Member

pelson commented Jun 15, 2012

@mdboom & @jkseppan: What do you think? Shall we go for the merge?

@mdboom: I assume there is no current means of writing a unit test for this functionality?

Member

jkseppan commented Jun 15, 2012

I agree, go ahead and merge.

Member

jkseppan commented Jul 2, 2012

Update: using homebrew, I have been able to test on OSX 10.6, Python 2.7.3, with the following backends:

try_gtk.py:matplotlib.use('GtkAgg')
try_osx.py:matplotlib.use('MacOSX')
try_qt.py:matplotlib.use('Qt4Agg')
try_tk.py:matplotlib.use('TkAgg')
try_wx.py:matplotlib.use('Wx')

Out of these, only Wx fails (so does WxAgg). I don't see a toolbar, and when I press the s key to save, I get a NotImplementedError. I wonder if I'm doing something wrong?

In any case, I'm still in favor of merging.

backend WX version 2.9.3.1
/opt/homebrew/Cellar/python/2.7.3/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.2.x-py2.7-macosx-10.6-x86_64.egg/matplotlib/backends/backend_wx.py:1412: wxPyDeprecationWarning: Using deprecated class. 
Traceback (most recent call last):
  File "/opt/homebrew/Cellar/python/2.7.3/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.2.x-py2.7-macosx-10.6-x86_64.egg/matplotlib/backends/backend_wx.py", line 1264, in _onKeyDown
    FigureCanvasBase.key_press_event(self, key, guiEvent=evt)
  File "/opt/homebrew/Cellar/python/2.7.3/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.2.x-py2.7-macosx-10.6-x86_64.egg/matplotlib/backend_bases.py", line 1608, in key_press_event
    self.callbacks.process(s, event)
  File "/opt/homebrew/Cellar/python/2.7.3/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.2.x-py2.7-macosx-10.6-x86_64.egg/matplotlib/cbook.py", line 312, in process
    proxy(*args, **kwargs)
  File "/opt/homebrew/Cellar/python/2.7.3/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.2.x-py2.7-macosx-10.6-x86_64.egg/matplotlib/cbook.py", line 239, in __call__
    return mtd(*args, **kwargs)
  File "/opt/homebrew/Cellar/python/2.7.3/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.2.x-py2.7-macosx-10.6-x86_64.egg/matplotlib/backend_bases.py", line 2418, in key_press
    key_press_handler(event, self.canvas, self.canvas.toolbar)
  File "/opt/homebrew/Cellar/python/2.7.3/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.2.x-py2.7-macosx-10.6-x86_64.egg/matplotlib/backend_bases.py", line 2336, in key_press_handler
    toolbar.save_figure()
  File "/opt/homebrew/Cellar/python/2.7.3/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib-1.2.x-py2.7-macosx-10.6-x86_64.egg/matplotlib/backend_bases.py", line 2921, in save_figure
    raise NotImplementedError
Member

pelson commented Jul 5, 2012

@mspacek: Would you mind re-basing this PR, then I will merge.

Contributor

mspacek commented Jul 5, 2012

OK, done. Again, I hope I've rebased correctly. For my own reference, this is roughly what I did:

git co default_filename
git pull origin/master
git rebase origin/master
git push mspacek --force

I noticed that there are two save figure buttons on the figure toolbar now. This must be something new from master.

Member

pelson commented Jul 6, 2012

OK, done. Again, I hope I've rebased correctly.

Thanks @mspacek. This is a nice feature. Thanks for all your work on it. Will merge in 24 hrs.

Contributor

mspacek commented Jul 8, 2012

You're welcome!

@pelson pelson added a commit that referenced this pull request Jul 9, 2012

@pelson pelson Merge pull request #908 from mspacek/default_filename
use window title as default savefig filename
4f75af3

@pelson pelson merged commit 4f75af3 into matplotlib:master Jul 9, 2012

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