[PATCH] color: labeled text should be passed to ui.write() as ui.labeled

[Steve Borho]

On Tue, Jun 8, 2010 at 4:42 PM, Matt Mackall <mpm at selenic.com> wrote:
> On Tue, 2010-06-08 at 16:32 -0500, Steve Borho wrote:
>> On Tue, Jun 8, 2010 at 3:49 PM, Steve Borho <steve at borho.org> wrote:
>> > On Tue, Jun 8, 2010 at 3:44 PM, Matt Mackall <mpm at selenic.com> wrote:
>> >> On Thu, 2010-06-03 at 23:53 -0500, Steve Borho wrote:
>> >>> # HG changeset patch
>> >>> # User Steve Borho <steve at borho.org>
>> >>> # Date 1275625098 18000
>> >>> # Node ID ce4224afddd4e2c3c3a138d0fdec001fe0dbd5ef
>> >>> # Parent  e581f3acc3385da5d5ddf547882f274d9f7e5b6d
>> >>> color: labeled text should be passed to ui.write() as ui.labeled
>> >>>
>> >>> Some implementations of ui.label() (HTML versions in particular) must escape
>> >>> the provided text and then markup the text with their tags.  When this marked
>> >>> up text is then passed to ui.write(), we must label the text as 'ui.labeled'
>> >>> so the implementation knows not to escape it a second time (exposing the initial
>> >>> markup).
>> >>>
>> >>> This required the addition of a 'ui.plain' label for text that is purposefully
>> >>> not marked up.
>> >>>
>> >>> I was a little pedantic here, passing even ' ' strings to ui.label() when it
>> >>> would be included with other labeled text in a ui.write() call.   But it seemed
>> >>> appropriate to lean to the side of caution.
>> >>
>> >> Missed this on the list, but I see this made it into crew. I can't say I
>> >> like it..
>> >
>> > I pinged this on IRC a couple of times as well, but got no comments.  Oh well.
>> >
>> >>> diff --git a/hgext/churn.py b/hgext/churn.py
>> >>> --- a/hgext/churn.py
>> >>> +++ b/hgext/churn.py
>> >>> @@ -150,8 +150,10 @@
>> >>>      if opts.get('diffstat'):
>> >>>          width -= 15
>> >>>          def format(name, (added, removed)):
>> >>> -            return "%s %15s %s%s\n" % (pad(name, maxname),
>> >>> -                                       '+%d/-%d' % (added, removed),
>> >>> +            return "%s %15s %s%s\n" % (ui.label(pad(name, maxname),
>> >>> +                                                'ui.plain'),
>> >>> +                                       ui.label('+%d/-%d' % (added, removed),
>> >>> +                                                'ui.plain'),
>> >>>                                         ui.label('+' * charnum(added),
>> >>>                                                  'diffstat.inserted'),
>> >>>                                         ui.label('-' * charnum(removed),
>> >>
>> >> There are a lot of changes here that don't make any sense unless you
>> >> know that there's some weird implementation detail -that you can't
>> >> actually find anywhere in the source- that's getting tripped over.
>> >>
>> >> This is not great. It'd be better to inflict some ugly hack on the HTML
>> >> labeler than on the whole of core. For instance, enclosing labeled
>> >> that's gone through the wash once already in "<div class=labelled>" and
>> >> then stripping that out later in a wrapper on ui.write().
>>
>> FWIW, this workaround cannot work.  Any changeset that introduced this
>> change would itself get mangled if it went through ui.write().
>
> Yes, you do have to ensure that the string you add to the labelled text
> doesn't appear literally in the source. But that's easy to do with +.

You lost me.  If I knew the string had been formatted before, it's
trivial.  But the API without my patch provides no hints whether the
ui.write() arguments are raw or formatted.  Inserting special
characters is a hack that artificially restricts what valid Mercurial
output looks like.  But.. the ANSII version of color.py already does
this so I guess it's safe for me as well.

>>   I
>> think I'll have to embed low ordinal ascii start-stop markers similar
>> to the ANSI ones and pray Mercurial does not use them in normal
>> output.
>
> That's fine: it's safe to assume that anything that gets labeled will
> pass through write and friends on its way out.

Certainly.

-- 
Steve Borho