don't assume cursor.selectedText() is a string #725

Merged
merged 1 commit into from Sep 9, 2011

Conversation

Projects
None yet
5 participants
@minrk
Member

minrk commented Aug 23, 2011

Apparently in some cases cursor.selectedText() can return None instead of an empty
string (Scientific Linux 6.0 being only reported case). Relax logic using this method,
so string methods are only called when the string is not empty.

Should fix #724

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Aug 23, 2011

Member

+1, correct logic, thanks for fixing this. Merge away.

Since your message doesn't say 'closes gh-724' you may need to do it manually, I don't know if gh will identify 'fix #724' as well.

Member

fperez commented Aug 23, 2011

+1, correct logic, thanks for fixing this. Merge away.

Since your message doesn't say 'closes gh-724' you may need to do it manually, I don't know if gh will identify 'fix #724' as well.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 23, 2011

Member

the actual commit message does have the 'closes' text. The commit parser is pretty greedy, though - I have had it auto-close Issues I only mentioned.

Member

minrk commented Aug 23, 2011

the actual commit message does have the 'closes' text. The commit parser is pretty greedy, though - I have had it auto-close Issues I only mentioned.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Aug 23, 2011

Member

Ah, got it. Good to know, thanks.

Member

fperez commented Aug 23, 2011

Ah, got it. Good to know, thanks.

@epatters

This comment has been minimized.

Show comment
Hide comment
@epatters

epatters Aug 24, 2011

Contributor

It doesn't seem appropriate to merge this. Please see the discussion in #724, in which the user reports further problems.

Contributor

epatters commented Aug 24, 2011

It doesn't seem appropriate to merge this. Please see the discussion in #724, in which the user reports further problems.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 24, 2011

Member

Yes, either we need to place a bound on our PyQt version dependency, or fix every string check to allow for None.

Member

minrk commented Aug 24, 2011

Yes, either we need to place a bound on our PyQt version dependency, or fix every string check to allow for None.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Aug 24, 2011

Member

On Wed, Aug 24, 2011 at 10:24 AM, minrk
reply@reply.github.com
wrote:

Yes, either we need to place a bound on our PyQt version dependency, or fix every string check to allow for None.

My vote is for a version check, since it seems those old versions are
simply broken, as per their own api specification. There's a limit to
how many ugly workarounds we can carry ourselves before the solution
is worse than the original disease...

Cheers,

f

Member

fperez commented Aug 24, 2011

On Wed, Aug 24, 2011 at 10:24 AM, minrk
reply@reply.github.com
wrote:

Yes, either we need to place a bound on our PyQt version dependency, or fix every string check to allow for None.

My vote is for a version check, since it seems those old versions are
simply broken, as per their own api specification. There's a limit to
how many ugly workarounds we can carry ourselves before the solution
is worse than the original disease...

Cheers,

f

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 24, 2011

Member

Fair enough, but what version? Do we just guess that 4.7.0 resolves the issue? I see no evidence of this change in the pyqt docs, and I'm certainly not going to build a bunch of PyQt versions to find out.

Should we make the PySide import version check as well, since early dev versions are known to not work?

Member

minrk commented Aug 24, 2011

Fair enough, but what version? Do we just guess that 4.7.0 resolves the issue? I see no evidence of this change in the pyqt docs, and I'm certainly not going to build a bunch of PyQt versions to find out.

Should we make the PySide import version check as well, since early dev versions are known to not work?

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Aug 24, 2011

Member

On Wed, Aug 24, 2011 at 2:22 PM, minrk
reply@reply.github.com
wrote:

Fair enough, but what version?  Do we just guess that 4.7.0 resolves the issue?  I see no evidence of this change in the pyqt docs, and I'm certainly not going to build a bunch of PyQt versions to find out.

Should we make the PySide import version check as well, since early dev versions are known to not work?

Sorry, no clue. @epatters, do you guys have a good feel for when these
things got fixed, even if it's over conservative?

Cheerrs,

f

Member

fperez commented Aug 24, 2011

On Wed, Aug 24, 2011 at 2:22 PM, minrk
reply@reply.github.com
wrote:

Fair enough, but what version?  Do we just guess that 4.7.0 resolves the issue?  I see no evidence of this change in the pyqt docs, and I'm certainly not going to build a bunch of PyQt versions to find out.

Should we make the PySide import version check as well, since early dev versions are known to not work?

Sorry, no clue. @epatters, do you guys have a good feel for when these
things got fixed, even if it's over conservative?

Cheerrs,

f

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 24, 2011

Member

There's also the hilarious fact that I have no idea how to find out the PyQt version from Python...

Member

minrk commented Aug 24, 2011

There's also the hilarious fact that I have no idea how to find out the PyQt version from Python...

@epatters

This comment has been minimized.

Show comment
Hide comment
@epatters

epatters Aug 24, 2011

Contributor

@fperez I don't know which versions of PyQt have this bug, sorry. It may be worth posting to their mailing list. For PySide, I think that versions 1.0.3+ work with IPython.

@minrk It's not very well advertised, but this works:

from PyQt4 import QtCore
QtCore.QT_VERSION, QtCore.QT_VERSION_STR  # Qt version info
QtCore.PYQT_VERSION, Qtcore.PYQT_VERSION_STR # Binding version info
Contributor

epatters commented Aug 24, 2011

@fperez I don't know which versions of PyQt have this bug, sorry. It may be worth posting to their mailing list. For PySide, I think that versions 1.0.3+ work with IPython.

@minrk It's not very well advertised, but this works:

from PyQt4 import QtCore
QtCore.QT_VERSION, QtCore.QT_VERSION_STR  # Qt version info
QtCore.PYQT_VERSION, Qtcore.PYQT_VERSION_STR # Binding version info
version check Qt bindings in external.qt
require PySide >= 1.0.3 or PyQt4 >= 4.7

closes gh-724
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 24, 2011

Member

Thanks, @epatters, it would have taken me a long time to find that.

I just redid this branch, so that it only adds version checks to external.qt, so the frontend won't try to start with PyQt < 4.7 or PySide < 1.0.3.

Member

minrk commented Aug 24, 2011

Thanks, @epatters, it would have taken me a long time to find that.

I just redid this branch, so that it only adds version checks to external.qt, so the frontend won't try to start with PyQt < 4.7 or PySide < 1.0.3.

@epatters

This comment has been minimized.

Show comment
Hide comment
@epatters

epatters Aug 30, 2011

Contributor

Well, this looks good to me. Thanks, Min.

Contributor

epatters commented Aug 30, 2011

Well, this looks good to me. Thanks, Min.

minrk added a commit that referenced this pull request Sep 9, 2011

Merge pull request #725 from minrk/i724
don't assume cursor.selectedText() is a string

closes gh-724
closes gh-655

@minrk minrk merged commit 6cf495c into ipython:master Sep 9, 2011

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver May 19, 2012

Member

@rajeeja: Hi, welcome to IPython. The best thing for this sort of problem is to join the ipython-user mailing list and ask for help there - that way more people will see your question. http://mail.scipy.org/mailman/listinfo/ipython-user

@rajeeja: Hi, welcome to IPython. The best thing for this sort of problem is to join the ipython-user mailing list and ask for help there - that way more people will see your question. http://mail.scipy.org/mailman/listinfo/ipython-user

This comment has been minimized.

Show comment
Hide comment
@rajeeja

rajeeja May 19, 2012

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #725 from minrk/i724
don't assume cursor.selectedText() is a string

closes gh-724
closes gh-655
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment