Re: [PATCH 2/4] gitweb: show notes in shortlog view

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

2010/2/6 Jakub Narebski <jnareb@gmail.com>:
> On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:
>
>> Subject: [PATCH 2/4] gitweb: show notes in shortlog view
>
> Is it RFC?

See reply to comments on 1/4 8-/

> Why it is only for 'shortlog' view, and not also for 'history' which is
> also shortlog-like view?  Or is there reason why it is not present for
> 'history' view?

I always forget about history view, probably because I never use it.

>> The presence of the note is shown by a small icon, hovering on which
>> reveals the actual note content.
>
> Signoff?

A-hem. (whistles innocently)

>> +
>> +span.notes span.note-container:before {
>> +     content: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAgAAAAIAgMAAAC5YVYYAAAAAXNSR0IArs4c6QAAAAlQTFRFAABnybuD//+t5rXizQAAAAF0Uk5TAEDm2GYAAAABYktHRACIBR1IAAAAGElEQVQI12MIDWXIWglDQHYIQ1YAQ6gDAFWPBrAKFe0fAAAAAElFTkSuQmCC');
>> +}
>
> Not all web browsers support ':before' pseudo-element, and 'content'
> (pseudo-)property.

I know it's neither good form nor good webdesigner attitude, but I
stopped caring about IE a long time ago. I understand however that
some ancient versions of Mozilla browsers might have the same issue
too.

> Not all web browsers support 'data:' URI schema in CSS; also such image
> cannot be cached (on the other hand it doesn't require extra TCP
> connection on first access, and CSS file is cached anyway).
>
> On the other hand adding extra images to gitweb would probably require
> additional (yet another) build time parameter to tell where static
> images are (besides logo and favicon).
>
> So perhaps it is good solution, at least for a first attempt.

A possible alternative could maybe do without images and just use
borders and backgrounds of an 8x8 fixed-size element. Wouldn't look as
nice, probably, but should render decently in everything that supports
CSS1.

>> +# display notes next to a commit
>> +sub format_notes_html {
>> +     my %notes = %{$_[0]};
>
> Why not use 'my $notes = shift;', and later '%$notes'?

No particular reason. I didn't check for syntax preferences regarding
this in gitweb, or I would have noticed there was a preference for the
one you mention.

>> +     my $ret = "";
>
> Perhaps $return or $result would be a better name, to better distinguish
> it from visually similar $ref (see $ref vs $res);

Yep, good point.

>> +     while (my ($ref, $text) = each %notes) {
>> +             # remove 'refs/notes/' and an optional final s
>> +             $ref =~ s/^refs\/notes\///;
>
> You can use different delimiter than / to avoid 'leaning toothpick'
> syndrome, e.g.: $ref =~ s!^refs/notes/!!;

Indeed I should.

>> +             $ref =~ s/s$//;
>> +
>> +             # double markup is needed to allow pure CSS cross-browser 'popup'
>> +             # of the note
>> +             $ret .= "<span title='$ref' class='note-container $ref'>";
>> +             $ret .= "<span title='$ref' class='note $ref'>";
>> +             foreach my $line (split /\n/, $text) {
>> +                     $ret .= esc_html($line) . "<br/>";
>
> Probably would want
>
>                        $ret .= esc_html($line) . "<br/>\n";
>
> here.  Or do we want single string here?

It's within a span element so I was trying to stick to single line in
the HTML source.

> Also, do you want/need final <br>?  If not, perhaps
>
>                join("<br/>", map { esc_html($_) } split(/\n/, $text);
>
> would be a better solution (you can always add final "<br/>" later)?

I did notice that the final br didn't seem to affect the box height,
so I didn't bother looking at ways to do without it, but it's probably
nicer to not have it.

-- 
Giuseppe "Oblomov" Bilotta