Fix handling of getSaveFileName to be consistent [backport to 1.4.x]

[mfitzp]

PyQt4 and PyQt5 handle getSaveFileName differently, returning either
a filename or a filename,filter tuple respectively. PySide behaves as
for PyQt5. This caused bug #3454 producing an 'format not supported'
error as the tuple did not match any of the string types.

This updates each API to return a tuple as per PyQt5 (following the
principle of using the latest API as a target). For recent PyQt4
this means using getSaveFileNameAndFilter instead, for older PyQt4
we wrap the function to output a tuple.

Figure saving has been tested on PySide, PyQt4 and PyQt5.

[tacaswell]

There is no back-compatibility issue here because this is all private details and anyone using them knew the risks.

[tacaswell]

And it looks like there are a couple of pep8 issues

======================================================================

FAIL: matplotlib.tests.test_coding_standards.test_pep8_conformance

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/virtualenv/python2.7.8/lib/python2.7/site-packages/nose/case.py", line 197, in runTest

self.test(*self.arg)

File "/home/travis/virtualenv/python2.7.8/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-linux-x86_64.egg/matplotlib/tests/test_coding_standards.py", line 253, in test_pep8_conformance

assert_pep8_conformance()

File "/home/travis/virtualenv/python2.7.8/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-linux-x86_64.egg/matplotlib/tests/test_coding_standards.py", line 237, in assert_pep8_conformance

assert_equal(result.total_errors, 0, msg)

AssertionError: Found code syntax errors (and warnings):

/home/travis/virtualenv/python2.7.8/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-linux-x86_64.egg/matplotlib/backends/backend_qt5.py:718:80: E501 line too long (85 > 79 characters)

/home/travis/virtualenv/python2.7.8/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-linux-x86_64.egg/matplotlib/backends/qt_compat.py:85:80: E501 line too long (83 > 79 characters)

/home/travis/virtualenv/python2.7.8/lib/python2.7/site-packages/matplotlib-1.5.x-py2.7-linux-x86_64.egg/matplotlib/backends/qt_compat.py:126:1: W391 blank line at end of file

----------------------------------------------------------------------

Ran 1 test in 26.138s

[mfitzp]

@tacaswell what you do mean by "There is no back-compatibility issue here because this is all private details and anyone using them knew the risks."? The changes don't attempt backwards compatibility just update the backends to correctly follow the "do it like PyQt5" (+PySide coincidentally) approach. Were you thinking if others may be using the matplotlib qt_compatwrapper elsewhere and expecting the behaviour of _getSaveFileName to stay the same?

I've fixed the PEP8 compliance issues and added another commit. I can combine them into one if needed?

[WeatherGod]

I think he was merely making a note to himself, reminding himself that this
does not technically break backwards compatibility because users shouldn't
have been using this part of the API anyway.

On Fri, Sep 5, 2014 at 4:15 AM, Martin Fitzpatrick <notifications@github.com

wrote:

@tacaswell https://github.com/tacaswell what you do mean by "There is
no back-compatibility issue here because this is all private details and
anyone using them knew the risks."? The changes don't attempt backwards
compatibility just update the backends to correctly follow the "do it like
PyQt5" (+PySide coincidentally) approach. Were you thinking if others may
be using the matplotlib qt_compatwrapper elsewhere and expecting the
behaviour of _getSaveFileName to stay the same?

I've fixed the PEP8 compliance issues and added another commit. I can
combine them into one if needed?

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

[mfitzp]

Thanks @WeatherGod. It's worth noting then @tacaswell that there is no need to break back-compatibility if it is an issue, as the wrapper can simply be written to look like the old-style. I changed it purely for the sake of staying as-close-as-possible to the latest API version, but as it is a private API feature that may not be considered important.

[tacaswell]

@WeatherGod hit it on the head. My first reaction was that this broke 'public' api (I at least use the qt_compat module in my own code and have suggested that scikit-image do the same), but it only affects 'private' (ie _foo) functions so there is no need to make said wrapper.

[tacaswell]

Merged to master as b892838

cherry-picked to 1.4.x as fe125ec

[tacaswell]

@mfitzp Thanks!

Good luck with the writing and the move. I did that about 9 months ago.