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

[Steve Borho]

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().
>
> The guiding principle here is: extensions don't get to inflict pain on
> the core. And this is definitely pain.

Right, I'll back it out in a few.

-- 
Steve Borho