Fix #514

[tokejepsen]

What's changed?

• Each tool gets destroyed on close.
• Each tool gets raised if there is an existing window.
• All tools' windows gets activated.

[mottosso]

Say what? :O How can all tools share a single lib.existing_app? Is app referring to their window instances? Also existing_app isn't the name of a boolean, better make it has_existing_app. I expected an instance of the QApplication there, is that not what it should refer to?

[tokejepsen]

How can all tools share a single lib.existing_app?

They are all sharing the same QApplication, but the existing code fails when trying to show the tools in a standalone QApplication. See #514 for more details.
It seems like the window instance is kept on the module level, but the QApplication gets deleted so we end up with a segfault when showing the tool for the second time.

Also existing_app isn't the name of a boolean, better make it has_existing_app.

Fair enough. Can change that.

[mottosso]

Is this case tested? What do we expect to happen? If there isn't a QApplication, then we probably can't create a window.

[tokejepsen]

I've tested this in Nuke/Maya and standalone (Photoshop).

If there isn't a QApplication the with lib.application() ensure there is a QApplication.

[mottosso]

But isn't lib.application() happening first? :O How can existing_app be False, if application() ensures it is true?

[mottosso]

If this is true, then the instance we store in module.window will be invalidated. It is deliberately not deleted such that it can be reused later. The window is preserved to also preserve e.g. user selection and focus within the app. I think we want those things?

[tokejepsen]

Good point. Will remove that.

[tokejepsen]

Actually looking closer, it seems like we are deleting the old window anyways: https://github.com/getavalon/core/blob/master/avalon/tools/contextmanager/app.py#L218-L222

[davidlatwe]

Currently only Loader's window get preserved, and I vote for keeping them :P

[mottosso]

This PR seems to conflate a few things.

#514 seems to be about QApplication being garbage collected, then why not just..

avalon/blender/pipeline.py

module = sys.modules[__name__]

def install()
  module.qapp = QApplication.instance()
  # Or make one, and store it here.

That would preserve it for the lifetime of the inner Python interpreter.

[BigRoy]

I agree that the fixes presented here seem like an odd "fix" to the described error, as if we didn't get to the bottom as to what exactly is causing the issue.

Did you try the fix mentioned at the bottom here:

app.setQuitOnLastWindowClosed(False)

If it really is an issue with the QApplication somehow getting garbage collected then Marcus' comment would fix that. However, as I believe the QApplication should stick around as a singleton in the module itself to me it still feels like the Window itself got "destroyed" as it was the last window to close or the QApplication stopped processing. If that's the case the result would be similar to what Marcus described here. Maybe in your odd case then @tokejepsen the QApplication.instance() does still exist so it would enter the else statement here. As such it would assume it has a valid QApplication currently yet it's not currently executing, and if that's the case a new call to exec_() or alike would be required. But I have never run into this particular case so we'll need to figure out why it's happening in this instance and see if we can have a simple reproducable example in e.g. Python standalone.

So I've done just that and tried to reproduce it there.

from PySide import QtGui

# Create an instance
app = QtGui.QApplication([])

# Create a QPushButton
button = QtGui.QPushButton("test")
button.show()

# As expected here the QPushButton pop ups but remains in an "white"
# or non-refreshing state as the QApplication is not running.
app.exec_()

# Now close QPushButton (press X on the window)
# ...

# Note how the QApplication instance still exists but it is not executing anymore
print QtGui.QApplication.instance()

# Create a QPushButton
button = QtGui.QPushButton("test")
button.show()

# Note how again, it's in the non-refreshing white state, until we execute the QApplication
QtGui.QApplication.instance().exec_()

# Et voila...

If there's a way that we can check whether the QApplication is currently executing at that point in time we can then trigger another exec_() in that else statement if it is not running currently. If querying the current active state of the QApplication is non-trivial then the app.setQuitOnLastWindowClosed(False) I believe would be your simplest bet for those Python process that you will keep open but yet where you'll often close the last open Qt windows.

[mottosso]

Note how the QApplication instance still exists but it is not executing anymore

Oh wow, that is a good find. I had no idea it would do that.

If there's a way that we can check whether the QApplication is currently executing at that point in time we can then trigger another exec_() in that else statement if it is not running currently.

The simple solution would be to just call it again, regardless. If it was running, then it would block and stop you from running it again, and if it wasn't blocking, then having it block should be normal.

[tokejepsen]

So I've done just that and tried to reproduce it there.

I cant seem to replicate the behaviour you are seeing?

[BigRoy]

@tokejepsen so you are running all commands at the same time. As such, after closing the first one it will instantly run through the second exec_() call.

Try this in a Python process and run them step by step, like the comments somewhat hinted. :)

Or to show the hanging UI... remove the last exec_() call.

[tokejepsen]

@BigRoy thanks, yeah just realized you probably meant going through the python prompt. Seeing the same behaviour now.

[tokejepsen]

Ok, I think I have a simple fix for this:

@contextlib.contextmanager
def application():
    self.app = QtWidgets.QApplication.instance()

    if self.app:
        print("Using existing QApplication..")
        yield self.app
    else:
        print("Starting new QApplication..")
        self.app = QtWidgets.QApplication(sys.argv)
        yield self.app

    self.app.exec_()

Before:

At the end you can see that the python process exits without any errors, which is what happens when the segfault occurs.

After:

At the end you can see that the python process is still running, and I can show another contextmanager with the existing QApplication.

If we like this fix, I will revert the changes and update with this code change instead.

[tokejepsen]

Any takers on this fix suggested above?

[davidlatwe]

Ok, I think I have a simple fix for this

I do like that fix. :)

[BigRoy]

Sorry, totally missed your comments.

I somehow expected that wouldn't fix it. But if it fixes all use cased by moving one line of code let's set up a PR and give it a couple of days of testing. Should we close this PR and set a new clean one? Or somehow squash the commits to solely that change?

Or were there other changes that are required?

[mottosso]

Seems appropriate. Does exec() not cause issues when e.g. Maya already has a QApplication?

[davidlatwe]

Does exec() not cause issues when e.g. Maya already has a QApplication?

It only return -1 and nothing else happens.