Skip to content
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

Dispatch console log messages to an editor, if one is active. #6

Merged
merged 5 commits into from
Dec 30, 2023

Conversation

bthj
Copy link
Contributor

@bthj bthj commented Dec 10, 2023

Using choc::json::toString on the messages; maybe there is a better way for converting the choc::value::Values?

Added one console.log statement in dsp/main.js to easily verify the log message dispatches, by e.g. turning the knobs:

Screenshot 2023-12-10 at 13 19 28

Copy link
Contributor

@nick-thompson nick-thompson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great @bthj thank you so much. I left a few small change requests in-line, but I'd love to merge this asap. Let me know if you agree with the suggestions!

@@ -25,13 +25,15 @@ function requestParamValueUpdate(paramId, value) {
}
}

import.meta.hot.on('reload-dsp', () => {
console.log('Sending reload dsp message');
if (process.env.NODE_ENV !== 'production') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

for (size_t i = 0; i < args.numArgs; ++i) {
DBG(choc::json::toString(*args[i], true));

// Dispatch to the UI if it's available
if (auto* editor = static_cast<WebViewEditor*>(getActiveEditor())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general idea here is perfect, but I think we should change the implementation slightly– right now, this will evaluate a new expression in the webView for each argument passed to the __log__ function. I think instead of running the for loop over args.numArgs we should serialize args as its own array, and then change the console.log(%) expression to console.log(...JSON.parse(%)) so that we only evaluate the expression once, but we still pass the var args to console.log correctly

Copy link
Contributor Author

@bthj bthj Dec 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! I think you know better how to implement this change :)

return true;
})();
)script";

for (size_t i = 0; i < args.numArgs; ++i) {
DBG(choc::json::toString(*args[i], true));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here let's just drop this DBG I think

dsp/main.js Outdated Show resolved Hide resolved
native/PluginProcessor.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@bthj bthj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing this. I've committed some of the changes but am unsure how to do the args serialisation.

dsp/main.js Outdated Show resolved Hide resolved
native/PluginProcessor.cpp Outdated Show resolved Hide resolved
for (size_t i = 0; i < args.numArgs; ++i) {
DBG(choc::json::toString(*args[i], true));

// Dispatch to the UI if it's available
if (auto* editor = static_cast<WebViewEditor*>(getActiveEditor())) {
Copy link
Contributor Author

@bthj bthj Dec 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! I think you know better how to implement this change :)

@nick-thompson
Copy link
Contributor

Great @bthj thanks for making those changes. I'm going to change this to merge into the develop branch then I'll finish up the args serialization and merge to main. I appreciate your help!

@nick-thompson nick-thompson changed the base branch from main to develop December 30, 2023 15:24
@nick-thompson nick-thompson merged commit d295b87 into elemaudio:develop Dec 30, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants