Skip to content
This repository has been archived by the owner on Dec 5, 2018. It is now read-only.

Fix for #237 the print command doesn't open the console #252

Merged
merged 6 commits into from
Jul 9, 2016

Conversation

atgeflu
Copy link
Contributor

@atgeflu atgeflu commented Jul 8, 2016

Hi,
This should be a fix for issue #237.

In this issue a fix was described already. I simply implemented it.

Now the console becomes visible when using print as well as println. I couldn't think of a way to add tests for it, so there aren't any. If you can think of a way to test this, I'd be willing to try to implement it.

Running node test didn't generate processing.min.js, so it's not included.

processing.js contains some unrelated changes again. I guess it might be because some files were changed after processing.js was updated.

Cheers,
Jean-Noël

} else {
e.javaconsole.scrollTop = e.javaconsole.scrollHeight;
}
if (e.BufferArray.length > e.BufferMax) e.BufferArray.splice(0, 1);
Copy link
Member

Choose a reason for hiding this comment

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

please don't include formatting changes in a functional PR, especially in this case: curly brackets serve a purpose, don't slow down code, and make things easier to read. Removing them is not a good idea =)

@Pomax
Copy link
Member

Pomax commented Jul 9, 2016

Looks good, but remember to focus on the issue at hand: if you're adding a PR that fixes print()s console showing, don't also sneak in formatting changes =)

(You can always file a new issue for fixing the formatting issue, and then land a second PR specifically for that)

@GoToLoop
Copy link
Contributor

GoToLoop commented Jul 9, 2016

Dunno much about print()'s implementation. The actual console.log() always append a linefeed at the end. Which of course doesn't match print()'s expected behavior...

@Pomax
Copy link
Member

Pomax commented Jul 9, 2016

Note that the print and println functions do not rely on the window.console API, they write string data into an overlay div, so console.log behaviour is not be a concern.

@GoToLoop
Copy link
Contributor

GoToLoop commented Jul 9, 2016

Alas, censorship again as always...

@atgeflu
Copy link
Contributor Author

atgeflu commented Jul 9, 2016

I undid the formatting changes. I only did them because the original poster thought it might be good to keep the formatting consistent throughout the file ;-)

But thanks for the feedback, I'll keep this in mind for future pull request.

@GoToLoop
Copy link
Contributor

GoToLoop commented Jul 9, 2016

In my deleted post, I've agreed w/ your 1 liner if (). It's tough to contribute here...

@atgeflu
Copy link
Contributor Author

atgeflu commented Jul 9, 2016

I personally don't mind the curly braces.
Mike's got a point though, not to mix formatting changes with the fix. Might also make it easier to check changes later in case something broke for example.
So I'll happily apply his advice and keep PRs focused on the actual fixes.

@Pomax
Copy link
Member

Pomax commented Jul 9, 2016

@GoToLoop apologies for that: I'm deleting any comment that sounds mocking, or unrelated to the PR itself (your comment was a joke involving me not likeing python programming style, which has nothing to do with a JS codebase. PR comments are for comments on code, not for comments about the people who wrote the code or review the code)

@Pomax Pomax merged commit b45a747 into processing-js:master Jul 9, 2016
@atgeflu atgeflu deleted the console branch July 13, 2016 20:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants