[dart:io] Move Platform.ansiSupported to {Stdin,Stdout}.supportsAnsiEscapes

[dart:io] Move Platform.ansiSupported to {Stdin,Stdout}.supportsAnsiEscapes

On Windows, some Windows 10 builds support only ANSI output, but not
input, so these need to be separated.

I'm also improving the detection on Mac and Linux to avoid hardcoding
the result. Instead, supportsAnsiEscapes will be true if isatty() and
the TERM environment variable contains the string 'xterm'.

related #28614

R=lrn@google.com

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

[zra, 2017-03-17 06:47:59]

Description was changed from

==========
[dart:io] Move Platform.ansiSupported to {Stdin,Stdout}.ansiSupported

related #28614
==========

to

==========
[dart:io] Move Platform.ansiSupported to {Stdin,Stdout}.ansiSupported

On Windows, some Windows 10 builds support only ANSI output, but not
input, so these need to be separated.

I'm also improving the detection on Mac and Linux to avoid hardcoding
the result. Instead, ansiSupported will be true if isatty() and
the TERM environment variable contains the string 'xterm'.

related #28614
==========

[zra, 2017-03-17 06:48:09]

zra@google.com changed reviewers:
+ asiva@google.com

[Lasse Reichstein Nielsen, 2017-03-17 08:55:28]

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

[Lasse Reichstein Nielsen, 2017-03-17 08:55:29]

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart
File sdk/lib/io/stdio.dart (right):

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart#n...
sdk/lib/io/stdio.dart:140: * When connected to a terminal, whether ANSI codes
are supported.
It doesn't say what happens when it's not connected to a terminal.
Would it be more correct to say:
  Whether connected to a terminal that supports "ANSI" escape sequences.
?

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart#n...
sdk/lib/io/stdio.dart:140: * When connected to a terminal, whether ANSI codes
are supported.
This is very vague. 

"ANSI codes" should probably be "ANSI escape codes" or "ANSI escape sequences",
because "ANSI codes" means something else
(https://www.census.gov/geo/reference/ansi.html)
It's still a bad name because it's not actually specified by ANSI, it's just
derived from VT-100 terminal escapes for (among other things) selecting ANSI
colors.

Since there is no formal specification, we can't say which escape codes are
supported, so add a paragraph, e.g.:

  The actual escape sequence support may differ between terminals,
  with some terminals supporting more escape sequences than others,
  and some terminals even differing in behavior for the same escape
  sequence.
  The ANSI color selection is generally supported.

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart#n...
sdk/lib/io/stdio.dart:144: * versions of Windows 10.
This is very specific. Consider just saying:

 Not all terminals are recognized, and not all recognized terminals can
 report whether they support ANSI escape sequences, so this value is a  
 best-effort attempt at detecting the support.

Then you can follow up with the preceding paragraph if you think it's useful:
  Currently, a `TERM` environment variable containing the string `xterm`
  will be taken as evidence that ANSI escape sequences are supported.
  On Windows, only versions of Windows 10 after v.1511 ("TH2")
  will be detected as supporting ANSI escape sequences.

Drop "recent", this comment might still be there in five years.

(Is the Windows comment correct? Is it only the Window32 console or also
Powersheell? Will we not detect that ansi.sys is loaded? Will we not detect
terminals like ConEmu, Cmder or Console2?)

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart#n...
sdk/lib/io/stdio.dart:146: external bool get ansiSupported;
I'd prefer a name like `supportsAnsiEscapes` (or *maybe* just `supportsAnsi`).
Even `isAnsiSupported` or `hasAnsiSupport` are better than the current, but the
first one is more direct.

[zra, 2017-03-17 15:38:21]

Description was changed from

==========
[dart:io] Move Platform.ansiSupported to {Stdin,Stdout}.ansiSupported

On Windows, some Windows 10 builds support only ANSI output, but not
input, so these need to be separated.

I'm also improving the detection on Mac and Linux to avoid hardcoding
the result. Instead, ansiSupported will be true if isatty() and
the TERM environment variable contains the string 'xterm'.

related #28614
==========

to

==========
[dart:io] Move Platform.ansiSupported to {Stdin,Stdout}.supportsAnsiEscapes

On Windows, some Windows 10 builds support only ANSI output, but not
input, so these need to be separated.

I'm also improving the detection on Mac and Linux to avoid hardcoding
the result. Instead, supportsAnsiEscapes will be true if isatty() and
the TERM environment variable contains the string 'xterm'.

related #28614
==========

[zra, 2017-03-17 15:38:51]

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart
File sdk/lib/io/stdio.dart (right):

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart#n...
sdk/lib/io/stdio.dart:140: * When connected to a terminal, whether ANSI codes
are supported.
On 2017/03/17 08:55:29, Lasse Reichstein Nielsen wrote:
> This is very vague. 
> 
> "ANSI codes" should probably be "ANSI escape codes" or "ANSI escape
sequences",
> because "ANSI codes" means something else
> (https://www.census.gov/geo/reference/ansi.html)
> It's still a bad name because it's not actually specified by ANSI, it's just
> derived from VT-100 terminal escapes for (among other things) selecting ANSI
> colors.
> 
> Since there is no formal specification, we can't say which escape codes are
> supported, so add a paragraph, e.g.:
> 
>   The actual escape sequence support may differ between terminals,
>   with some terminals supporting more escape sequences than others,
>   and some terminals even differing in behavior for the same escape
>   sequence.
>   The ANSI color selection is generally supported.
> 

Done.

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart#n...
sdk/lib/io/stdio.dart:140: * When connected to a terminal, whether ANSI codes
are supported.
On 2017/03/17 08:55:29, Lasse Reichstein Nielsen wrote:
> It doesn't say what happens when it's not connected to a terminal.
> Would it be more correct to say:
>   Whether connected to a terminal that supports "ANSI" escape sequences.
> ?

Done.

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart#n...
sdk/lib/io/stdio.dart:144: * versions of Windows 10.
On 2017/03/17 08:55:28, Lasse Reichstein Nielsen wrote:
> This is very specific. Consider just saying:
> 
>  Not all terminals are recognized, and not all recognized terminals can
>  report whether they support ANSI escape sequences, so this value is a  
>  best-effort attempt at detecting the support.
> 
> Then you can follow up with the preceding paragraph if you think it's useful:
>   Currently, a `TERM` environment variable containing the string `xterm`
>   will be taken as evidence that ANSI escape sequences are supported.
>   On Windows, only versions of Windows 10 after v.1511 ("TH2")
>   will be detected as supporting ANSI escape sequences.
> 
> Drop "recent", this comment might still be there in five years.
> 
> (Is the Windows comment correct? Is it only the Window32 console or also
> Powersheell? Will we not detect that ansi.sys is loaded? Will we not detect
> terminals like ConEmu, Cmder or Console2?)

Done.

Flutter is recommending PowerShell on Windows, so I don't think we should go to
great lengths to support anything else.

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart#n...
sdk/lib/io/stdio.dart:146: external bool get ansiSupported;
On 2017/03/17 08:55:29, Lasse Reichstein Nielsen wrote:
> I'd prefer a name like `supportsAnsiEscapes` (or *maybe* just `supportsAnsi`).
> Even `isAnsiSupported` or `hasAnsiSupport` are better than the current, but
the
> first one is more direct.

Done.

[Lasse Reichstein Nielsen, 2017-03-17 16:02:49]

lgtm

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart
File sdk/lib/io/stdio.dart (right):

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart#n...
sdk/lib/io/stdio.dart:144: * versions of Windows 10.
This is not Flutter specific, the comment applies to all platforms that support
dart:io.

https://codereview.chromium.org/2753233002/diff/60001/sdk/lib/io/stdio.dart
File sdk/lib/io/stdio.dart (right):

https://codereview.chromium.org/2753233002/diff/60001/sdk/lib/io/stdio.dart#n...
sdk/lib/io/stdio.dart:206: *   * Get the number of lines of the terminal.
Undo here :)

[zra, 2017-03-17 17:45:35]

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart
File sdk/lib/io/stdio.dart (right):

https://codereview.chromium.org/2753233002/diff/20001/sdk/lib/io/stdio.dart#n...
sdk/lib/io/stdio.dart:144: * versions of Windows 10.
On 2017/03/17 16:02:49, Lasse Reichstein Nielsen wrote:
> This is not Flutter specific, the comment applies to all platforms that
support
> dart:io.

I'm afraid I don't follow you. You asked whether various consoles are supported
by this change on Windows. I responded that developing and validating support
beyond what we have a customer requesting is not in scope right now.

https://codereview.chromium.org/2753233002/diff/60001/sdk/lib/io/stdio.dart
File sdk/lib/io/stdio.dart (right):

https://codereview.chromium.org/2753233002/diff/60001/sdk/lib/io/stdio.dart#n...
sdk/lib/io/stdio.dart:206: *   * Get the number of lines of the terminal.
On 2017/03/17 16:02:49, Lasse Reichstein Nielsen wrote:
> Undo here :)

Whoops! Thanks.

[zra, 2017-03-17 19:35:41]

Description was changed from

==========
[dart:io] Move Platform.ansiSupported to {Stdin,Stdout}.supportsAnsiEscapes

On Windows, some Windows 10 builds support only ANSI output, but not
input, so these need to be separated.

I'm also improving the detection on Mac and Linux to avoid hardcoding
the result. Instead, supportsAnsiEscapes will be true if isatty() and
the TERM environment variable contains the string 'xterm'.

related #28614
==========

to

==========
[dart:io] Move Platform.ansiSupported to {Stdin,Stdout}.supportsAnsiEscapes

On Windows, some Windows 10 builds support only ANSI output, but not
input, so these need to be separated.

I'm also improving the detection on Mac and Linux to avoid hardcoding
the result. Instead, supportsAnsiEscapes will be true if isatty() and
the TERM environment variable contains the string 'xterm'.

related #28614

R=lrn@google.com

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

[zra, 2017-03-17 19:35:42]

Committed patchset #5 (id:80001) manually as
b3093ecee63152032750ede294f52275c5d34928 (presubmit successful).

[siva, 2017-03-17 23:54:24]

lgtm

https://codereview.chromium.org/2753233002/diff/80001/runtime/bin/stdio.cc
File runtime/bin/stdio.cc (right):

https://codereview.chromium.org/2753233002/diff/80001/runtime/bin/stdio.cc#ne...
runtime/bin/stdio.cc:116: }
The above chunk could be written as (avoids multiple transitions into the VM)
intptr_fd = -1;
Dart_Handle result = Dart_GetNativeIntegerArgument(args, 0, &fd);
if (Dart_IsError(result) || (fd != 1) || (fd != 2)) {
  Dart_SetReturnValue(args, Dart_IsError(result) ? result :
Dart_NewApiError("Terminal fd must be 1 or 2"));
}
....
....

https://codereview.chromium.org/2753233002/diff/80001/runtime/bin/stdio_patch...
File runtime/bin/stdio_patch.dart (right):

https://codereview.chromium.org/2753233002/diff/80001/runtime/bin/stdio_patch...
runtime/bin/stdio_patch.dart:128: throw new StdoutException("Error determining
ANSI support", result);
Would it make sense to include the 'fd' in the exception error message?

[siva, 2017-03-17 23:54:26]

lgtm