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

[Danek Duvall]

On Mon, Mar 14, 2011 at 01:46:45PM +0100, Martin Geisler wrote:

> > +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?

The first paragraph, yes.  I've restored it.  Someone else can rewrap the
lines if they don't like the unevenness.  I've rewrapped the other
paragraphs I modified or added to 70 characters, so this first one is now a
little out of place.

> > [...]
> > +
> > +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.

Okay;  I'll update my patch once that makes it in.

> 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.

Thanks; that makes more sense.

> >  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.

The if branch is the same, not the else.  But I'm not sure what you mean I
should do here.  I'm not sure there's any way of preserving the indentation
without making the code itself harder to maintain.

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

Done.

> Our messages all start with a lower-case letter.

Done.

Thanks for the review!  I'll send the patch out again once we converge on
the render_effects() changes.

Thanks,
Danek