-
Notifications
You must be signed in to change notification settings - Fork 49
Workfiles: Add basic workfiles api #1275
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
base: develop
Are you sure you want to change the base?
Conversation
output_filenames.append(filename) | ||
output_filepaths.append(filepath) | ||
|
||
output_filepath = None |
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.
This really warrants a comment I noticed.
output_filepath = None | |
# Among matching (highest) version numbers | |
# take the last modified file | |
output_filepath = None |
There being dedicated entry points for a save to another context - I like, it may help simplifying some logic that we currently trigger when opening a workfile from another context. However, from the methods I'm not entirely sure how one would identify that the context is actually a different context than the one the user was/is currently operating in? Say I wanted to reimplement that method and do something like "if saving to another context then do X". Actually, I'd love it if the event system would actually emit that too.
I think this is absolutely fine actually. Perfect.
Please do keep the events - they are the easiest way to 'install' any custom studio behavior on save/load or any event that does not require actually changing host integrations. I think actually it'd be welcome if we'd also start emitting custom events for "save to different context", "open from different context" (incl. before/after files)
This is a massive PR, doubles the line count - but lacking the production examples / use cases to actually TEST with and confirm the changes against too. I think we should find a few cases that warrant these changes and try (as for testing the PR) to implement those. Also, I couldn't directly find - API-wise, how to easily set e.g. a workfile "note". Was that API already existing so that could be easily done? Do you have an example snippet perhaps? |
Host is responsible for holding current context, most of them are using default implementation (using env variables). So you can compare current context with context passed in. I don't think, API wise, that you should pass to host it's previous context, the host know that better than whoevery is triggering the method.
True, I focused on moving what we had in workfiles tool. Is there more api functions that comes to you mind? I would say that the host workfiles implementation should be also responsible for change of current context (because it is host's responsibility), which is not true now. The reason why is because that triggers the events -> we would have to move the responsibility to trigger the events to hosts. |
Changelog Description
Moved functionality from workfiles tool and moved a lot of responsibility to host integration.
Additional info
First of all we did not have functions that would mimic what workfiles tool did, which made it difficult to use workfiles in automations (for example). At the same time the functionality in workfiles tool was "generic" which worked for most of DCC integrations, but not all of them -> it was necessary to more the responsibility to host integration.
Hosts responsibility
Host integration now has more targeted functions that are related to what it really does, and is context acknowledged. Instead of just open workfile and save workfile it has "Open workfile in context", "Copy workfile to context", "Copy representation as workfile to context", "List workfiles for context" and "List published workfiles for context". These functions are pre-implemented on
IWorkfileHost
interface to do what was done before this PR, but they can be overriden by host integration if necessary.IWorkfileHost
has pre-implemented methods that should be abstract, but that would break all existing host integrations and most of them would implement the exact same logic.API functions
The logic in workfiles tool did trigger events before/after workfile was saved and on context change. This functionality was kept, BUT based on my (quick) research it was used only by host integrations to run additional functionality on context change/file change -> with the new methods introduced that could be potentially ommited over time. The event triggers are not reliable as it has to go through the utils functions in workfiles API now.
Please write down comments what is good/bad, should be moved elsewhere, should happen in different order etc.
Testing notes: