Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce renamed methods of QHeaderView in PyQt4 and PySide #65

Merged
merged 5 commits into from Aug 25, 2016

Conversation

tobiasgehring
Copy link
Contributor

No description provided.

@goanpeca
Copy link
Member

goanpeca commented Aug 18, 2016

@tobiasgehring thanks for this :-)

Question:

QHeaderView.sectionsClickable() -> bool
"""
return QHeaderView.isClickable(self)
QHeaderView.sectionsClickable = sectionsClickable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why define a new method each time ? This code should be equivalent:

QHeaderView.sectionsClickable = QHeaderView.isClickable

I think there a re some differences, like the __name__ attribute would still be the one from the initial method. On the other hand the help should be more complete (instead of just the signature in the current state of this PR).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nodd the signatures of pyqt are no better than what Tobias did

@tobiasgehring
Copy link
Contributor Author

  • According to the documentation the old methods were tagged "obsolete" and thus they were removed from PyQt5.
  • If I figure out how to run the tests, I guess I can. Can you give me a hint?
$ python3 runtests.py 
usage: runtests.py [options] [file_or_dir] [file_or_dir] [...]
runtests.py: error: unrecognized arguments: --cov=qtpy --cov-report=term-missing

So it must be somehow else.

  • apart from the signatures,
    QHeaderView.setSectionResizeMode = QHeaderView.setResizeMode
    doesn't work:
from PyQt4.QtGui import QHeaderView, QApplication
from PyQt4.QtCore import Qt
app = QApplication([])
QHeaderView.setSectionResizeMode = QHeaderView.setResizeMode
a = QHeaderView(Qt.Orientation())
a.setSectionResizeMode(QHeaderView.Interactive)

TypeError: arguments did not match any overloaded call:
  QHeaderView.setResizeMode(QHeaderView.ResizeMode): first argument of unbound method must have type 'QHeaderView'
  QHeaderView.setResizeMode(int, QHeaderView.ResizeMode): first argument of unbound method must have type 'QHeaderView'

a.setSectionResizeMode(a, QHeaderView.Interactive)

a.setResizeMode(QHeaderView.Interactive)

@goanpeca
Copy link
Member

According to the documentation the old methods were tagged "obsolete" and thus they were removed from PyQt5.

Can we also patch these obsolete methods then so that they raise an exception? (What do you think @Nodd?)

If I figure out how to run the tests, I guess I can. Can you give me a hint?

Oops, we dont have any documentation for that.

You need to install
conda install pyqt pytest pytest-cov if using conda... otherwise
pip install pyqt pytest pytest-cov

@tobiasgehring
Copy link
Contributor Author

thanks, the pytest-cov package was missing.

When running the tests now, I get:

ERROR: file not found: qtpy

and then later on a coverage exception saying that it has no data to report.

On 08/19/2016 09:26 AM, Gonzalo Peña-Castellanos wrote:

According to the documentation the old methods were tagged
"obsolete" and thus they were removed from PyQt5.

Can we also patch these obsolete methods then so that they raise an
exception? (What do you think @Nodd https://github.com/nodd?)

If I figure out how to run the tests, I guess I can. Can you give me
a hint?

Oops, we dont have any documentation for that.

You need to install
|conda install pyqt pytest pytest-cov| if using conda... otherwise
|pip install pyqt pytest pytest-cov|


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#65 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/ABV7UgAZpJPBB1u8LDItdBuP4YX-k6lbks5qhVqwgaJpZM4Jn8Rf.

@goanpeca
Copy link
Member

Hi @tobiasgehring

I need more info... what is your system.. what is your env... do you have qtpy installed ? as develop? How ar eyou running the tests...?

@tobiasgehring
Copy link
Contributor Author

I think I figured it out how to use the tests.

Shall I or shall I not make the obsolete functions raise an exception? For PyQt5 it was apparently decided to just not include them.

@goanpeca
Copy link
Member

goanpeca commented Aug 19, 2016

Hey @tobiasgehring glad you sorted it out.

Well the idea of raising the error is, people using qtpy should be encouraged (gently :-p forced) to write code that looks like qt5 code, so if the have qt4 installed and try to access a method that is only available on qt4, it should raise an error, so they know which method to use.

from qtpy.QtWidgets import SomeWidget

# User with qt4 installed
>>> SomeWidget.oldMethodOnlyOnQt4()
Exception: oldMethodOnlyOnQt4 only available on Qt4, use SomeWidget.newMethodOnQt5.

# User with qt5 installed
>>> SomeWidget.newMethodOnQt5()
:-)

@ccordoba12 ccordoba12 added this to the v1.2 milestone Aug 20, 2016
@ccordoba12
Copy link
Member

@tobiasgehring, this now has conflicts with master. Please rebase or merge it with master to fix them.

@goanpeca, this looks good to me. Please merge when you consider it necessary :-)

@goanpeca
Copy link
Member

goanpeca commented Aug 25, 2016

@tobiasgehring this is awesome work :-) thank you very much.

Good to go, merging

@goanpeca goanpeca merged commit 3a61276 into spyder-ide:master Aug 25, 2016
@ccordoba12 ccordoba12 changed the title fixes #64 introduce renamed methods of QHeaderView in PyQt4 and PySide Introduce renamed methods of QHeaderView in PyQt4 and PySide Aug 25, 2016
mottosso added a commit to abstractfactory/Qt.py that referenced this pull request Aug 27, 2016
@mottosso
Copy link

This approach causes issues for me with Python 3.5; for one reason or another, the monkey patch doesn't make it through from asking another class about it.

widget = QtWidgets.QTreeView()
header = widget.header()
header.setSectionResizeMode(qheaderview.Fixed) # AttributeError!

Do you support Python 3.5? Any ideas on what it could be?

@mottosso
Copy link

mottosso commented Aug 27, 2016

Let me give you a full reproducible, I'm unsure of how specific this problem is. Possibly it's related to this particular version of Python, binding and OS.

# Ubuntu 16.04
$ apt-get update
$ apt-get install python3-pyside python3-pip
$ pip3 install qtpy
$ python3
>>> from PySide import QtGui
>>> # setSectionResizeMode is added by qtpy on import
>>> assert not hasattr(QtGui.QHeaderView, "setSectionResizeMode")
>>> import sys
>>> from qtpy import QtWidgets
>>> app = QtWidgets.QApplication(sys.argv)
>>> widget = QtWidgets.QTreeView()
>>> qheaderview = widget.header()
>>> qheaderview.setSectionResizeMode(qheaderview.Fixed)
AttributeError: 'PySide.QtGui.QHeaderView' object has no attribute 'setSectionResizeMode'

The problem only occurs when I first import PySide. If I don't import it, it works. Like magic.

Let me know what you think.

@goanpeca
Copy link
Member

Could you open a new issue for this @mottosso?

@mottosso
Copy link

Hold on, I'm not sure this is an issue yet, mainly testing the waters to see if it's something familiar to you or Tobias.

I noticed this PR wasn't on PyPI yet, so it couldn't have worked in any Python distribution in my example above. This on the other hand does work.

# Ubuntu 16.04
$ apt-get update
$ apt-get install python3-pyside git
$ git clone https://github.com/spyder-ide/qtpy.git
$ export PYTHONPATH=$(pwd)/qtpy
$ python3
>>> from PySide import QtGui
>>> # setSectionResizeMode is added by qtpy on import
>>> assert not hasattr(QtGui.QHeaderView, "setSectionResizeMode")
>>> import sys
>>> from qtpy import QtWidgets
>>> app = QtWidgets.QApplication(sys.argv)
>>> widget = QtWidgets.QTreeView()
>>> qheaderview = widget.header()
>>> qheaderview.setSectionResizeMode(qheaderview.Fixed)

@goanpeca
Copy link
Member

Even if its not yet, I think an issue is a better place to discuss it, not a PR :-)

@titusjan
Copy link

titusjan commented Oct 9, 2016

The exceptions that are thrown in case you use an obsolete method can break things when you mix packages that use qtpy with packages that don't.

Consider an application that uses PyQt4 directly and and imports a package that uses qtpy (with QT_API=pyqt4). As soon as the qtpy.QtWidgets module is imported the obsolete methods of QHeaderView are deleted. Since qtpy.QtWidgets.QHeaderView and PyQt4.QtGui.QHeaderView point to the same class, the PyQt4.QtGui.QHeaderView methods are deleted as well, and this breaks the application.

Therefore, please don't throw exceptions in case an old method is used. That is, please revert commit 722e745.

@goanpeca, you mention above that the purpose of the exceptions is to encourage the users to use the new methods. This may be admirable but I don't think it's necessary. They will notice the old method doesn't exist anymore as soon as they test their code under PyQt5. The exceptions might only notify them earlier but I don't think this is worth the risk of breaking things.

titusjan added a commit to titusjan/objbrowser that referenced this pull request Oct 30, 2016
Included qtpy instead of using a dependency.
See spyder-ide/qtpy#65 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants