[PATCH 1 of 1] color: add support for terminfo-based attributes and color

[Martin Geisler]

Danek Duvall <duvall at comfychair.org> writes:

Cool that you updated the patch!

> diff --git a/hgext/color.py b/hgext/color.py
> --- a/hgext/color.py
> +++ b/hgext/color.py
> @@ -19,15 +19,17 @@
>  '''colorize output from some commands
>  
>  This extension modifies the status and resolve commands to add color to their
> -output to reflect file status, the qseries command to add color to reflect
> -patch status (applied, unapplied, missing), and to diff-related
> -commands to highlight additions, removals, diff headers, and trailing
> -whitespace.
> +output to reflect file status, the qseries command to add color to reflect patch
> +status (applied, unapplied, missing), and to diff-related commands to highlight
> +additions, removals, diff headers, and trailing whitespace.

I think you just re-wrapped the docstring above, but did not really
change anything?

Please send such unrelated changes in separate patches -- here I would
say the lines were fine as they were. I use Emacs to wrap lines and the
default line length is 70 characters (not 80 like I think you used).

The advantage of using something less than 80 is that the lines can be
indented without wrapping in patches and emails. In the reply above,
each line is indented with three charactes and further replies will
indent with one or two extra characters ('>' or '> ').

> [...]
> +
> +The color extension will try to detect whether to use terminfo, ANSI codes or
> +Win32 console APIs, unless it is made explicit; e.g.::

The Win32 stuff is slated for removal, Matt has queued a patch by Adrian
that will use ctypes instead:

  http://mercurial.markmail.org/message/tve4avf43hqjeezv

I guess it is orthogonal to your patch, but it will still cause
confclits for some of you. I think Matt will push the ctypes change in
the next days.

> +try:
> +    import curses
> +    # Mapping from effect name to terminfo attribute name or color number.
> +    # This will also force-load the curses module.
> +    _terminfo_params = {'none': (True, 'sgr0'),
> +                        'standout': (True, 'smso'),
>
> [...]
>
> +                        'white': (False, curses.COLOR_WHITE)}
> +
> +    # If True, use terminfo-based color; if False use ECMA-48 codes directly.
> +    # Disable if we were unable to load the curses module.
> +    _terminfo = True
> +except ImportError:
> +    _terminfo = False

I guess I would do:

  try:
      import curses
      _terminfo = { ... }
  except ImportError:
      _terminfo = {}

and then just branch on 'if _terminfo' later in the code. Then there is
no need to keep a boolean around.

>  def render_effects(text, effects):
>      'Wrap text in commands to turn on each effect.'
>      if not text:
>          return text
> -    start = [str(_effects[e]) for e in ['none'] + effects.split()]
> -    start = '\033[' + ';'.join(start) + 'm'
> -    stop = '\033[' + str(_effects['none']) + 'm'
> +    if not _terminfo:
> +        start = [str(_effects[e]) for e in ['none'] + effects.split()]
> +        start = '\033[' + ';'.join(start) + 'm'
> +        stop = '\033[' + str(_effects['none']) + 'm'
> +    else:
> +        start = ''.join([
> +            _effect_str(effect)
> +            for effect in ['none'] + effects.split()
> +        ])
> +        stop = _effect_str('none')

The else-branch is doing the same as the original code, right? If so,
then try to preserve the lines as-is so that it's super easy for
reviewers to see what was just moved (indented) and what was changed.

>      return ''.join([start, text, stop])
>
> [...]
>
> +    for key, (b, e) in _terminfo_params.items():
> +        if not b:
> +            continue
> +        if not curses.tigetstr(e):
> +            # Most terminals don't support dim, invis, etc, so don't be
> +            # noisy and use ui.debug().
> +            ui.debug(_("No terminfo entry for %s\n") % e)

We have decided not to translate debug messages since they are often
quite internal in nature.

> +            del _terminfo_params[key]
> +    if not curses.tigetstr('setaf') or not curses.tigetstr('setab'):
> +        ui.warn(_("No terminfo entry for setab/setaf: reverting to "
> +          "ECMA-48 color\n"))
> +        _terminfo = False

Our messages all start with a lower-case letter.


-- 
Martin Geisler

aragost Trifork
Professional Mercurial support
http://aragost.com/en/services/mercurial/blog/