[PATCH 2 of 3 V2] color: enable ANSI support on Windows 10

[FUJIWARA Katsunori]

At Sun, 28 May 2017 14:11:15 -0400,
Matt Harbison wrote:
> 
> 
> > On May 28, 2017, at 1:14 PM, FUJIWARA Katsunori <foozy at lares.dti.ne.jp> wrote:
> > 
> > At Sat, 27 May 2017 00:19:22 -0400,
> > Matt Harbison wrote:
> >> 
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison at yahoo.com>
> >> # Date 1495506038 14400
> >> #      Mon May 22 22:20:38 2017 -0400
> >> # Node ID d35feab18347e36cf3e55b6b797602088917de09
> >> # Parent  01e230f1d93a0b0e54164fbce14b5a0c093a4fc4
> >> color: enable ANSI support on Windows 10
> >> 
> >> This will display color if "color.mode=ansi", and default to 'ansi' if the mode
> >> is set to 'auto'.  The 'debugcolor' command also reflects this policy.
> >> Previously, "color.mode=ansi" on Windows printed jibberish around the normal
> >> text.  Using ANSI color is better, as it avoids the normal loss of color when
> >> the default pager is enabled on Windows.  See also issue5570.
> >> 
> >> When the underlying function fails (e.g. when run on older Windows), 'auto'
> >> still falls back to 'win32'.  Apparently, Microsoft originally had this feature
> >> turned on by default, and then made it opt-in[1].  Therefore, not enabling it
> >> unconditionally seems safer.  Instead, only do it in the 'auto' case after
> >> processing the existing check for support in a Unix-like environment.
> >> 
> >> [1] https://github.com/symfony/symfony/issues/17499#issuecomment-243481052
> >> 
> >> diff --git a/mercurial/color.py b/mercurial/color.py
> >> --- a/mercurial/color.py
> >> +++ b/mercurial/color.py
> >> @@ -215,8 +215,10 @@
> >>         mode = ui.config('color', 'pagermode', mode)
> >> 
> >>     realmode = mode
> >> -    if mode == 'auto':
> >> -        if pycompat.osname == 'nt':
> >> +    if pycompat.osname == 'nt':
> >> +        from . import win32
> >> +
> >> +        if mode == 'auto':
> >>             term = encoding.environ.get('TERM')
> >>             # TERM won't be defined in a vanilla cmd.exe environment.
> >> 
> >> @@ -227,12 +229,15 @@
> >>             # gibberish, we error on the side of selecting "win32". However, if
> >>             # w32effects is not defined, we almost certainly don't support
> >>             # "win32", so don't even try.
> >> -            if (term and 'xterm' in term) or not w32effects:
> >> +            if ((term and 'xterm' in term) or win32.enablevtmode()
> >> +                 or not w32effects):
> > 
> > Is there any reason to examine win32.enablevtmode() before w32effects?
> > 
> > Examining w32effects earlier can avoid meaningless SetConsoleMode(),
> > if stdout is redirected.
> 
> The way this conditional is here:
> 
> 1) 'xterm' in term -> the status quo, where we think the environment supports ANSI without help from Windows.  So use ANSI without asking Windows for help, because presumably it worked before
> 2) win32.enablevtmode() == True -> Windows supports it (and it's now enabled), so go with ANSI
> 3) not w32effects -> we know win32 doesn't work, so try ANSI (without Windows support) and hope for the best
> 4) Otherwise, we're pretty sure that ANSI is not supported, but win32 is.  So go with that.
> 
> I think ANSI (with Windows support) should take precedence over
> win32, because that works with the default pager.  That's a way more
> common case than redirecting stdout, so it seems like this is a
> micro optimization outweighed by the normal UX.  Do you have a
> concern with calling SetConsoleMode() extraneously?
> 

I think that examination of w32effects before win32.enablevtmode() is
better, because:

  - we know that GetConsoleMode() and SetConsoleMode() on stdout fail,
    if stdout is redirected

  - we know "not w32effects" is true, if stdout is redirected

  - "not w32effects" is already examined in existing code

color.modesetup() will be invoked:

  - x 2 in dispatch, for ui and lui
  - x # of localrepo instance in localrepository.__init__
  - x 1 in ui.pager, if color.pagermode is explicitly configured

Even though total cost of Get/SetConsoleMode() should be cheaper
enough than each hg commands, avoiding meaningless executions is
better, if it can be achieved enough cheaply, IMHO.

But I'm OK, if reordering these makes precedence of "ANSI" in mode
detection more understandable.


> > 
> >>                 realmode = 'ansi'
> >>             else:
> >>                 realmode = 'win32'
> >> -        else:
> >> -            realmode = 'ansi'
> >> +        elif mode == 'ansi':
> >> +            win32.enablevtmode()
> > 
> > Similarly, "mode == 'ansi' and w32effects".
> 
> In this case, the user explicitly set ANSI, so w32effects doesn't matter.  We do still need to enable support in Windows for cmd.exe.  (And yes, this does contravene the choice above to be conservative about enabling this, but I didn't see a better way, or know if it really matters.  I don't imagine that there are a lot of people setting ANSI on Windows explicitly.)
> 

I overlooked similarity between "ansi for auto" and "explicit ansi",
too.

In implementation above, "(term and 'xterm' in term)" avoids
SetConsoleMode() at "auto" mode, but doesn't at explicit "ansi"
mode. We should keep similarity between then in order to avoid
unintentional difference of behavior especially for third party
terminals and/or pagers, shouldn't we?


> >> +    elif mode =='auto':
> >> +        realmode = 'ansi'
> >> 
> >>     def modewarn():
> >>         # only warn if color.mode was explicitly set and we're in
> >> 
> > 
> > 
> > -- 
> > ----------------------------------------------------------------------
> > [FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp
> 

-- 
----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp