-
Notifications
You must be signed in to change notification settings - Fork 14
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
Dispatch console log messages to an editor, if one is active. #6
Conversation
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 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') { |
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.
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())) { |
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.
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
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.
Sounds good! I think you know better how to implement this change :)
native/PluginProcessor.cpp
Outdated
return true; | ||
})(); | ||
)script"; | ||
|
||
for (size_t i = 0; i < args.numArgs; ++i) { | ||
DBG(choc::json::toString(*args[i], true)); |
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.
While we're here let's just drop this DBG I think
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.
Thanks for reviewing this. I've committed some of the changes but am unsure how to do the args
serialisation.
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())) { |
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.
Sounds good! I think you know better how to implement this change :)
Great @bthj thanks for making those changes. I'm going to change this to merge into the |
Using
choc::json::toString
on the messages; maybe there is a better way for converting thechoc::value::Value
s?Added one
console.log
statement indsp/main.js
to easily verify the log message dispatches, by e.g. turning the knobs: