Skip to content
This repository has been archived by the owner on May 30, 2023. It is now read-only.

Sync std{in,out,err} Encodings with Terminal #11234

Closed

Conversation

execjosh
Copy link
Contributor

This is a spin off from #11168.

This modification keeps the system.std{in,out,err} encodings in sync with the Terminal. That is, whenever the Terminal encoding gets changed, that update will now be propagated to the System module.

SIDE NOTE: Should the reverse case (propagate system.std{in,out,err} encoding change to Terminal) be handled?

If a `File` is in "text" mode, then it has an encoding.  This
encoding defaults to UTF-8; however, it can be set only at time of
construction (by using `fs.open`).

This modification allows the user to change the encoding on-the-fly
for "text" mode `File` instances.
@ariya
Copy link
Owner

ariya commented Apr 14, 2013

Do you want this to be merged as a squashed commit? If not, we still need to refer to the issue for later tracking.

@JamesMGreene
Copy link
Collaborator

@execjosh: Regarding your side note:

Should the reverse case (propagate system.std{in,out,err} encoding change to Terminal) be handled?

Does the Terminal encoding need to match the system.std{in, out, err} encoding in order to read/write characters properly? If so, then I would definitely say "yes". If not, then I'm not certain... maybe still "yes" because you'd want that encoding setting to persist across different system.std{in, out, err} "sessions" within a single PhantomJS run but then you should also revert the Terminal's encoding to its original when PhantomJS exits.

@execjosh
Copy link
Contributor Author

@ariya: I wasn't really sure what "the issue" should be... I guess should I link to both #11168 and #11162 in all of the commits?

As for squashing, I don't really have any preference; so, whatever granularity you think is best for the history is probably best 😃

@execjosh
Copy link
Contributor Author

@JamesMGreene: Thank you for your input!

Terminal is a singleton that monopolizes c{out,err} (essentially any output from the C++ side, and, I guess, console.log). It is purely internal to PhantomJS and should have absolutely no effect on the actual shell environment running phantomjs. The encoding used by Terminal can be set via --output-encoding on the command line (it initially gets set to UTF-8).

system.std{in,out,err} are File instances that use UTF-8 for their encoding, each instance having their own, separate pointer to the text codec. If at some point in time the Terminal instance encoding gets changed, then they will not be updated, which could possibly lead to unexpected behavior. Hence this modification.

However, if the user changes one of system.std{out,err}'s encoding, I had thought that it might be a good idea to propagate back to Terminal. But, that could lead to system.stdout and Terminal being one encoding, and system.stderr being in another.

Hmm... It might actually be best just not to do any syncing in either direction at all, maybe? Hmm...

I guess the big problem is that Terminal, system.stdin, system.stdout, and system.stderr each have their own encodings...

@execjosh
Copy link
Contributor Author

Any opinions?

@ariya
Copy link
Owner

ariya commented Apr 25, 2013

Sorry for the late response, will have a look this evening.

ariya pushed a commit that referenced this pull request Apr 29, 2013
ariya pushed a commit that referenced this pull request Apr 29, 2013
If a `File` is in "text" mode, then it has an encoding.  This
encoding defaults to UTF-8; however, it can be set only at time of
construction (by using `fs.open`).

This modification allows the user to change the encoding on-the-fly
for "text" mode `File` instances.

See #11234 #11234
Spin off from #11168 #11168
ariya pushed a commit that referenced this pull request Apr 29, 2013
@ariya
Copy link
Owner

ariya commented Apr 29, 2013

Merged. Thanks!

@ariya ariya closed this Apr 29, 2013
@execjosh execjosh deleted the sync-std-stream-enc-with-terminal branch May 9, 2013 14:17
@execjosh
Copy link
Contributor Author

execjosh commented May 9, 2013

If anyone's getting link errors after applying this, they may need to do touch src/phantomjs.pro (or the equivalent on their OS) so that qmake rebuilds the moc stuff for Terminal.

@vitallium
Copy link
Collaborator

Exactly! I forgot to re-run qmake :(

JamesMGreene referenced this pull request Jun 25, 2013
See [issue 333][1] and pull request #192.

**Caveat**

`File::read` currently takes no parameters and is equivalent to a
"`readAll`".  This will be changed later to match [IO/A Spec's
`Stream#read`][2]; but, should still be noted.

[1]: http://code.google.com/p/phantomjs/issues/detail?id=333
[2]: http://wiki.commonjs.org/wiki/IO/A#Instance_Methods
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.

None yet

4 participants