-
Notifications
You must be signed in to change notification settings - Fork 59
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
Data Handlers #238
Data Handlers #238
Conversation
Thanks @tokejepsen, having a proper look at this tonight. |
Adding some additional info for completeness. GoalHandle GUI events - such as the user toggling an Instance on/off. Usage
Now whenever an item is toggled, it would print:
Given that an instance named "myInstance" was toggled. Full ExampleThe below runs in Maya as-is, and produces a toggleable Instance and Plug-in. import pyblish.api
def toggle_handler(item, data_item, new_value, old_value):
print("%s.data[\"%s\"] = %s (was %s)" % (item, data_item, new_value, old_value))
pyblish.api.register_handler(toggle_handler)
class Collector(pyblish.api.Collector):
def process(self, context):
context.create_instance("myInstance")
class Validator(pyblish.api.Validator):
optional = True
pyblish.api.register_plugin(Collector)
pyblish.api.register_plugin(Validator)
import pyblish_maya
pyblish_maya.show() FeedbackThere are a few things that pop out to me.
For (1), I would suggest including an identifier towards the even a handler is meant to listen for. Think of it is the equivalent Alternatively, we could got full-Qt. pyblish.api.item_toggled.connect(my_handler) Maybe it would probably be a better fit for pyblish-qml, seeing as the even is coming from there? pyblish_qml.item_toggled.connect(my_handler) Or maybe it's suited to the core Pyblish, in case the event is triggered from somewhere else, like in headless mode? Would that be interesting? I'd imagine a similar interface for other events too. pyblish.api.register_handler("pluginFailed", handle_failed)
pyblish.api.register_handler("finished", print_report)
pyblish.api.register_handler("validated", handle_validated)
pyblish.api.register_handler("extracted", handle_extracted)
pyblish.api.register_handler("tabChanged", handle_extracted) Where some are GUI-only, some are relevant to headless publishing and some to both. With this interface, the registration towards an even that doesn't exist isn't a big deal, like if there is a handler for the GUI, but the GUI isn't running. Or if an even is handled that isn't present because the version of Pyblish is a few versions old. For (2), I would make it a dictionary, with a key of the function name. For (3), a simple For (4), And finally, as we are talking about events in the context of Pyblish, I'll include this (slightly related) link here. |
This does sound a lot more useful than just GUI related events. GUI events are limited (right now), so expanding it to operate on all events in the Pyblish framework sounds great. Currently when toggling we explicitly call the handler(here > here > here), meaning that we would need to explicitly call the handlers from all events we need/want. I don't know whether there are cleverer solution to this? |
No I think this is precisely how it should work. Only the emitter knows when anything is emitted, and it's good to be explicit about it. It means we could potentially emit the same signal from multiple places, like if an instance got toggled by other means (such as in toggling them via the section label, and if that wasn't already triggering the same function in this case) or to mock or fake signals that aren't necessarily coming from where they say their coming from. Bottom line, this is a clean, lean and efficient implementation. Good work. :) About the other points, what are you thoughts? |
They all make sense, and I'll have a look at them for the next iteration:) |
Re-reading this, about not so much emitting signals (which is as I said, perfect as it is right now) but actually triggering the response; yes, we might want to implement a "headquarters" in which the actual handlers are being called. pyblish.api.emit("itemToggled", {"some": "data"})
self.host.emit("itemToggled", {"some": "data"}) The handlers would then need to be registered where those guys could access them, such that they know what to trigger, which I think is already the case. Edit: Sorry, to clarify; the fact that a signal is emitted from the actual place it is made is good. It makes sense, and shouldn't change. That was what I was commenting on in the previous post. The "headquarters" would simply be adding potential extra methods of Proxy, like this one, together into a single method/function somewhere that could handle the arbitrary "key", e.g. Does that make sense? :S |
Added clarification. |
There are a new PR across pyblish, pyblish-qml and pyblish-rpc. |
👍 Great work. You'll notice some errors on pyblish-rpc, but don't fret it. It's because the tests are using the current version of Pyblish, which doesn't have the new functionality yet. Once we've merged the in the right order, starting with pyblish, those tests will make more sense. |
I've been thinking more about signals. How about we namespace the signals? By namespacing the signals we could have the same signal from pyblish-qml and pyblish without clashing. This would also make it easier to identify where the signal are coming from. |
That's a really cool idea. Are you thinking something along these lines? pyblish.api.emit("pyblish-qml:itemToggled", my_handler) I would probably wait with it, at least until we've seen how many signals we end up with. No need to add namespaces to 5-6 signals, or we'd just end up cluttering. Once we've seen and used these things in action for a while, it will become more obvious whether an additional level of organisation is a good idea. |
Yup, pretty much😀 When we have this merged, I'll be implementing it in our pipeline. |
Just for clarity, are you waiting for me to comment on anything or to merge this, or do you have things left to do? We will at least need to pass the coveralls by adding tests for the new functionality. |
Was waiting on a merge:) Don't know how the coveralls work, so will look at it tomorrow to get it passed. |
Ah! Ok, having a closer look at the code then. :) |
pyblish._registered_callbacks[signal] = [callback] | ||
|
||
|
||
def deregister_callback(callback): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, there's probably no need for both deregister_callback
and deregister_callbacks
. Sure, it will reduce typing, but increase maintenance. Remember, we will need to keep both of these functions forever and ever. The less of that the better!
Additionally, I would make the signature identical to the registration. Just so we don't need to remember two signatures.
deregister_callback("myEvent", my_callback)
Coveralls will run all tests, and see what lines of code were run during that time. What you'll need to do, is look inside of A test should try and break the code, find ways of making the functionality behave in unexpected ways. Like registering a function of the same name multiple times, and deregistering it afterwards. Does it still work? Once you've made a test, run |
- emit now raises an exception - removed deregister_callbacks - doc strings - tests for new code
Passed the coverage! Woop woop:) |
Nice! Having a closer look asap, but from first glance, it looks good! |
callback(**kwargs) | ||
except Exception as e: | ||
traceback.print_exc(e) | ||
raise Exception(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You added a raise
here, that's problematic. It means the loop will stop once it throws the first exception, meaning other callbacks won't get triggered due to one of them being bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was to be able to test the code properly. Couldn't figure out how to trigger coveralls with just printing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. One trick is to throw an exception in the test while building it, like assert False
, just after it prints. That way, it will throw your exception, and show you what got printed before it. Once you've seen that it prints the right things, you can remove the temp exception.
Hey @tokejepsen are you working on this? I've found a few other issues with the code in general that I'd like to address, mainly about using relative imports, that would have a small effect on many place in the code. It would be best to merge this before doing that, or if you haven't started yet, I can send a PR to you. |
Nearly done, just need to address the docstring. |
Cool |
I noticed that when the doctest runs, it will register a callback. That callback will then survive until all testing is done. It can be a good idea to deregister all callbacks in tests that rely on there not being any callbacks. |
If you look at some of the other tests, they have a |
Yeah, just noticed that as well. Thanks for the advice on direction, I would have been lost:) |
- deregister_all_callbacks clears instead of reinstantiate - clearing memory for callback tests, cause of docstring testing in lib.py
Alright, my pen is down:) I'll await your PR. |
Sweet :) Sending shortly. |
Relative imports, refactoring
Success! |
Now to the rest of the repos:) |
Yeah! As pyblish-qml depends on pyblish-rpc, we'll have to handle that first. |
This introduces data handlers, which handle data coming from pybilsh-qml.
The test for this is in pyblish-rpc, so when running pyblish-qml/app.py you see "Toggle Instance" and "ValidateTogglePlugin". When toggling the instance and/or the plugin, a message should get printed to the console similar to:
The data handler is in pyblish-rpc/mocking.py, which triggers functions on the instance and the plugin.
Each data handler runs when data is send from pyblish-qml in pyblish-rpc/service.py. Here I'm converting data from pyblish-qml to classes. The arguments for a data handler is:
As it stands pyblish-qml only send data across on toggling an item; https://github.com/tokejepsen/pyblish-qml/blob/b6f11235fc3d8450af4961dc83f4cd27a827cbcf/pyblish_qml/control.py#L457-L458
Let me know if anything is unclear:)
Currently I don't like that you need to use a static method on a plugin, but we can probably figure out a different workflow.
Overall I think its a bit over complicated, but it does lay down the foundation for saving any data to the scene.