Fix #5687: Don't pass unicode to QApplication() #5690

Merged
merged 1 commit into from Dec 22, 2015

Conversation

Projects
None yet
3 participants
Contributor

pankajp commented Dec 17, 2015

PySide QApplication constructor cannot handle unicode text
(segfaults), so we instead pass an empty list to the constructor.

Fixes #5687

jdmarch commented Dec 17, 2015

Confirmed fixes segfault on Mac and Linux

Owner

tacaswell commented Dec 17, 2015

Thing change went in in 378e37d so this is definitily a regression from 1.4.

Sorry, I could have sworn I tested this against PySide when this went in.

attn @mfitzp

tacaswell added the GUI/Qt label Dec 17, 2015

Contributor

pankajp commented Dec 18, 2015

@tacaswell Thanks for pointing to the documentation which states there must be atleast one string in the list. I was not aware of that. In fact i tested using a empty list and it works fine in all combinations of PySide/PyQt4/PyQt5 X python2/python3
I guess I should just change it back to a normal str with space then as it was before that commit.

@pankajp pankajp Fix #5687: Don't pass unicode to QApplication()
PySide QApplication constructor cannot handle unicode text.
793accd
Contributor

pankajp commented Dec 18, 2015

I have reverted the QApplication() args to the way it was before commit 378e37d broke it.

Owner

tacaswell commented Dec 18, 2015

If an empty list works, I would assume the documentation is wrong 😈

Is this only a pyside + LPy issue? I think it would be better to have conditional code to special case that rather than have incorrect unicode handling.

Contributor

pankajp commented Dec 18, 2015

I don't think passing string (in py2) is incorrect behavior because the underlying C++ method accepts char**, and also because passing unicode segfaults pyside. It was changed to unicode in a misguided attempt to unify text type. Using unicode here has no correctness benefit simply because we are passing in a constant, and not text which can be set by user and/or the OS.

Owner

tacaswell commented Dec 19, 2015

Can you also file a bug/is there a bug upstream with pyside?

I guess this is ok. The path to dropping py2 is going to include a sed s/six.text_type/str/g operation anyway and as you point out we do not take user input here so we can not be surprised by unicode input.

Test failures look un-related (and my fault 😞 )

tacaswell closed this Dec 21, 2015

tacaswell reopened this Dec 21, 2015

@tacaswell tacaswell added needs_review and removed needs_review labels Dec 21, 2015

tacaswell self-assigned this Dec 22, 2015

@tacaswell tacaswell added a commit that referenced this pull request Dec 22, 2015

@tacaswell tacaswell Merge pull request #5690 from pankajp/patch-1
Fix #5687: Don't pass unicode to QApplication()
fd83789

@tacaswell tacaswell merged commit fd83789 into matplotlib:master Dec 22, 2015

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 68.319%
Details

tacaswell removed the needs_review label Dec 22, 2015

Owner

tacaswell commented Dec 22, 2015

@pankajp Can you please follow up with upstream (pyside) on this?

pankajp deleted the pankajp:patch-1 branch Dec 22, 2015

Contributor

pankajp commented Dec 22, 2015

Reported upstream: https://bugreports.qt.io/browse/PYSIDE-306. Should be easy to fix if someone gets the time.

Owner

tacaswell commented Dec 22, 2015

Thanks !

On Tue, Dec 22, 2015, 00:11 Pankaj Pandey notifications@github.com wrote:

Reported upstream: https://bugreports.qt.io/browse/PYSIDE-306. Should be
easy to fix if someone gets the time.


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

@tacaswell tacaswell added a commit that referenced this pull request Jan 1, 2016

@tacaswell tacaswell Merge pull request #5690 from pankajp/patch-1
Fix #5687: Don't pass unicode to QApplication()
1241122
Owner

tacaswell commented Jan 1, 2016

Backported as 1241122

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