-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fix: prevent print() with no arguments from triggering browser print dialog #8244
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
Conversation
|
🎉 Thanks for opening this pull request! For guidance on contributing, check out our contributor guidelines and other resources for contributors! Thank You! |
src/core/environment.js
Outdated
| windowPrintDisabled = true; | ||
| } | ||
| } | ||
| console.warn('p5.js: print() called with no arguments. Nothing to print.'); |
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.
shouldn't we print a newline character if no argument is passed.
It is a convention that is followed in java println or js console.log.
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 the suggestion, @oneplusiota! 🙏
That’s a great point — following the convention of console.log() and Java’s println makes sense for consistency.
I can update the implementation so that when print() is called without any arguments, it simply prints a newline (i.e., console.log("")) instead of showing a warning.
Would you prefer that behaviour instead of the warning message? I can make the adjustment and push an update once confirmed.
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 this will work.
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.
Great! I've updated the implementation so that print() with no arguments now outputs a blank line (console.log("")).
Let me know if any further tweaks are needed. 👍
perminder-17
left a comment
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.
From my opinion, if we write just print() , it's supposed to print() the window. I don't find that to be a bug? Probably we could improve the documentations letting the user know what does print() does with no arguments? What you think?
|
@perminder-17 I disagree. Why should |
|
Thanks both for your feedback — I appreciate the different perspectives here 🙏 My intent with this fix was to prevent I agree that consistency is important. From a usability point of view, having |
oneplusiota
left a comment
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.
lgtm
perminder-17
left a comment
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.
I'm so sorry, but this is the intended behavior of the print() functions and these are the discussions related to it: #4272 and #2765.
Thanks for all your work on this. Keep looking at the issue section: https://github.com/processing/p5.js/issues and would be very great to ask maintainers confirmation on the issues before working.
Really thanks for your work and patience on this one.
Resolves processing/p5.js-web-editor#3678
Changes:
Behavior Before:
Behavior After:
PR Checklist
npm run lintpasses