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

Add QMatrix to QtGui members #330

Merged
merged 5 commits into from
Dec 12, 2019
Merged

Conversation

justinfx
Copy link
Contributor

Adds the missing QtGui.QMatrix member which should be present across all Qt4/5 bindings

@CLAassistant
Copy link

CLAassistant commented Nov 26, 2019

CLA assistant check
All committers have signed the CLA.

@justinfx
Copy link
Contributor Author

@mottosso looks like QMatrix was actually deprecated by PyQt5 in favour of QTransform. Should QMatrix be mapped to QMatrix3x3 for PyQt5, as per the Qt docs?

@justinfx
Copy link
Contributor Author

Hmm. This may not be something that can be preserved across all versions. The QMatrixNxM classes don't seem to be the same interface as QMatrix, so there isn't really something to return. For instance, I used to be able to construct a QMatrix with no arguments and then rotate it. This is no longer possible with the fixed-size matrix classes.

@mottosso
Copy link
Owner

Hmm, I'm unsure.

The important bit is that any code written with Qt.py works identically across all versions. If QMatrix can't cope with that, then it probably shouldn't be used with Qt.py.

Is QTransform a viable alternative, for e.g. your use case, Justin?

@justinfx
Copy link
Contributor Author

Yep, QTransform works for me. It just becomes a case where existing code would crash if it were using QMatrix so the user would need to probably see a caveat added. Or maybe Qt.py could have some kind of NotImplemented base type that it could put into the list so that when a user's code tries to construct something like QMatrix it could crash with a Qt.py specific error?

@mottosso
Copy link
Owner

Or maybe Qt.py could have some kind of NotImplemented base type that it could put into the list so that when a user's code tries to construct something like QMatrix it could crash with a Qt.py specific error?

Isn't the non-existence error doing exactly that though? xD

Are you thinking something along the lines of, if the user tries to access QtGui.QMatrix, it'd replace the normal "Does not exist" message with a "Hey, this wouldn't work with PyQt5, try QTransform instead, yo"?

I think that should be possible, given we're the ones constructing the QtGui "module". It could be replaced with a class that could intercept calls via `getattr' or the like.. but I wonder whether it may cause issues elsewhere.. and whether it's worth it. Not sure, what do you think?

@mottosso
Copy link
Owner

Re-reading your message, you were thinking of actually providing a "mock" QMatrix object that could throw an error on instantiation? That could work! Haven't thought about that before, or seen it elsewhere. Wouldn't have thought throwing errors during construction was wise or expected. Is that what you meant?

@justinfx
Copy link
Contributor Author

Normally I would not suggest raising exceptions in constructors, sure. But given the nature of Qt.py as a dynamic API layer, it seems somewhat suitable. The getattr approach could also be completely acceptable as well. I think what I am after is more just the end effect of a user seeing more specifically why QMatrix isn't available after switching to the compatibility layer. If there were some type of mapping where you could keep adding entries, it might make it easier for users to know whats going on?

Also the closest example of really throwing exceptions from constructors is if one were using abc.Meta classes to create an abstract base class. If someone tried to construct the abstract class directly they would get an exception. So my thought here was to just generate the same placeholder for deprecated classes.

@mottosso
Copy link
Owner

If there were some type of mapping where you could keep adding entries, it might make it easier for users to know whats going on?

I think that's a good idea, thanks for sharing. If you're interested in getting that started, this PR seems as good a place as any.

@justinfx
Copy link
Contributor Author

I've got two implementations that I could submit, but I wanted to get some feedback on which one is preferable.

Option 1: RuntimeWarning on attribute access of 'missing' member

>>> hasattr(Qt.QtGui, 'QMatrix')
RuntimeWarning: Qt.QtGui.QMatrix is not a common object across PySide2 and the other Qt bindings. It is not included as a common member in the Qt.py layer.
False

>>> klass = Qt.QtGui.QMatrix
RuntimeWarning: Qt.QtGui.QMatrix is not a common object across PySide2 and the other Qt bindings. It is not included as a common member in the Qt.py layer.
...
AttributeError: module 'Qt.QtGui' has no attribute 'QMatrix'

Option 2: MissingMember placeholder class that handles various access

>>> QtGui.QMatrix
<MissingMember: QtGui.QMatrix>

>>> QtGui.QMatrix()
NotImplementedError: QtGui.QMatrix is not a common object across PySide2 and the other Qt bindings. It is not included as a common member in the Qt.py layer.

>>> QtGui.QMatrix.foo
NotImplementedError: QtGui.QMatrix is not a common object across PySide2 and the other Qt bindings. It is not included as a common member in the Qt.py layer.

@mottosso
Copy link
Owner

mottosso commented Dec 2, 2019

Thanks for this, and sorry for the delay.

Based on having tinkered with overriding the behavior of a module in the past (by swapping it out for a class at import, which is what I suspect you've got in mind here) and run into various subtle issues - like from x import y no longer working, I would think Option 2 is preferrable.

In particular without any __getattr__ override, and just putting something like this in place of each missing member.

# Qt.py

class _QMatrix():
  def __init__(self, *args, **kwargs):
    raise RuntimeError("Message here")

QtGui.QMatrix = _QMatrix

Perhaps automated somehow, such that we could add to a dictionary of known-but-missing members.

excluded = {
  # Member  Reason
  "QMatrix", "Because PyQt5 has deprecated it",
}

I wouldn't catch invalid attribute access, but maybe preventing instantiation is enough for most/all cases?

Qt.py Outdated Show resolved Hide resolved
@justinfx
Copy link
Contributor Author

justinfx commented Dec 2, 2019

Based on having tinkered with overriding the behavior of a module in the past (by swapping it out for a class at import, which is what I suspect you've got in mind here) and run into various subtle issues - like from x import y no longer working, I would think Option 2 is preferrable.

Yes this is the approach I had written for option 1, where it derives from ModuleType and implements __getattr__. But it only would have been called during the case of missing attributes. And it's only role was to log the warning and preserve the original attribute access behaviour. We can skip this implementation.

In particular without any __getattr__ override, and just putting something like this in place of each missing member.

# Qt.py

class _QMatrix():
  def __init__(self, *args, **kwargs):
    raise RuntimeError("Message here")

QtGui.QMatrix = _QMatrix

Perhaps automated somehow, such that we could add to a dictionary of known-but-missing members.

excluded = {
  # Member  Reason
  "QMatrix", "Because PyQt5 has deprecated it",
}

I wouldn't catch invalid attribute access, but maybe preventing instantiation is enough for most/all cases?

This is effectively what I had written and just pushed up. There is a missing members mapping that can be populated with names and descriptions. Then there is a MissingMember class that is automatically generated during _install.

@mottosso
Copy link
Owner

I think this looks good, thanks @justinfx!

@mottosso mottosso merged commit 62dc86b into mottosso:master Dec 12, 2019
@justinfx justinfx deleted the justinfx-QMatrix branch July 10, 2020 02:19
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

4 participants