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 #360 - quit Pyblish QML when Blender's operator is cancelled #361

Closed jasperges closed 4 years ago

jasperges commented 4 years ago

This resolves https://github.com/pyblish/pyblish-qml/issues/360

mottosso commented 4 years ago

Nice one, but do still keep the reference to the server so it's clear what "quit()" refers to. It's a little too similar to exit() which would kill the Python interpreter.

jasperges commented 4 years ago

The reason I just call quit() is twofold:

  1. The example code you shared in #360, didn't work, because it calls quit() on the server and not on the proxy.
  2. While looking what went wrong with 1, I discovered the quit() function in host.py. That function does exactly what I need via the proxy_call wrapper.

So I'm not sure how you would like me to change this. By using the proxy_call wrapper there is an extra safety check to see if the server is actually still alive. Even though the chances are very slim (or even non-existent) that the server is gone when the operator is not cancelled yet, I think it's a good idea to have this check. To keep a reference to the proxy I would have to duplicate this code, which seems like a bad idea.

mottosso commented 4 years ago

The example code you shared in #360, didn't work, because it calls quit() on the server and not on the proxy.

Eeek. The terminology here isn't great; the "proxy" wraps the "server" with commands, like quit. Server in this case is Pyblish QML, where Blender is the client.

Looks like I left out a line from my example.

https://github.com/pyblish/pyblish-qml/blob/18826c8cad919b61e2c5332055242f85db814fd6/pyblish_qml/host.py#L116

With that, you should be able to call on proxy.quit().

While looking what went wrong with 1, I discovered the quit() function in host.py

Double-eeek! I had also not seen this function. It's a context manager for a wrapper of a wrapper for the server. The proxy should be handling where the server has been killer or is missing.

Let's stick with calling proxy.quit() directly, so that we can migrate away from those free-floating functions that have no business being in host.py (but should be in ipc/server.py).

jasperges commented 4 years ago

Okay, thanks for clarifying. I completely agree that proxy.quit() is better. It makes it clear what has to quit.

I have another question: api.current_server() calls host.current_server(). Is it okay to directly call current_server() or do you prefer to do it via api, so things can be moved around without breaking this code? And is it okay if I do from . import api?

So many questions for such a simple thing... :sweat_smile:

mottosso commented 4 years ago

The API is only for external use. Internal modules shouldn't call it, unless it's an even higher-level of abstraction than the API (like e.g. pyblish.util).

jasperges commented 4 years ago

Sorry, now I'm confused. Should I use api.current_server() or just current_server()?

mottosso commented 4 years ago

I would do what other functions in the module do, unless there's a reason not to.

https://github.com/pyblish/pyblish-qml/blob/326c68d8a13506042b5f7afda4205ab77b641520/pyblish_qml/host.py#L115-L116

jasperges commented 4 years ago

To be on the safe side, I use:

server = _state.get("currentServer")
if server:
    proxy = ipc.server.Proxy(server)
    proxy.quit()
mottosso commented 4 years ago

Looks good, unsure of why the Docker image fails for the tests.. hopefully it's temporary and doesn't involve this change. Thanks @jasperges