Skip to content

Fix PySide6 6.2.2 breaking change#2132

Merged
j9ac9k merged 1 commit intopyqtgraph:masterfrom
j9ac9k:fix-pyside_622
Dec 9, 2021
Merged

Fix PySide6 6.2.2 breaking change#2132
j9ac9k merged 1 commit intopyqtgraph:masterfrom
j9ac9k:fix-pyside_622

Conversation

@j9ac9k
Copy link
Copy Markdown
Member

@j9ac9k j9ac9k commented Dec 4, 2021

This fixes an issue that came up in PySide6 6.2.2 where calls to GraphicsObject.parentChanged were passed to QGraphicsObject.parentChanged instead of GraphicsItem.parentChanged

After looking a bit more, the issue seems to only occur when there is a conflict between a method and signal, but when both inherited objects have the same method name, the correct one is used... and there does not appear to be any other conflicts with respect to signal names and method names.

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Dec 4, 2021

I'm not convinced that this is the ideal fix; there is something not write to me with having the signal/method conflict via multiple-inheritance.

Given that GraphicsItem.parentChanged() just called GraphicsItem._updateView() should we instead do something like

# in GraphicsObject.__init__
self.parentChanged.connect(self._updateView)

And get rid of GraphicsView.parentChanged method altogether.

Thoughts?

EDIT: On second thought, that would be a whole new behavior change; instead perhaps we should change the diff to call ._updateView() instead of .parentChanged() (and delete the .parentChanged() method altogether)

@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Dec 5, 2021

So the following would be the MWE. Python rules say that M is searched before Q.
The puzzling thing is: how does PySide6 6.2.2 manage to subvert the Python resolution machinery?

from PySide6 import QtCore

class Q(QtCore.QObject):
    signal = QtCore.Signal()

    def method(self):
        print('Q::method')

class M:
    def signal(self):
        print('M::signal')

    def method(self):
        print('M::method')

class C(M, Q):
    def __init__(self):
        Q.__init__(self)
        M.__init__(self)

c = C()
c.method()    # okay
c.signal()    # problem on PySide6 6.2.2

@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Dec 6, 2021

Documenting here that calling GraphicsItem.parentChanged(self) instead of self.parentChanged() prevents ScaleBar::parentChanged method from getting called.

@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Dec 7, 2021

I have a workaround that will successfully call ScaleBar.parentChanged.
Change in GraphicsObject.py:

           self.parentChanged()

to:

            import types
            if isinstance(self.parentChanged, types.MethodType):
                self.parentChanged()
            else:
                # workaround PySide6 6.2.2
                getattr(self.__class__, 'parentChanged')(self)

The else branch actually works for all bindings, but given that this appears to be a bug of PySide6 6.2.2 which may perhaps get fixed in the future, writing it this way documents the why.

@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Dec 8, 2021

Holding off on merging this until I can submit a bug-report with the pyside folks so I can reference the specific issue number; unfortunately I can't set 6.2.2 as the "affected version" for the submission process...hopefully I can do that tomorrow.

This fixes an issue that came up in PySide6 6.2.2 where calls to
GraphicsObject.parentChanged were passed to
QGraphicsObject.parentChanged instead of GraphicsItem.parentChanged. In
addition, this fix handles the case of ScaleBar.parentChanged as well,
where specifying the specific parent class was insufficient.

Thanks to @pijyoi for identifying this fix.
@j9ac9k
Copy link
Copy Markdown
Member Author

j9ac9k commented Dec 8, 2021

Created https://bugreports.qt.io/browse/PYSIDE-1730 with @pijyoi 's minimum reproducible example, added a comment to this issue, think this is ready for merging now 👍🏻

@j9ac9k j9ac9k merged commit e790b02 into pyqtgraph:master Dec 9, 2021
@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Dec 11, 2021

The following is a better test of any eventual fix in PySide6 and reflects pyqtgraph's usage. (ScaleBar)
In particular, on PySide6 6.2.2, print(d.signal) yields <PySide6.QtCore.SignalInstance object at ...>, whereas on PySide6 <= 6.2.1 and other bindings, it yields `<bound method D.signal of ...>'

It seems from the current discussion on https://bugreports.qt.io/browse/PYSIDE-1730 that a fixed version might still yield <PySide6.QtCore.SignalInstance object at ...>.

from PySide6 import QtCore

class Q(QtCore.QObject):
    signal = QtCore.Signal()

class M:
    def signal(self):
        # default implementation
        print("M::signal")

class C(M, Q):
    pass

class D(C):
    def signal(self):
        # overridden implementation
        print("D::signal")

c = C()
print(getattr(c.__class__, 'signal'))
print(c.signal)

d = D()
print(getattr(d.__class__, 'signal'))
print(d.signal)

c.signal()    # expect M::signal
d.signal()    # expect D::signal

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.

2 participants