Enable colors on-demand.

Enable colors on-demand.

R=kustermann@google.com, sortie@google.com

Committed: https://github.com/dart-lang/sdk/commit/4d302b0adbebb6ee9cdfc99a5e9ecf96c686befe

[ahe, 2017-03-13 15:34:32]

ahe@google.com changed reviewers:
+ kustermann@google.com

[kustermann, 2017-03-13 15:39:18]

LGTM

https://codereview.chromium.org/2746963002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/colors.dart (right):

https://codereview.chromium.org/2746963002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/colors.dart:48: return stdioType(stdout) ==
StdioType.TERMINAL && numberOfColors() >= 8;
Are you writing errors to stdout or stderr?

[sortie, 2017-03-13 15:51:03]

sortie@google.com changed reviewers:
+ sortie@google.com

[sortie, 2017-03-13 15:51:03]

lgtm fwiw

https://codereview.chromium.org/2746963002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/colors.dart (right):

https://codereview.chromium.org/2746963002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/colors.dart:21: const String WHITE_COLOR =
'\u001b[37m';
These sequences are very widely implemented and standard, but they're not
universal and there's still weird terminals out there. Since you're already
invoking tput, you may want to consider using it to figure out the appropriate
color sequences, but with our current dart audiences, I don't really see any
compelling reason to incur the cost of invoking a program.

https://codereview.chromium.org/2746963002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/colors.dart:41: ProcessResult result =
Process.runSync("/usr/bin/tput", ["colors"]);
You may want to use the tput in the user's PATH which may not necessarily be
/usr/bin/tput.

[ahe, 2017-03-14 11:52:06]

Major changes, PTAL.

https://codereview.chromium.org/2746963002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/colors.dart (right):

https://codereview.chromium.org/2746963002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/colors.dart:21: const String WHITE_COLOR =
'\u001b[37m';
On 2017/03/13 15:51:03, sortie wrote:
> These sequences are very widely implemented and standard, but they're not
> universal and there's still weird terminals out there. Since you're already
> invoking tput, you may want to consider using it to figure out the appropriate
> color sequences, but with our current dart audiences, I don't really see any
> compelling reason to incur the cost of invoking a program.

Done.

https://codereview.chromium.org/2746963002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/colors.dart:41: ProcessResult result =
Process.runSync("/usr/bin/tput", ["colors"]);
On 2017/03/13 15:51:03, sortie wrote:
> You may want to use the tput in the user's PATH which may not necessarily be
> /usr/bin/tput.

Done.

https://codereview.chromium.org/2746963002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/colors.dart:48: return stdioType(stdout) ==
StdioType.TERMINAL && numberOfColors() >= 8;
On 2017/03/13 15:39:18, kustermann wrote:
> Are you writing errors to stdout or stderr?

In general, I don't know. So I've changed it to ensure both are terminals.

[sortie, 2017-03-14 12:13:48]

lgtm

https://codereview.chromium.org/2746963002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/colors.dart (right):

https://codereview.chromium.org/2746963002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/colors.dart:25: /// command-line program `tput setaf
0`.
You probably mean setaf 1 and etc below.

https://codereview.chromium.org/2746963002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/colors.dart:125: "/bin/sh", ["-c", "printf '%s'
'$TERMINAL_CAPABILITIES' | tput -S"]);
You can runInShell: true here to have /bin/sh -c added, and then just put the
shell pipeline command as the executable name with no arguments.

https://codereview.chromium.org/2746963002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/colors.dart:157: "${JSON.encode(ALL_CODES)} !=
${JSON.encode(allCodes)}.");
You're going to all the trouble to learn the appropriate escape colors for the
user's terminal and then you discard them if it doesn't match what you expect.
You could just respect them, although splitting the error codes for each color
is probably tricky when you don't know the format, as it's all on one line. This
current approach is fine though, people on weird terminals don't get colors,
rather than wrong colors.

[ahe, 2017-03-14 12:58:57]

Thank you, Martin and Jonas!

https://codereview.chromium.org/2746963002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/colors.dart (right):

https://codereview.chromium.org/2746963002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/colors.dart:25: /// command-line program `tput setaf
0`.
On 2017/03/14 12:13:48, sortie wrote:
> You probably mean setaf 1 and etc below.

Done.

https://codereview.chromium.org/2746963002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/colors.dart:125: "/bin/sh", ["-c", "printf '%s'
'$TERMINAL_CAPABILITIES' | tput -S"]);
On 2017/03/14 12:13:48, sortie wrote:
> You can runInShell: true here to have /bin/sh -c added, and then just put the
> shell pipeline command as the executable name with no arguments.

That doesn't work because runInShell also adds quotes that shouldn't be there.

  ProcessResult result = Process.runSync(
      'printf %s \"$TERMINAL_CAPABILITIES\" | tput -S', [], runInShell: true);

This becomes equivalent to:

  /bin/sh -c "'printf %s \"$TERMINAL_CAPABILITIES\" | tput -S'"

No matter what I do, I can't get make it not quote `|`.

For example:

Process.runSync("echo hello | cat", [], runInShell: true);

Prints this error:

/bin/sh: echo hello | cat: command not found

That is, the entire command is interpreted as a file name.

Process.runSync("echo", ["hello | cat"], runInShell: true);

This prints "hello | cat", not "hello".

Same happens for

Process.runSync("echo", ["hello", "|", "cat"], runInShell: true);

https://codereview.chromium.org/2746963002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/colors.dart:157: "${JSON.encode(ALL_CODES)} !=
${JSON.encode(allCodes)}.");
On 2017/03/14 12:13:48, sortie wrote:
> You're going to all the trouble to learn the appropriate escape colors for the
> user's terminal and then you discard them if it doesn't match what you expect.
> You could just respect them, although splitting the error codes for each color
> is probably tricky when you don't know the format, as it's all on one line.
This
> current approach is fine though, people on weird terminals don't get colors,
> rather than wrong colors.

Computing the error codes dynamically would have these negative side-effects:

1. I'd have to call tput 8 times.

2. The color codes wouldn't be compile-time constants anymore.

I don't think that's worth it, because, as you say, people on weird terminals
just don't get colors.

If there was a way to generate colors on a Windows terminal, now that would be
something :-)

[ahe, 2017-03-14 13:23:27]

Description was changed from

==========
Enable colors on-demand.
==========

to

==========
Enable colors on-demand.

R=kustermann@google.com, sortie@google.com

Committed:
https://github.com/dart-lang/sdk/commit/4d302b0adbebb6ee9cdfc99a5e9ecf96c686befe
==========

[ahe, 2017-03-14 13:23:28]

Committed patchset #3 (id:40001) manually as
4d302b0adbebb6ee9cdfc99a5e9ecf96c686befe (presubmit successful).