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

Reparenting windows fails if the original parent is dead. #1064

Closed
andrewkaufman opened this issue Oct 28, 2014 · 6 comments
Closed

Reparenting windows fails if the original parent is dead. #1064

andrewkaufman opened this issue Oct 28, 2014 · 6 comments
Assignees
Labels
bug Issues representing bugs

Comments

@andrewkaufman
Copy link
Contributor

To reproduce, open a script, launch the DispatcherWindow, close it, open a new script, close the first ScriptWindow, then try to launch the DispatcherWindow from your new ScriptWindow.

Traceback (most recent call last):
  File "/home/andrewk/apps/gaffer/0.3.0.0dev/cent6.x86_64/cortex/9/gaffer/python/Gaffer/WeakMethod.py", line 67, in __call__
    return m( *args, **kwArgs )
  File "/home/andrewk/apps/gaffer/0.3.0.0dev/cent6.x86_64/cortex/9/gaffer/python/GafferUI/Button.py", line 138, in __clicked
    self.clickedSignal()( self )
  File "/home/andrewk/apps/gaffer/0.3.0.0dev/cent6.x86_64/cortex/9/gaffer/python/Gaffer/WeakMethod.py", line 67, in __call__
    return m( *args, **kwArgs )
  File "/home/andrewk/apps/gaffer/0.3.0.0dev/cent6.x86_64/cortex/9/gaffer/python/GafferUI/DispatcherUI.py", line 239, in __executeClicked
    _showDispatcherWindow( [ self.getPlug().node() ] )
  File "/home/andrewk/apps/gaffer/0.3.0.0dev/cent6.x86_64/cortex/9/gaffer/python/GafferUI/DispatcherUI.py", line 450, in _showDispatcherWindow
    window = __dispatcherWindow( nodes[0].scriptNode() )
  File "/home/andrewk/apps/gaffer/0.3.0.0dev/cent6.x86_64/cortex/9/gaffer/python/GafferUI/DispatcherUI.py", line 444, in __dispatcherWindow
    scriptWindow.addChildWindow( window )
  File "/home/andrewk/apps/gaffer/0.3.0.0dev/cent6.x86_64/cortex/9/gaffer/python/GafferUI/Window.py", line 171, in addChildWindow
    oldParent = childWindow.parent()
  File "/home/andrewk/apps/gaffer/0.3.0.0dev/cent6.x86_64/cortex/9/gaffer/python/GafferUI/Widget.py", line 290, in parent
    parentWidget = q.parentWidget()
RuntimeError: underlying C/C++ object has been deleted
@andrewkaufman andrewkaufman added this to the Usability in Caribou milestone Oct 28, 2014
@johnhaddon johnhaddon added the bug Issues representing bugs label Oct 28, 2014
@johnhaddon
Copy link
Member

This is a general problem with the QWidget wrapping mechanism - if you want to keep a child after a parent has died, you need to remove it from the parent before the parent dies. When the parent dies on the C++ side, it deletes all the children recursively. If you remove the child first, the python wrappers reassume ownership of the C++ child instance, and when the parent dies it doesn't delete the child, because it's not a child any more. Qt doesn't use smart pointers for shared ownership like we do in Gaffer/Cortex, so this behaviour is counterintuitive compared to the rest of our APIs, and a case where our UI abstraction on top of Qt is leaky.

It might be worth investigating whether or not we can remove the children automatically from a QObject::destroyed slot, but if that doesn't work then I'm out of ideas for the general case.

For this specific case, the preferences window in ApplicationMenu.py provides a working example - it only keeps weak references to the preferences window, so if the window is destroyed by a parent ScriptWindow, it will get created again.

@andrewkaufman
Copy link
Contributor Author

Actually, is there a reason we need to parent the DispatcherWindow to the ScriptWindow in the first place? Now that it's kept alive by the application, it seems like we could avoid this whole problem by not parenting it at all. I just tried that locally, and it still pops up on top on the ScriptWindow when you open it, and this error goes away... what do you think?

@johnhaddon
Copy link
Member

Does it stay on top if you go fullscreen? If it does I'll try the same on OS X to see if it works here too, but I'm pretty sure parenting was necessary to keep things on top in all scenarios.

@andrewkaufman
Copy link
Contributor Author

Yeah, even in full screen it goes on top

@johnhaddon
Copy link
Member

Hmm - not on OS X, not even in non-fullscreen mode. I thought there was a reason things were the way they are.

@andrewkaufman andrewkaufman self-assigned this Oct 28, 2014
@andrewkaufman
Copy link
Contributor Author

Ok, I've got a commit up in #1066 which uses weakref for both the DispatcherWindow and the new LocalJobsWindow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues representing bugs
Projects
None yet
Development

No branches or pull requests

2 participants