search

pyblish / pyblish-qml

Pyblish QML frontend for Maya 2013+, Houdini 11+, Nuke 8+ and more
GNU Lesser General Public License v3.0
114 stars 44 forks source link

Fix KeyError when calling pyblish_qml.show() more than once #365

Open jboisvertrodeofx opened 4 years ago

jboisvertrodeofx commented 4 years ago

It seems that since pyblish-qml-1.9.6, a KeyError is raised when calling pyblish_qml.show() more than once if pyblish.api.deregister_all_callbacks() is called before the second call.

This is the stack trace:

# Traceback (most recent call last):
#   File "<maya console>", line 148, in <module>
#   File "/softwareLocal/rez_python_envs/shared/2.7.16/0xb578cca6b312bb8d/lib/python2.7/site-packages/pyblish_qml/__init__.py", line 21, in show
#     return host.show(parent, targets, modal, auto_publish, auto_validate)
#   File "/softwareLocal/rez_python_envs/shared/2.7.16/0xb578cca6b312bb8d/lib/python2.7/site-packages/pyblish_qml/host.py", line 107, in show
#     install(modal)
#   File "/softwareLocal/rez_python_envs/shared/2.7.16/0xb578cca6b312bb8d/lib/python2.7/site-packages/pyblish_qml/host.py", line 63, in install
#     uninstall()
#   File "/softwareLocal/rez_python_envs/shared/2.7.16/0xb578cca6b312bb8d/lib/python2.7/site-packages/pyblish_qml/host.py", line 75, in uninstall
#     uninstall_callbacks()
#   File "/softwareLocal/rez_python_envs/shared/2.7.16/0xb578cca6b312bb8d/lib/python2.7/site-packages/pyblish_qml/host.py", line 194, in uninstall_callbacks
#     pyblish.api.deregister_callback("instanceToggled", _toggle_instance)
#   File "/softwareLocal/rez_python_envs/shared/2.7.16/0xb578cca6b312bb8d/lib/python2.7/site-packages/pyblish/plugin.py", line 910, in deregister_callback
#     _registered_callbacks[signal].remove(callback)
# KeyError: 'instanceToggled' #

I believe this is this the commit that exposed the issue: https://github.com/pyblish/pyblish-qml/commit/e4dacaa075f3e535fe8a706b7a1808ef1defcbde

I've seen in other places of the code that we deal with this case with a simple try/except, so I've the done the same in my fix here.

jboisvertrodeofx commented 4 years ago

Code sample to replicate the issue:

import pyblish.api
import pyblish_qml

server = pyblish_qml.show()
server.wait()

pyblish.api.deregister_all_callbacks()
server = pyblish_qml.show()
server.wait()
mottosso commented 4 years ago

Thanks for this @jboisvertrodeofx, and for hunting down the original cause. And for the replicable. But this doesn't seem like the right way to go. :/ I think you've identified the actual cause, install() being called multiple times to force an event handler to get installed.

Install was initially meant to be called by the user. It's designed as a one-off. It got rather consistent that you always call this whenever you show the GUI, so we made it automatic. But now that automation has become a problem, and the solution should be to to silence the warnings.

This isn't what you signed up for, but if you are able, I would much rather we revert that commit you found, for install() to conveniently and automatically get called if it hasn't already been called, and put the install of the event handler into the show() function explicitly. That's the only one that should get uninstalled when the GUI goes dormant, because it's a performance hog. It's an optimisation.

Let me know what you think.

jboisvertrodeofx commented 4 years ago

Hey @mottosso thanks for the quick feedback!

Just to see if I understanding correctly, you'd add a call to install_callbacks directly in the host.show function, but you'd remove the call to install_callbacks from host.install, along with reverting the 1.9.6 commit that removed the check to see if host.install was previously called?

If that's the case, I think it could somewhat work, but one small issue I would see is that we'd have to add a check to see if the callbacks were previously installed before re-installing them, because pyblish.api.register_callback can append the same callback multiple times and it seems like we wouldn't be calling host.uninstall anymore.

mottosso commented 4 years ago

Yes, I think so.

  1. On show()
  2. Install event handler

Along with..

  1. On installing the event handler..
  2. First uninstall any already installed

And finally..

  1. On close of window..
  2. Uninstall event handler

That way, it'll be guaranteed to be installed whenever the GUI is visible, and most likely be uninstalled whenever it's closed. The exception being crashes or such, when the close event is lost. But that's a fine compromise, as it would be uninstalled the next time around, and ultimately is just an optimisation that won't harm the overall function of the DCC.

jboisvertrodeofx commented 4 years ago

I just committed a WIP, so it will be easier to explain.

There seems to be a side effect to reverting the https://github.com/pyblish/pyblish-qml/commit/e4dacaa075f3e535fe8a706b7a1808ef1defcbde commit, where QtHost.uninstall() will then only be called the first time host.show() is called, because QtHost.state seems diverge from the standalone _state at some point, and so the pyblishQmlClose and pyblishQmlCloseForced callbacks are not added the subsequent times.

I was planning on re-using QtHost.uninstall() to call uninstall_callbacks(), but maybe you had a different idea in mind?