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

PyQt enums patch #352

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

PyQt enums patch #352

wants to merge 5 commits into from

Conversation

j0yu
Copy link

@j0yu j0yu commented Nov 18, 2020

Patch PyQt* Qt namespace enums to behave like PySide*.

i.e. Fixes ❎ in QtCore.Qt

Attributes PyQt* PySide*
Qt.Checked value ✔️ ✔️
Qt.CheckState class ✔️ ✔️
Qt.CheckState.Checked value ✔️
Qt.CheckState.values dict ✔️

UPDATE Clarification: Above is just 1 example, the intention is to patch for all flags/enums in the Qt Namespace

Would this fall under a similar re-name/re-expose pyqtSignal as Signal? Just curious if this will actually fall under the umbrella of No wrappers

@CLAassistant
Copy link

CLAassistant commented Nov 18, 2020

CLA assistant check
All committers have signed the CLA.

tests.py Outdated Show resolved Hide resolved
Qt.py Show resolved Hide resolved
Qt.py Outdated Show resolved Hide resolved
Copy link
Owner

@mottosso mottosso left a comment

Choose a reason for hiding this comment

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

Thanks for this, good job with the little table there demonstrating the changes. Nice touch. 👍

However, this is too magical/automatic with too much formatting going on to understand what's happening. See comments in code.

If the goal is adding these two members, it would be much preferred just adding them directly, although it can't modify the original class so it'll need to go in QtCompat

# Bad
from PyQt4 import QtCore
QtCore.Qt.CheckState.Checked

# How did you get there?
# Ah.. someone has imported Qt.py somewhere
# Good
from Qt import QtCompat
QtCompat.CheckState.Checked

Comment on lines +1809 to +1813
qt_attrs = {
name: getattr(Qt.QtCore.Qt, name)
for name in dir(Qt.QtCore.Qt)
if not name.startswith("_")
}
Copy link
Owner

Choose a reason for hiding this comment

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

This one for example could (and likely is) pick up enums other than the 2 being fixed by this PR.

Copy link
Author

Choose a reason for hiding this comment

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

yeah, it was the original intention to be able to capture all cases found, not just CheckState only. The description in the PR could probably do with a better wording to highlight it.

tests.py Outdated
Comment on lines 1174 to 1176
values_path = "QtCore.Qt.{0}.{1}".format(class_name, "values")
assert hasattr(qt_class, "values"), (
"{0} should exist, but is missing".format(values_path))
Copy link
Owner

Choose a reason for hiding this comment

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

This is very hard to read

Copy link
Author

Choose a reason for hiding this comment

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

I switched to %s notations in newest commits

@j0yu
Copy link
Author

j0yu commented Nov 18, 2020

Ah.. someone has imported Qt.py somewhere

Great point, I think the QtCompat way is more tidy. But it will be great for the future if there is a wizardry to retain the QtCore.Qt.CheckState.Checked syntax without ruining the original PyQt5.QtCore module

@j0yu
Copy link
Author

j0yu commented Nov 18, 2020

0fa580a: it's starting to gets messy imo, but I want to get an opinion on this.

QtCompat

image

As per #352 (comment), the above implementation will work, but it does add the enums directly and if not careful, can conflict with existing attributes already inside QtCompat

EnumFlags class

To replicate the PySide* behaviour in QtCompat, it requires a class EnumFlags. This might go against simplicity and is effectively a new feature as it is a construct that's made in Qt.py to mock the PySide* behaviour.


Bottom line, it works, but at what cost?

@mottosso
Copy link
Owner

Thanks for the update, but this is still to magical. Or implicit, is the word.

Let's just make it real explicit instead, e.g. here:

Qt.py/Qt.py

Lines 1547 to 1553 in 8a8953b

if hasattr(Qt, "_QtCore"):
Qt.__binding_version__ = Qt._QtCore.PYQT_VERSION_STR
Qt.__qt_version__ = Qt._QtCore.QT_VERSION_STR
Qt.QtCompat.dataChanged = (
lambda self, topleft, bottomright, roles=None:
self.dataChanged.emit(topleft, bottomright, roles or [])
)

One could put..

CheckState = type("CheckState", (object,), {})
CheckState.Checked = Qt.Checked
CheckState.values = ...  # etc

QtCompat.CheckState = CheckState
QtCompat.Enum2 = Qt.Enum2
QtCompat.Enum3 = Qt.Enum3
...  # etc

And so forth. That way, we control which enums are actually patched, and also document what they are. That implicit method is not only hard to read, but doesn't tell you which enums are patched. It also runs the risk of patching different enums on different builds of PySide2/PyQt5/etc.

@mottosso
Copy link
Owner

Also keep an eye on the tests, they're unhappy about something.

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.

4 participants