On Thu, 4 Feb 2010, Giuseppe Bilotta wrote:

quoted text
> Subject: [PATCH 2/4] gitweb: show notes in shortlog view

Is it RFC?

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?

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

Signoff?

quoted text
> ---
>  gitweb/gitweb.css  |   29 +++++++++++++++++++++++++++++
>  gitweb/gitweb.perl |   30 +++++++++++++++++++++++++++++-
>  2 files changed, 58 insertions(+), 1 deletions(-)
> 
> diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
> index 50067f2..7d1836b 100644
> --- a/gitweb/gitweb.css
> +++ b/gitweb/gitweb.css
> @@ -572,3 +572,32 @@ span.match {
>  div.binary {
>  	font-style: italic;
>  }
> +
> +span.notes {
> +	float:right;
> +	position:relative;
> +}
> +
> +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.

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.

[...]
quoted text
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 9ba5815..1701ed1 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1628,6 +1628,33 @@ sub format_subject_html {
>  	}
>  }
>  
> +# display notes next to a commit
> +sub format_notes_html {
> +	my %notes = %{$_[0]};

Why not use 'my $notes = shift;', and later '%$notes'?

quoted text
> +	my $ret = "";

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

quoted text
> +	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/!!;

quoted text
> +		$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?


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)?

quoted text
> +		}
> +		$ret .= "</span></span>";
> +	}
> +	if ($ret) {
> +		return "<span class='notes'>$ret</span>";
> +	} else {
> +		return $ret;
> +	}
> +
> +
> +}
> +
>  # Rather than recomputing the url for an email multiple times, we cache it
>  # after the first hit. This gives a visible benefit in views where the avatar
>  # for the same email is used repeatedly (e.g. shortlog).
> @@ -4595,6 +4622,7 @@ sub git_shortlog_body {
>  		my %co = %{$commitlist->[$i]};
>  		my $commit = $co{'id'};
>  		my $ref = format_ref_marker($refs, $commit);
> +		my $notes = format_notes_html($co{'notes'});
>  		if ($alternate) {
>  			print "<tr class=\"dark\">\n";
>  		} else {
> @@ -4605,7 +4633,7 @@ sub git_shortlog_body {
>  		print "<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n" .
>  		      format_author_html('td', \%co, 10) . "<td>";
>  		print format_subject_html($co{'title'}, $co{'title_short'},
> -		                          href(action=>"commit", hash=>$commit), $ref);
> +		                          href(action=>"commit", hash=>$commit), $ref . $notes);
>  		print "</td>\n" .
>  		      "<td class=\"link\">" .
>  		      $cgi->a({-href => href(action=>"commit", hash=>$commit)}, "commit") . " | " .

Nice.

-- 
Jakub Narebski
Poland
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html