Patchwork [2,of,4] color: stop mutating the default effects map

login
register
mail settings
Submitter Matt Harbison
Date March 26, 2017, 4:41 a.m.
Message ID <a263702b064a5a3ce1ca.1490503265@Envy>
Download mbox | patch
Permalink /patch/19675/
State Accepted
Headers show

Comments

Matt Harbison - March 26, 2017, 4:41 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1490464217 14400
#      Sat Mar 25 13:50:17 2017 -0400
# Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874
# Parent  22533c3af63d5a67d9210596eafd4b99ab9c7904
color: stop mutating the default effects map

A future change will make color.setup() callable a second time when the pager is
spawned, in order to honor the 'color.pagermode' setting.  The problem was that
when 'color.mode=auto' was resolved to 'win32' in the first pass, the default
ANSI effects were overwritten, making it impossible to honor 'pagermode=ansi'.
Also, the two separate maps didn't have the same keys.  The symmetric difference
is 'dim' and 'italic' (from ANSI), and 'bold_background' (from win32).  Thus,
the update left entries that didn't belong for the current mode.

As an added bonus, this now correctly enables color with MSYS `less` for a
command like this, where pager is forced on:

    $ hg log --config color.pagermode=ansi --pager=yes

Previously, the output was corrupted.  The raw output, as seen through the ANSI
blind `more.com` was:

    <-[-1;6mchangeset:   34840:50bcb95d40f5<-[-1m
    ...

which MSYS `less -FRX` rendered as:

    1;6mchangeset:   34840:50bcb95d40f51m
    ...

(The two '<-' instances were actually an arrow character that TortoiseHg warned
couldn't be encoded, and notepad++ translated to a single '?'.)
Pierre-Yves David - March 26, 2017, 2:34 p.m.
On 03/26/2017 06:41 AM, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1490464217 14400
> #      Sat Mar 25 13:50:17 2017 -0400
> # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874
> # Parent  22533c3af63d5a67d9210596eafd4b99ab9c7904
> color: stop mutating the default effects map

As far as color is concerned, this is a step in the right direction, 
Code change looks good to me. Thanks
Yuya Nishihara - March 26, 2017, 2:35 p.m.
On Sun, 26 Mar 2017 00:41:05 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1490464217 14400
> #      Sat Mar 25 13:50:17 2017 -0400
> # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874
> # Parent  22533c3af63d5a67d9210596eafd4b99ab9c7904
> color: stop mutating the default effects map

I re-read the code, and noticed that both _effects and w32effects are mostly
constants. Perhaps we can simply switch them by ui._colormode.

  if ui._colormode == 'win32':
      activeeffects = w32neffects
  else:
      activeeffects = _effects
  ... embed escape sequences by using activeeffects ...
Matt Harbison - March 26, 2017, 5:27 p.m.
On Sun, 26 Mar 2017 10:35:34 -0400, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sun, 26 Mar 2017 00:41:05 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1490464217 14400
>> #      Sat Mar 25 13:50:17 2017 -0400
>> # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874
>> # Parent  22533c3af63d5a67d9210596eafd4b99ab9c7904
>> color: stop mutating the default effects map
>
> I re-read the code, and noticed that both _effects and w32effects are  
> mostly
> constants. Perhaps we can simply switch them by ui._colormode.
>
>   if ui._colormode == 'win32':
>       activeeffects = w32neffects
>   else:
>       activeeffects = _effects
>   ... embed escape sequences by using activeeffects ...

Will we get into trouble with each ui instance having a _colormode, but  
the activeeffects map effectively being global?  I don't fully understand  
the purpose of all the separate ui objects.
Yuya Nishihara - March 27, 2017, 12:19 p.m.
On Sun, 26 Mar 2017 13:27:57 -0400, Matt Harbison wrote:
> On Sun, 26 Mar 2017 10:35:34 -0400, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Sun, 26 Mar 2017 00:41:05 -0400, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1490464217 14400
> >> #      Sat Mar 25 13:50:17 2017 -0400
> >> # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874
> >> # Parent  22533c3af63d5a67d9210596eafd4b99ab9c7904
> >> color: stop mutating the default effects map
> >
> > I re-read the code, and noticed that both _effects and w32effects are  
> > mostly
> > constants. Perhaps we can simply switch them by ui._colormode.
> >
> >   if ui._colormode == 'win32':
> >       activeeffects = w32neffects
> >   else:
> >       activeeffects = _effects
> >   ... embed escape sequences by using activeeffects ...
> 
> Will we get into trouble with each ui instance having a _colormode, but  
> the activeeffects map effectively being global?

Yeah, maybe. But we can select either w32effects or _effects only by
ui._colormode. Unlike ui._styles, effects table is not user configurable,
which probably means we don't have to store a copy of effects in ui.

>  I don't fully understand  
> the purpose of all the separate ui objects.

That wouldn't be practically important for color, but it isn't nice to
mutate globals depending on ui.
Pierre-Yves David - March 27, 2017, 12:41 p.m.
On 03/26/2017 04:35 PM, Yuya Nishihara wrote:
> On Sun, 26 Mar 2017 00:41:05 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1490464217 14400
>> #      Sat Mar 25 13:50:17 2017 -0400
>> # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874
>> # Parent  22533c3af63d5a67d9210596eafd4b99ab9c7904
>> color: stop mutating the default effects map
>
> I re-read the code, and noticed that both _effects and w32effects are mostly
> constants. Perhaps we can simply switch them by ui._colormode.
>
>   if ui._colormode == 'win32':
>       activeeffects = w32neffects
>   else:
>       activeeffects = _effects
>   ... embed escape sequences by using activeeffects ...

Ultimately, yes we want to be there, but I remember other things muding 
with the effects dict so someone would have to give a close look to the 
current magic before moving forward.
Yuya Nishihara - March 27, 2017, 2:39 p.m.
On Mon, 27 Mar 2017 14:41:46 +0200, Pierre-Yves David wrote:
> On 03/26/2017 04:35 PM, Yuya Nishihara wrote:
> > On Sun, 26 Mar 2017 00:41:05 -0400, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1490464217 14400
> >> #      Sat Mar 25 13:50:17 2017 -0400
> >> # Node ID a263702b064a5a3ce1ca74b227e8e624e4b05874
> >> # Parent  22533c3af63d5a67d9210596eafd4b99ab9c7904
> >> color: stop mutating the default effects map
> >
> > I re-read the code, and noticed that both _effects and w32effects are mostly
> > constants. Perhaps we can simply switch them by ui._colormode.
> >
> >   if ui._colormode == 'win32':
> >       activeeffects = w32neffects
> >   else:
> >       activeeffects = _effects
> >   ... embed escape sequences by using activeeffects ...
> 
> Ultimately, yes we want to be there, but I remember other things muding
> with the effects dict so someone would have to give a close look to the 
> current magic before moving forward.

Maybe that is debugcolor (and w32effects.)

% grep _effects  **/*.py
mercurial/color.py:_effects = {
mercurial/color.py:        _effects.update(w32effects)
mercurial/color.py:    return ((not ui._terminfoparams and effect in _effects)
mercurial/color.py:    '''Helper function for render_effects().'''
mercurial/color.py:def _render_effects(ui, text, effects):
mercurial/color.py:        start = [str(_effects[e]) for e in ['none'] + effects.split()]
mercurial/color.py:        stop = '\033[' + str(_effects['none']) + 'm'
mercurial/color.py:            msg = '\n'.join([_render_effects(ui, line, effects)
mercurial/debugcommands.py:    for effect in color._effects.keys():

Patch

diff --git a/mercurial/color.py b/mercurial/color.py
--- a/mercurial/color.py
+++ b/mercurial/color.py
@@ -233,12 +233,14 @@ 
         if mode == realmode and ui.formatted():
             ui.warn(_('warning: failed to set color mode to %s\n') % mode)
 
+    ui._coloreffects = _effects.copy()
+
     if realmode == 'win32':
         ui._terminfoparams.clear()
         if not w32effects:
             modewarn()
             return None
-        _effects.update(w32effects)
+        ui._coloreffects = w32effects.copy()
     elif realmode == 'ansi':
         ui._terminfoparams.clear()
     elif realmode == 'terminfo':
@@ -273,7 +275,7 @@ 
 
 def valideffect(ui, effect):
     'Determine if the effect is valid or not.'
-    return ((not ui._terminfoparams and effect in _effects)
+    return ((not ui._terminfoparams and effect in ui._coloreffects)
              or (effect in ui._terminfoparams
                  or effect[:-11] in ui._terminfoparams))
 
@@ -324,9 +326,9 @@ 
                         for effect in ['none'] + effects.split())
         stop = _effect_str(ui, 'none')
     else:
-        start = [str(_effects[e]) for e in ['none'] + effects.split()]
+        start = [str(ui._coloreffects[e]) for e in ['none'] + effects.split()]
         start = '\033[' + ';'.join(start) + 'm'
-        stop = '\033[' + str(_effects['none']) + 'm'
+        stop = '\033[' + str(ui._coloreffects['none']) + 'm'
     return _mergeeffects(text, start, stop)
 
 _ansieffectre = re.compile(br'\x1b\[[0-9;]*m')
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -157,6 +157,7 @@ 
         # Blocked time
         self.logblockedtimes = False
         # color mode: see mercurial/color.py for possible value
+        self._coloreffects = {}
         self._colormode = None
         self._terminfoparams = {}
         self._styles = {}
@@ -176,6 +177,7 @@ 
             self.environ = src.environ
             self.callhooks = src.callhooks
             self.insecureconnections = src.insecureconnections
+            self._coloreffects = src._coloreffects.copy()
             self._colormode = src._colormode
             self._terminfoparams = src._terminfoparams.copy()
             self._styles = src._styles.copy()