-
-
Notifications
You must be signed in to change notification settings - Fork 655
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
Extending video recording extension #3282
Comments
I'm very open to modifying how Extensions are configured! I'm not sure inheriting the class is the right solution for maintainability. We've never really thought of plugins or extensions as inheriting and modifying other plugins or extensions. My concern is that it creates a set of dependencies that we didn't configure the project to maintain. @bjoluc has good instincts on this kind of thing, so hoping he will chime in. I'm happy to be convinced that I'm wrong by either of you! Is there a way that the |
Thanks for your input Josh! I will wait to hear more from @bjoluc and @okaycj.
Yes the extension would need to be modified to take a user-defined data handler function (probably a good idea for all jsPsych users anyway!). We were also going to add some default behavior and optional parameters to our CHS version - things like controlling what happens while the recording is first starting up and while the upload finishes (error handling, time limits, optional content to show to the participant). I don't think this is possible with the current recording extension. So we would probably also need |
If we added all the hooks that are necessary would that solve the problem, or would it still create a difficult-to-manage situation because you have to inject the code for those hooks into the user's experiments? |
Good point. I don't think this is an issue though: Extension instances are publicly exposed via the
I don't think that's a good idea as it would publicly expose the internal implementation. But adding additional overridable methods along the lines of
Definitely, yes. As long as you either call |
Thanks very much for your quick feedback @jodeleeuw and @bjoluc! I'm going to wait to chat with @okaycj about this and get back to you. |
and
I will always be looking for a solution that doesn't require upstream changes, but adding a method such as mentioned above would be amazing and could make our lives easier. Additionally, could there be a way for the extension to pass the data through Thank you @bjoluc for the quick bind lesson and @jodeleeuw for the suggestion. Please let me know if anyone needs anything. |
That seems very reasonable to me. Happy to make upstream changes here given that they will be improving the functionality for everyone! |
Thanks @jodeleeuw! I can do a quick first pass at this, if that would be helpful? |
Yes please! Thanks @becky-gilbert |
Hi @jspsych/core, we're hoping to extend the recording extension for our project (CHS/Lookit) video recording extension, and use our extended version to push the recorded chunks into our own storage. However, the problem we're having when we try to extend the
RecordVideoExtension
class is that everything is either marked as private or not accessible because it is instantiated as a property rather than a method. E.g.Questions:
recordedChunks
as public?on_start
etc.) as methods?Please let us know if we're missing something obvious here! Do you know of any other users who have added features to extension classes? We'd be happy to reference their solutions.
We can do what we need to do without extending this class, so if these changes aren't possible, we can create our own recording extension rather than extending the existing one. We just thought it would be nice to extend the
RecordVideoExtension
class to avoid re-inventing the wheel, and so that we can push other features into jsPsych's class for other users (if you're interested).The text was updated successfully, but these errors were encountered: