search

google / spatial-media

Specifications and tools for 360º video and spatial audio.
Other
1.86k stars 429 forks source link

Explicitly encode to UTF-8 when writing to console. #105

Closed pwroberts closed 8 years ago

pwroberts commented 8 years ago

This hopefully fixes https://github.com/google/spatial-media/issues/95. When running from the command line with "python gui.py", sys.stdout.encoding is "UTF-8", and printing Unicode strings to the console works. However, when running the PyInstaller binary, the console is redirected somewhere, sys.stdout.encoding is None, and print() blows up if the string contains any non-ASCII characters. By explicitly encoding the string passed to print() as UTF-8, it shouldn't matter what encoding sys.stdout is using.

suderman-google commented 8 years ago

Could you add the same change for the commandline tool in main.py?

dcower commented 8 years ago

LGTM % @suderman-google's comment

pwroberts commented 8 years ago

Adding the same change to the console output function in the command line tool actually breaks it, at least when I run it from the terminal on my Mac. The input strings coming from the command line parser are already "unicode" objects, and calling .encode('utf-8') on them tries to decode using the 'ascii' codec first, which fails if the string contains non-ASCII characters. Unchanged, the command line tool currently handles non-ASCII file paths just fine for me, even if I redirect the terminal output.

Since we haven't had any reports of encoding errors with the command line tool (AFAIK), I'd like to leave that code alone and restrict this change to the GUI app, where there is a known issue preventing people from using the tool.

Long term, or if there is a similar issue with the command line tool, we could pull in the UTF-8 writer from kitchen.text.converters mentioned in https://pythonhosted.org/kitchen/unicode-frustrations.html#frustration-4-now-it-doesn-t-take-byte-strings. This looks like it would give us robust console output across the board. But it involves a new package dependency (kitchen), and it looks like we don't have an automated way to declare dependencies right now. Once we have a setup.py, this will be easy, but that requires sorting out our versioning situation first.

dcower commented 8 years ago

SGTM! Thanks for the in-depth report. LGTM.