Re: [PATCH 2/4] gitweb: show notes in shortlog view
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Feb 6, 2010, 09:24 UTC
- Message-ID
- <cb7bb73a1002060124s7f9bc4f7y9c2cb418e4802f64@mail.gmail.com>
- In-Reply-To
- <201002060118.21137.jnareb@gmail.com>
2010/2/6 Jakub Narebski <jnareb@gmail.com>:
Show 5 quoted lines
> 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)
Show 7 quoted lines
>> +
>> +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.
Show 9 quoted lines
> 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.
Show 5 quoted lines
>> +# 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.
Show 6 quoted lines
>> + 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.
Show 14 quoted lines
>> + $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.
Show 5 quoted lines
> 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