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

2010/2/6 Jakub Narebski [off-list ref]:

On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:

quoted

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.

quoted

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

Signoff?

A-hem. (whistles innocently)

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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