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

LazyMethod : Fix running idle callback without a valid Qt Widget #5914

Merged

Conversation

chrisc-lee
Copy link
Contributor

We sometimes have issues when creating UI with PlugValueWidgets that when they are cleaned up after setting a plug value, they may try to still set the value on the widget via the LazyMethod idle callback even though the Qt widget is no longer around. By checking for a valid Qt widget as well here, we avoid instances in which the Gaffer widget is still around but the Qt widget has been cleaned up, which can cause issues when running the callback given that the Qt widget is the underlying widget for the Gaffer widget and one often relies on the other.

This PR is more of a suggestion/discussion as well since, while I believe this approach makes sense, there may be some other things this can affect that I am not too sure about.

This an example of the error we have been seeing related to this issue:

// Error: EventLoop.__qtIdleCallback : Traceback (most recent call last):
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/EventLoop.py", line 284, in __qtIdleCallback
    if not c() :
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/LazyMethod.py", line 163, in __idle
    cls.__doPendingCalls( widget, method )
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/LazyMethod.py", line 176, in __doPendingCalls
    method( widget, *pendingCall.args, **pendingCall.kw )
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/PlugValueWidget.py", line 568, in __callUpdateFromValues
    self._updateFromValues( self._valuesForUpdate( self.getPlugs(), self.__auxiliaryPlugs ), None )
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/StringPlugValueWidget.py", line 79, in _updateFromValues
    if text != self.__textWidget.getText() :
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/TextWidget.py", line 68, in getText
    return self._qtWidget().text()
RuntimeError: Internal C++ object (_LineEdit) already deleted.
 // 
// Error: EventLoop.__qtIdleCallback : Traceback (most recent call last):
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/EventLoop.py", line 284, in __qtIdleCallback
    if not c() :
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/LazyMethod.py", line 163, in __idle
    cls.__doPendingCalls( widget, method )
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/LazyMethod.py", line 176, in __doPendingCalls
    method( widget, *pendingCall.args, **pendingCall.kw )
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/PlugValueWidget.py", line 568, in __callUpdateFromValues
    self._updateFromValues( self._valuesForUpdate( self.getPlugs(), self.__auxiliaryPlugs ), None )
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/LabelPlugValueWidget.py", line 133, in _updateFromValues
    self.__setValueChanged( any( values ) )
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/LabelPlugValueWidget.py", line 163, in __setValueChanged
    if valueChanged == self.__getValueChanged() :
  File "/software/apps/gaffer/1.3.16.3/cent7.x86_64/cortex/10.5/maya/2022/python/GafferUI/LabelPlugValueWidget.py", line 171, in __getValueChanged
    if "gafferValueChanged" not in self.__label._qtWidget().dynamicPropertyNames() :
RuntimeError: Internal C++ object (PySide2.QtWidgets.QLabel) already deleted.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

By checking for a valid Qt widget as well, we avoid instances in which
the Gaffer widget is still around but the the Qt widget as been
cleaned up which can cause issues when running the callback given that
the Qt widget is the underlying widget for the Gaffer widget
@johnhaddon
Copy link
Member

Thanks Chris!

I think the "C++ object already deleted" errors have two broad categories of causes :

  1. Circular dependencies keeping a UI component alive after we think we've disposed of it. A common cause of this is connecting to a signal without using WeakMethod. We want to fix these at source, because regardless of the error, we don't want "zombie" UI components hanging around doing unnecessary work.
  2. More fundamental problems due to the fact that we don't have control over destruction order in Python, so no matter how carefully you manage ownership in your widgets, there is always a chance that Qt has destroyed the C++ part of a Widget before the Python part has gone fully.

I'm always reluctant to resort to _qtObjectIsValid() for things in the first category, as it'd be better to address the root cause. Without knowing more about your specific problem case, it's hard to know if it might fall into this category or not. So it might be worth you doing a bit of further investigation to see if some more specific fix might be available.

But I also think it's clear that your fix also applies to potential problems in the second category, where the widget author has already done everything they can to manage lifetime well. We have a similar check in place for BackgroundMethod already, which provides additional support for doing it in LazyMethod. So I'm going to merge...thanks!

@johnhaddon johnhaddon merged commit 05cbd5d into GafferHQ:1.3_maintenance Jun 24, 2024
4 checks passed
@chrisc-lee
Copy link
Contributor Author

Thanks for the explanation John!

We did do a thorough investigation and we couldn't find any place where we were missing connecting to a signal using WeakMethod and that's where we ended up getting stuck and coming up with this solution. So it is likely the second category in our case.

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