From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:46:59
One more attempt at adding avatar support to gitweb.
The most significant change since the previous iteration is the addition
of a picon provider, and its use as default from gravatar.
Most other patches were changed to follow Jakub's suggestions. In
particular, the two-line author/committer information was refactored
to reduce code duplication, with the benefit that it could also be used
in 'tag' view.
Giuseppe Bilotta (9):
gitweb: refactor author name insertion
gitweb: uniform author info for commit and commitdiff
gitweb: use git_print_authorship_rows in 'tag' view too
gitweb: right-align date cell in shortlog
gitweb: (gr)avatar support
gitweb: gravatar url cache
gitweb: picon avatar provider
gitweb: use picon for gravatar fallback
gitweb: add alt text to avatar img
gitweb/gitweb.css | 13 ++-
gitweb/gitweb.perl | 238 ++++++++++++++++++++++++++------
t/t9500-gitweb-standalone-no-errors.sh | 2 +
3 files changed, 207 insertions(+), 46 deletions(-)
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:46:59
Collect all author display code in appropriate functions, making it
easier to extend these functions on the CGI side.
We also move some of the presentation code from hard-coded HTML to CSS,
for easier customization.
Signed-off-by: Giuseppe Bilotta <redacted>
---
gitweb/gitweb.css | 5 ++-
gitweb/gitweb.perl | 96 +++++++++++++++++++++++++++++++--------------------
2 files changed, 62 insertions(+), 39 deletions(-)
@@ -1469,6 +1469,16 @@ sub format_subject_html {}}+# format the author name of the given commit with the given tag+# the author name is chopped and escaped according to the other+# optional parameters (see chop_str).+subformat_author_html{+my$tag=shift;+my$co=shift;+my$author=chop_and_escape_str($co->{'author_name'},@_);+return"<$tag class=\"author\">".$author."</$tag>";+}+# format git diff header line, i.e. "diff --(git|combined|cc) ..."subformat_git_diff_header_line{my$line=shift;
@@ -3214,21 +3224,53 @@ sub git_print_header_div {"\n</div>\n";}+# Outputs the author name and date in long formsubgit_print_authorship{my$co=shift;+my%opts=@_;+my$tag=$opts{-tag}||'div';my%ad=parse_date($co->{'author_epoch'},$co->{'author_tz'});-print"<div class=\"author_date\">".+print"<$tag class=\"author_date\">".esc_html($co->{'author_name'})." [$ad{'rfc2822'}";-if($ad{'hour_local'}<6){-printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",-$ad{'hour_local'},$ad{'minute_local'},$ad{'tz_local'});-}else{-printf(" (%02d:%02d %s)",-$ad{'hour_local'},$ad{'minute_local'},$ad{'tz_local'});+if($opts{-localtime}){+if($ad{'hour_local'}<6){+printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",+$ad{'hour_local'},$ad{'minute_local'},$ad{'tz_local'});+}else{+printf(" (%02d:%02d %s)",+$ad{'hour_local'},$ad{'minute_local'},$ad{'tz_local'});+}+}+print"]</$tag>\n";+}++# Outputs table rows containing the full author or committer information,+# in the format expected for 'commit' view (& similia).+# Parameters are a commit hash reference, followed by the list of people+# to output information for. If the list is empty it defalts to both+# author and committer.+subgit_print_authorship_rows{+my$co=shift;+# too bad we can't use @people = @_ || ('author', 'committer')+my@people=@_;+@people=('author','committer')unless@people;+foreachmy$who(@people){+my%ad=parse_date($co->{$who.'_epoch'},$co->{$who.'_tz'});+print"<tr><td>$who</td><td>".esc_html($co->{$who})."</td></tr>\n".+"<tr>".+"<td></td><td> $ad{'rfc2822'}";+if($ad{'hour_local'}<6){+printf(" (<span class=\"atnight\">%02d:%02d</span> %s)",+$ad{'hour_local'},$ad{'minute_local'},$ad{'tz_local'});+}else{+printf(" (%02d:%02d %s)",+$ad{'hour_local'},$ad{'minute_local'},$ad{'tz_local'});+}+print"</td>".+"</tr>\n";}-print"]</div>\n";}subgit_print_page_path{
@@ -4142,11 +4184,9 @@ sub git_shortlog_body {print"<tr class=\"light\">\n";}$alternate^=1;-my$author=chop_and_escape_str($co{'author_name'},10);# git_summary() used print "<td><i>$co{'age_string'}</i></td>\n" .print"<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n".-"<td><i>".$author."</i></td>\n".-"<td>";+format_author_html('td',\%co,10)."<td>";printformat_subject_html($co{'title'},$co{'title_short'},href(action=>"commit",hash=>$commit),$ref);print"</td>\n".
@@ -4193,11 +4233,9 @@ sub git_history_body {print"<tr class=\"light\">\n";}$alternate^=1;-# shortlog uses chop_str($co{'author_name'}, 10)-my$author=chop_and_escape_str($co{'author_name'},15,3);print"<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n".-"<td><i>".$author."</i></td>\n".-"<td>";+# shortlog: format_author_html('td', \%co, 10)+format_author_html('td',\%co,15,3)."<td>";# originally git_history used chop_str($co{'title'}, 50)printformat_subject_html($co{'title'},$co{'title_short'},href(action=>"commit",hash=>$commit),$ref);
@@ -4350,9 +4388,8 @@ sub git_search_grep_body {print"<tr class=\"light\">\n";}$alternate^=1;-my$author=chop_and_escape_str($co{'author_name'},15,5);print"<td title=\"$co{'age_string_age'}\"><i>$co{'age_string_date'}</i></td>\n".-"<td><i>".$author."</i></td>\n".+format_author_html('td',\%co,15,5)."<td>".$cgi->a({-href=>href(action=>"commit",hash=>$co{'id'}),-class=>"list subject"},
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:46:59
parse_tag has to be adapted to use the hash keys expected by
git_print_authorship_rows, which is not a problem since git_tag
is the only user of this sub.
Signed-off-by: Giuseppe Bilotta <redacted>
---
gitweb/gitweb.perl | 16 +++++++++-------
1 files changed, 9 insertions(+), 7 deletions(-)
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:46:59
Switch from
A U Thor [off-list ref] [date time]
to
author A U Thor [off-list ref]
date time
committer C O Mitter [off-list ref]
committer date time
Signed-off-by: Giuseppe Bilotta <redacted>
---
gitweb/gitweb.perl | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:46:59
Views which contain many occurrences of the same email address (e.g.
shortlog view) benefit from not having to recalculate the MD5 of the
email address every time.
Signed-off-by: Giuseppe Bilotta <redacted>
---
gitweb/gitweb.perl | 30 +++++++++++++++++++++++++++---
1 files changed, 27 insertions(+), 3 deletions(-)
@@ -1515,6 +1515,27 @@ sub format_subject_html {}}+# 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).+# The cache is shared by all avatar engines (currently gravatar only), which+# are free to use it as preferred. Since only one avatar engine is used for any+# given page, there's no risk for cache conflicts.+our%avatar_cache=();++# Compute the gravatar url for a given email, if it's not in the cache already.+# Gravatar stores only the part of the URL before the size, since that's the+# one computationally more expensive. This also allows reuse of the cache for+# different sizes (for this particular engine).+subgravatar_url{+my$email=lcshift;+my$size=shift;+$avatar_cache{$email}||=+"http://www.gravatar.com/avatar.php?gravatar_id=".+Digest::MD5::md5_hex($email)."&size=";+return$avatar_cache{$email}.$size;+}+# Insert an avatar for the given $email at the given $size if the feature# is enabled.subgit_get_avatar{
@@ -1524,15 +1545,18 @@ sub git_get_avatar {my$size=$avatar_size{$opts{'size'}}||$avatar_size{'default'};my$url="";if($git_avatareq'gravatar'){-$url="http://www.gravatar.com/avatar.php?gravatar_id=".-Digest::MD5::md5_hex(lc$email)."&size=$size";+$url=gravatar_url($email,$size);}# Currently only gravatars are supported, but other forms such as# picons can be added by putting an else up here and defining $url# as needed. If no variant puts something in $url, we assume avatars# are completely disabled/unavailable.if($url){-return$pre_white."<img width=\"$size\" class=\"avatar\" src=\"$url\" />".$post_white;+return$pre_white.+"<img width=\"$size\" ".+"class=\"avatar\" ".+"src=\"$url\" ".+"/>".$post_white;}else{return"";}
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:46:59
Introduce avatar support: the feature adds the appropriate img tag next
to author and committer in commit(diff), history, shortlog, log and tag
views. Multiple avatar providers are possible, but only gravatar is
implemented at the moment.
Gravatar support depends on Digest::MD5, which is a core package since
Perl 5.8. If gravatars are activated but Digest::MD5 cannot be found,
the feature will be automatically disabled.
No avatar provider is selected by default, except in the t9500 test.
Signed-off-by: Giuseppe Bilotta <redacted>
---
gitweb/gitweb.css | 4 ++
gitweb/gitweb.perl | 81 ++++++++++++++++++++++++++++++-
t/t9500-gitweb-standalone-no-errors.sh | 2 +
3 files changed, 84 insertions(+), 3 deletions(-)
@@ -195,6 +195,14 @@ our %known_snapshot_format_aliases = ('x-zip'=>undef,''=>undef,);+# Pixel sizes for icons and avatars. If the default font sizes or lineheights+# are changed, it may be appropriate to change these values too via+# $GITWEB_CONFIG.+our%avatar_size=(+'default'=>16,+'double'=>32+);+# You define site-wide feature defaults here; override them with# $GITWEB_CONFIG as necessary.our%feature=(
@@ -365,6 +373,23 @@ our %feature = ('sub'=>\&feature_patches,'override'=>0,'default'=>[16]},++# Avatar support. When this feature is enabled, views such as+# shortlog or commit will display an avatar associated with+# the email of the committer(s) and/or author(s).++# Currently only the gravatar provider is available, and it+# depends on Digest::MD5.++# To enable system wide have in $GITWEB_CONFIG+# $feature{'avatar'}{'default'} = ['gravatar'];+# To have project specific config enable override in $GITWEB_CONFIG+# $feature{'avatar'}{'override'} = 1;+# and in project config gitweb.avatar = gravatar;+'avatar'=>{+'sub'=>\&feature_avatar,+'override'=>0,+'default'=>['']},);subgitweb_get_feature{
@@ -433,6 +458,16 @@ sub feature_patches {return($_[0]);}+subfeature_avatar{+my@val=(git_get_project_config('avatar'));++if(@val){+return@val;+}++return@_;+}+# checking HEAD file with -e is fragile if the repository was# initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed# and then pruned.
@@ -814,6 +849,17 @@ $git_dir = "$projectroot/$project" if $project;our@snapshot_fmts=gitweb_get_feature('snapshot');@snapshot_fmts=filter_snapshot_fmts(@snapshot_fmts);+# check that the avatar feature is set to a known provider name,+# and for each provider check if the dependencies are satisfied.+# if the provider name is invalid or the dependencies are not met,+# reset $git_avatar to the empty string.+our($git_avatar)=gitweb_get_feature('avatar');+if($git_avatareq'gravatar'){+$git_avatar=''unless(eval{requireDigest::MD5;1;});+}else{+$git_avatar='';+}+# dispatchif(!defined$action){if(defined$hash){
@@ -1469,6 +1515,29 @@ sub format_subject_html {}}+# Insert an avatar for the given $email at the given $size if the feature+# is enabled.+subgit_get_avatar{+my($email,%opts)=@_;+my$pre_white=($opts{'pad_before'}?" ":"");+my$post_white=($opts{'pad_after'}?" ":"");+my$size=$avatar_size{$opts{'size'}}||$avatar_size{'default'};+my$url="";+if($git_avatareq'gravatar'){+$url="http://www.gravatar.com/avatar.php?gravatar_id=".+Digest::MD5::md5_hex(lc$email)."&size=$size";+}+# Currently only gravatars are supported, but other forms such as+# picons can be added by putting an else up here and defining $url+# as needed. If no variant puts something in $url, we assume avatars+# are completely disabled/unavailable.+if($url){+return$pre_white."<img width=\"$size\" class=\"avatar\" src=\"$url\" />".$post_white;+}else{+return"";+}+}+# format the author name of the given commit with the given tag# the author name is chopped and escaped according to the other# optional parameters (see chop_str).
@@ -1476,7 +1545,9 @@ sub format_author_html {my$tag=shift;my$co=shift;my$author=chop_and_escape_str($co->{'author_name'},@_);-return"<$tag class=\"author\">".$author."</$tag>";+return"<$tag class=\"author\">".+git_get_avatar($co->{'author_email'},'pad_after'=>1).+$author."</$tag>";}# format git diff header line, i.e. "diff --(git|combined|cc) ..."
@@ -3249,7 +3320,8 @@ sub git_print_authorship {$ad{'hour_local'},$ad{'minute_local'},$ad{'tz_local'});}}-print"]</$tag>\n";+print"]".git_get_avatar($co->{'author_email'},'pad_before'=>1)+."</$tag>\n";}# Outputs table rows containing the full author or committer information,
@@ -3264,7 +3336,10 @@ sub git_print_authorship_rows {@people=('author','committer')unless@people;foreachmy$who(@people){my%ad=parse_date($co->{$who.'_epoch'},$co->{$who.'_tz'});-print"<tr><td>$who</td><td>".esc_html($co->{$who})."</td></tr>\n".+print"<tr><td>$who</td><td>".esc_html($co->{$who})."</td>".+"<td rowspan=\"2\">".+git_get_avatar($co->{$who.'_email'},'size'=>'double').+"</td></tr>\n"."<tr>"."<td></td><td> $ad{'rfc2822'}";if($ad{'hour_local'}<6){
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:46:59
The alt text, set to the avatar provider (i.e. image type), is more
friendly than the img url in some text-only browsers.
---
gitweb/gitweb.perl | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:46:59
Simple implementation of picon that only relies on the indiana.edu
database.
Signed-off-by: Giuseppe Bilotta <redacted>
---
gitweb/gitweb.perl | 26 ++++++++++++++++++++++----
1 files changed, 22 insertions(+), 4 deletions(-)
@@ -378,14 +378,17 @@ our %feature = (# shortlog or commit will display an avatar associated with# the email of the committer(s) and/or author(s).-# Currently only the gravatar provider is available, and it-# depends on Digest::MD5.+# Currently available providers are gravatar and picon.++# Gravatar depends on Digest::MD5.+# Picon currently relies on the indiana.edu database.# To enable system wide have in $GITWEB_CONFIG-# $feature{'avatar'}{'default'} = ['gravatar'];+# $feature{'avatar'}{'default'} = ['provider'];+# where provider is either gravatar or picon.# To have project specific config enable override in $GITWEB_CONFIG# $feature{'avatar'}{'override'} = 1;-# and in project config gitweb.avatar = gravatar;+# and in project config gitweb.avatar = provider;'avatar'=>{'sub'=>\&feature_avatar,'override'=>0,
@@ -856,6 +859,8 @@ our @snapshot_fmts = gitweb_get_feature('snapshot');our($git_avatar)=gitweb_get_feature('avatar');if($git_avatareq'gravatar'){$git_avatar=''unless(eval{requireDigest::MD5;1;});+}elsif($git_avatareq'picon'){+# no dependencies}else{$git_avatar='';}
@@ -1523,6 +1528,17 @@ sub format_subject_html {# given page, there's no risk for cache conflicts.our%avatar_cache=();+# Compute the picon url for a given email, by using the picon search service over at+# http://www.cs.indiana.edu/picons/search.html+subpicon_url{+my$email=lcshift;+if(!$avatar_cache{$email}){+my($user,$domain)=split('@',$email);+$avatar_cache{$email}="http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/single";+}+return$avatar_cache{$email};+}+# Compute the gravatar url for a given email, if it's not in the cache already.# Gravatar stores only the part of the URL before the size, since that's the# one computationally more expensive. This also allows reuse of the cache for
@@ -1546,6 +1562,8 @@ sub git_get_avatar {my$url="";if($git_avatareq'gravatar'){$url=gravatar_url($email,$size);+}elsif($git_avatareq'picon'){+$url=picon_url($email);}# Currently only gravatars are supported, but other forms such as# picons can be added by putting an else up here and defining $url
From: Jakub Narebski <hidden> Date: 2016-06-15 22:46:59
On Sat, 27 June 2009, Giuseppe Bilotta wrote:
Collect all author display code in appropriate functions, making it
easier to extend these functions on the CGI side.
We also move some of the presentation code from hard-coded HTML to CSS,
for easier customization.
Very nicely written, compact yet comprehensive commit message.
It _might_ be worth adding that accidentally commit date got "atnight"
warning too. I consider that improvement, even though for local commit
(committer == author) it duplicates information. And it removes
(some of) code duplication.
Signed-off-by: Giuseppe Bilotta <redacted>
If not for the fact that due to typo you lost localtime info from
'commitdiff' view, I would ACK it.
@@ -1469,6 +1469,16 @@ sub format_subject_html {}}+# format the author name of the given commit with the given tag+# the author name is chopped and escaped according to the other+# optional parameters (see chop_str).+subformat_author_html{+my$tag=shift;+my$co=shift;+my$author=chop_and_escape_str($co->{'author_name'},@_);+return"<$tag class=\"author\">".$author."</$tag>";+}
In the future we might want to change it so instead of
<$tag class="author"><span title="Joe Random Hacker">Joe Random... </span></$tag>
we would use 'title' attribute of $tag element
<$tag class="author" title="Joe Random Hacker">Joe Random... </$tag>
But I guess this is not worth complicating code required to do this.
quoted hunk
+
# format git diff header line, i.e. "diff --(git|combined|cc) ..."
sub format_git_diff_header_line {
my $line = shift;
@@ -3214,21 +3224,53 @@ sub git_print_header_div { "\n</div>\n"; }+# Outputs the author name and date in long form sub git_print_authorship { my $co = shift;+ my %opts = @_;+ my $tag = $opts{-tag} || 'div';
BTW. gitweb is a bit inconsistent here. Sometimes we use $opts{'-tag'}
form, and sometimes $opts{-tag} form like you use here.
(Actually we use '-' prefixing i.e. '-key' not 'key' to make it safe
to write $opts{-key} and -key => 1).
By the way, this "atnight" warning is duplicated in subroutine below
git_print_authorship_rows(), but IMHO it is not something very important.
It can be left for later.
+ print "]</$tag>\n";
+}
+
+# Outputs table rows containing the full author or committer information,
+# in the format expected for 'commit' view (& similia).
+# Parameters are a commit hash reference, followed by the list of people
+# to output information for. If the list is empty it defalts to both
+# author and committer.
+sub git_print_authorship_rows {
+ my $co = shift;
+ # too bad we can't use @people = @_ || ('author', 'committer')
+ my @people = @_;
+ @people = ('author', 'committer') unless @people;
+ foreach my $who (@people) {
+ my %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});
Wouldn't
$co->{"${who}_epoch"}
be simpler than
$co->{$who . '_epoch'}
Also it is now %wd (%who_date) or just %date rather than
%ad (%author_date) vs %cd (%committer_date).
Both of those issues are very, very minor.
Nice simplification. Even without making 'commitdiff' use the same
authorship info layout like 'commit', and 'tag' reusing this subroutine
it might have been worth it.
ATTENTION !!!
You should have
- git_print_authorship(\%co);
+ git_print_authorship(\%co, '-localtime' => 1);
here ('-localtime' instead of 'localtime'), otherwise you will loose
localtime information (and "atnight" warning) from 'commitdiff' view.
Thanks again for your diligent work on this patch series!
--
Jakub Narebski
Poland
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:46:59
2009/6/27 Jakub Narebski [off-list ref]:
By the way, this "atnight" warning is duplicated in subroutine below
git_print_authorship_rows(), but IMHO it is not something very important.
It can be left for later.
I'll resend this patch, with this thing refactored too, and with the
localtime fix.
quoted
+ foreach my $who (@people) {
+ my %ad = parse_date($co->{$who . '_epoch'}, $co->{$who . '_tz'});
Wouldn't
$co->{"${who}_epoch"}
be simpler than
$co->{$who . '_epoch'}
Also it is now %wd (%who_date) or just %date rather than
%ad (%author_date) vs %cd (%committer_date).
Both of those issues are very, very minor.
But I can tune them out 8-)
Nice... although this set of diff chunks made me wonder why we
use "10" for 'shortlog', "15, 3" for 'history, and "15, 5" for
'search' (grep).
I don't know about that. It might be worth considering unifying them.
Thanks again for your diligent work on this patch series!
Thank you very much for your comments. I could clearly see the code
improving significantly at each iteration. It's a great sastisfaction,
even though it's quite some work.
--
Giuseppe "Oblomov" Bilotta
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:46:59
On Sat, Jun 27, 2009 at 5:26 PM, Giuseppe
Bilotta[off-list ref] wrote:
2009/6/27 Jakub Narebski [off-list ref]:
quoted
By the way, this "atnight" warning is duplicated in subroutine below
git_print_authorship_rows(), but IMHO it is not something very important.
It can be left for later.
I'll resend this patch, with this thing refactored too, and with the
localtime fix.
From: Jakub Narebski <hidden> Date: 2016-06-15 22:46:59
On Sat, 27 June 2009, Giuseppe Bilotta wrote:
Switch from
A U Thor [off-list ref] [date time]
to
author A U Thor [off-list ref]
date time
committer C O Mitter [off-list ref]
committer date time
I would use:
Switch from form similar to the one used by 'log' view
A U Thor [off-list ref] [date time]
to the form used in 'commit' view
author A U Thor [off-list ref]
date time
committer C O Mitter [off-list ref]
date time
(i.e. use spaces and not tabs to align). But this is minor
issue, not worth worrying about IMVHO.
Signed-off-by: Giuseppe Bilotta <redacted>
I am still not sure about this change. On one hand side it is unifying
of 'commit' and 'commitdiff' view; most other web interfaces have single
view equivalent to git-show, which displays both commit info, and the
diff. (And 'commitdiff' with 'hp' parameter i.e. between two commits
has to be redesigned anyway, so this issue doesn't enter this
consideration).
On the other hand side IIRC 'commitdiff' uses short (one-line)
authorship info because the main point is the diff, and multi-line
author and commit info like the one used in 'commit' view takes
a bit of vertical space. Also one can use similarity between
'log' and 'commitdiff' views (git-log and git-show) as a counter
for argument that 'commitdiff' has to look like 'commit'.
But otherwise I quite like this patch.
Nice and short, thanks to earlier (re)factoring.
BTW. after this change the -localtime part of git_print_authorship()
subroutine is unused... just saying ;-) Not something terribly
important.
--
Jakub Narebski
Poland
From: Jakub Narebski <hidden> Date: 2016-06-15 22:46:59
On Sat, 27 June 2009, Giuseppe Bilotta wrote:
parse_tag has to be adapted to use the hash keys expected by
git_print_authorship_rows, which is not a problem since git_tag
is the only user of this sub.
Nitpick: I think that with s/has/had/ (past tense) and
s/, which/. This/ (split paragraph into two sentences)
it would read better.
Sidenote: I wonder if it would be worth doing to factor out code dealing
with extracting data from (parsing) author/committer/tagger object headers.
Probably not worth it...
From: Jakub Narebski <hidden> Date: 2016-06-15 22:46:59
On Sat, 27 June 2009, Giuseppe Bilotta wrote:
Signed-off-by: Giuseppe Bilotta <redacted>
I don't like it. In my opinion it is less readable that way, especially
if word wrapping gets involved:
5 hours |
ago |
5 hours |
ago |
5 days |
ago |
It is IMVHO a bit ugly.
That aside this is *not complete*. Take a look at 'summary' view.
You have there 'date' column in 'shortlog' section aligned to the right,
while in 'heads' and 'tags' section it is aligned to the left. Add to
that "Last Change" column in projects list view (which should probably
be aligned to the left, even with this patch completed).
NAK from me. (Alternate stylesheet? Just kidding...)
From: Jakub Narebski <hidden> Date: 2016-06-15 22:47:00
On Sat, 27 June 2009, Giuseppe Bilotta wrote:
Introduce avatar support: the feature adds the appropriate img tag next
to author and committer in commit(diff), history, shortlog, log and tag
views. Multiple avatar providers are possible, but only gravatar is
implemented at the moment.
Gravatar support depends on Digest::MD5, which is a core package since
Perl 5.8. If gravatars are activated but Digest::MD5 cannot be found,
the feature will be automatically disabled.
No avatar provider is selected by default, except in the t9500 test.
@@ -195,6 +195,14 @@ our %known_snapshot_format_aliases = ('x-zip'=>undef,''=>undef,);+# Pixel sizes for icons and avatars. If the default font sizes or lineheights+# are changed, it may be appropriate to change these values too via+# $GITWEB_CONFIG.+our%avatar_size=(+'default'=>16,+'double'=>32+);
Nice. Good and complete explanation.
[...]
+ # Avatar support. When this feature is enabled, views such as
+ # shortlog or commit will display an avatar associated with
+ # the email of the committer(s) and/or author(s).
+
+ # Currently only the gravatar provider is available, and it
+ # depends on Digest::MD5.
+
+ # To enable system wide have in $GITWEB_CONFIG
+ # $feature{'avatar'}{'default'} = ['gravatar'];
+ # To have project specific config enable override in $GITWEB_CONFIG
+ # $feature{'avatar'}{'override'} = 1;
+ # and in project config gitweb.avatar = gravatar;
+ 'avatar' => {
+ 'sub' => \&feature_avatar,
+ 'override' => 0,
+ 'default' => ['']},
);
So you have chosen [''] and not [] as "no avatar" value, to simplify
later code. All right.
quoted hunk
sub gitweb_get_feature {
@@ -433,6 +458,16 @@ sub feature_patches { return ($_[0]); }+sub feature_avatar {+ my @val = (git_get_project_config('avatar'));++ if (@val) {+ return @val;+ }++ return @_;+}
Hmmm... why not simply
+ return @val ? @val : @_;
By the way, we might want to accept 'none' instead of empty value
or no value to turn off avatar support for specific project (if
avatars are turned on globally, and project specific override is on).
We use this technique for 'snapshot' feature.
But this isn't terribly important.
[...]
quoted hunk
@@ -814,6 +849,17 @@ $git_dir = "$projectroot/$project" if $project; our @snapshot_fmts = gitweb_get_feature('snapshot'); @snapshot_fmts = filter_snapshot_fmts(@snapshot_fmts);+# check that the avatar feature is set to a known provider name,+# and for each provider check if the dependencies are satisfied.+# if the provider name is invalid or the dependencies are not met,+# reset $git_avatar to the empty string.+our ($git_avatar) = gitweb_get_feature('avatar');+if ($git_avatar eq 'gravatar') {+ $git_avatar = '' unless (eval { require Digest::MD5; 1; });+} else {+ $git_avatar = '';+}
Thoughts for the future: this can lead to not very pretty if-elsif
chain. We have replaces such chain for selecting action (for dispatch)
by using %actions hash of subroutine refs (as a kind of 'switch'/'given'
statement). We could do the same for avatar provider initialization
and validation subroutines.
But for now it is clear enough. Don't worry about this issue now.
quoted hunk
@@ -1469,6 +1515,29 @@ sub format_subject_html { } }+# Insert an avatar for the given $email at the given $size if the feature+# is enabled.+sub git_get_avatar {+ my ($email, %opts) = @_;+ my $pre_white = ($opts{'pad_before'} ? " " : "");+ my $post_white = ($opts{'pad_after'} ? " " : "");+ my $size = $avatar_size{$opts{'size'}} || $avatar_size{'default'};
Running t9500-gitweb-standalone-no-errors.sh with --debug option shows
failing of 15 among 87 tests, with error:
gitweb.perl: Use of uninitialized value in hash element at gitweb/gitweb.perl line 1524.
which is the above line. This line should read:
+ my $size = exists $opts{'size'}
+ ? $avatar_size{$opts{'size'}} || $avatar_size{'default'}
+ : $avatar_size{'default'};
or something like that, e.g.:
+ my $size = $avatar_size{ defined $opts{'size'} ? $opts{'size'} : 'default' }
+ || $avatar_size{'default'};
BTW. didn't we agree on '-size' and '-pad_before' convention?
+ }
+ # Currently only gravatars are supported, but other forms such as
+ # picons can be added by putting an else up here and defining $url
+ # as needed. If no variant puts something in $url, we assume avatars
+ # are completely disabled/unavailable.
+ if ($url) {
+ return $pre_white . "<img width=\"$size\" class=\"avatar\" src=\"$url\" />" . $post_white;
+ } else {
+ return "";
+ }
+}
Good idea, and nice description.
[...]
quoted hunk
@@ -1476,7 +1545,9 @@ sub format_author_html { my $tag = shift; my $co = shift; my $author = chop_and_escape_str($co->{'author_name'}, @_);- return "<$tag class=\"author\">" . $author . "</$tag>";+ return "<$tag class=\"author\">" .+ git_get_avatar($co->{'author_email'}, 'pad_after' => 1) .+ $author . "</$tag>"; } # format git diff header line, i.e. "diff --(git|combined|cc) ..."
@@ -3249,7 +3320,8 @@ sub git_print_authorship { $ad{'hour_local'}, $ad{'minute_local'}, $ad{'tz_local'}); } }- print "]</$tag>\n";+ print "]" . git_get_avatar($co->{'author_email'}, 'pad_before' => 1)+ . "</$tag>\n"; } # Outputs table rows containing the full author or committer information,
@@ -671,6 +672,7 @@ test_expect_success \'config override: tree view, features disabled in repo config'\'gitconfiggitweb.blameno&&gitconfiggitweb.snapshotnone&&+gitconfiggitweb.avatargravatar&&gitweb_run"p=.git;a=tree"' test_debug'cat gitweb.log'
Please run it too :-)
Apart from this issue of Perl warning (which would get logged, but
otherwise is not an error), and using old Gravatar API, this patch
shapes very nicely, thanks to all the work done earlier on refactoring.
I like it very much.
--
Jakub Narebski
Poland
From: Jakub Narebski <hidden> Date: 2016-06-15 22:47:00
On Sat, 27 June 2009, Giuseppe Bilotta wrote:
Views which contain many occurrences of the same email address (e.g.
shortlog view) benefit from not having to recalculate the MD5 of the
email address every time.
Signed-off-by: Giuseppe Bilotta <redacted>
Acked-by: Jakub Narebski <redacted>
---
Here there are some very simple benchmarks of the effect of gravatar
URL cache. I'm not sure if they should be put in commit message.
When running gitweb as a script
$ time gitweb-run.sh "p=git.git;a=shortlog" >/dev/null
I got the following results
| before | after
------------+-----------------|------------------
summary | 0.978s (0.964s) | 0.961s (0.940s)
shortlog | 1.001s (0.980s) | 1.033s (1.008s)
which means the same results within the margin of error, (as seen when
repeating benchmark the differences are within the range of fluctuations).
Time is real (wallclock) time, in parentheses there is user + sys time.
When using ApacheBench 2.0.41-dev, with gitweb run as CGI script
(not as legacy, i.e. ModPerl::Registry, mod_perl script)
$ ab <opt> "http://localhost/cgi-bin/gitweb/gitweb.cgi/git.git/shortlog"
I got the folowing results
| before | after
------------+-----------------|------------------
-n 10 | 1094.050 [ms] | 989.136 [ms]
-n 10 -c 2 | 1147.965 [ms] | 1041.324 [ms]
which means around 10% improvement. Standard deviation is around 10 ms
for no concurrency, and around 200 ms (more than third of difference)
for concurrent connections. Shown here is time per request (mean,
across all concurrent requests).
@@ -1515,6 +1515,27 @@ sub format_subject_html {}}+# 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).+# The cache is shared by all avatar engines (currently gravatar only), which+# are free to use it as preferred. Since only one avatar engine is used for any+# given page, there's no risk for cache conflicts.+our%avatar_cache=();
Good comment.
+
+# Compute the gravatar url for a given email, if it's not in the cache already.
+# Gravatar stores only the part of the URL before the size, since that's the
+# one computationally more expensive. This also allows reuse of the cache for
+# different sizes (for this particular engine).
+sub gravatar_url {
+ my $email = lc shift;
+ my $size = shift;
+ $avatar_cache{$email} ||=
+ "http://www.gravatar.com/avatar.php?gravatar_id=" .
+ Digest::MD5::md5_hex($email) . "&size=";
The same comment as for previous patch: why not use modern API?
+ "http://www.gravatar.com/avatar/" .
+ Digest::MD5::md5_hex($email) . "?size=";
+ return $avatar_cache{$email} . $size;
+}
Nice.
quoted hunk
+
# Insert an avatar for the given $email at the given $size if the feature
# is enabled.
sub git_get_avatar {
@@ -1524,15 +1545,18 @@ sub git_get_avatar { my $size = $avatar_size{$opts{'size'}} || $avatar_size{'default'}; my $url = ""; if ($git_avatar eq 'gravatar') {- $url = "http://www.gravatar.com/avatar.php?gravatar_id=" .- Digest::MD5::md5_hex(lc $email) . "&size=$size";+ $url = gravatar_url($email, $size); }
Great encapsulation.
# Currently only gravatars are supported, but other forms such as
# picons can be added by putting an else up here and defining $url
# as needed. If no variant puts something in $url, we assume avatars
# are completely disabled/unavailable.
if ($url) {
- return $pre_white . "<img width=\"$size\" class=\"avatar\" src=\"$url\" />" . $post_white;
+ return $pre_white .
+ "<img width=\"$size\" " .
+ "class=\"avatar\" " .
+ "src=\"$url\" " .
+ "/>" . $post_white;
This is independent change. It shouldn't be here; if you prefer such
solution, you should squash it with previous patch. Please drop this
chunk.
} else {
return "";
}
--
Thanks again for great work on this series!
--
Jakub Narebski
Poland
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:47:00
2009/6/27 Jakub Narebski [off-list ref]:
On Sat, 27 June 2009, Giuseppe Bilotta wrote:
quoted
parse_tag has to be adapted to use the hash keys expected by
git_print_authorship_rows, which is not a problem since git_tag
is the only user of this sub.
Nitpick: I think that with s/has/had/ (past tense) and
s/, which/. This/ (split paragraph into two sentences)
it would read better.
I can do that.
Sidenote: I wonder if it would be worth doing to factor out code dealing
with extracting data from (parsing) author/committer/tagger object headers.
Probably not worth it...
It might be worth it, but I'd say we can leave it to some other time ;-)
I wonder why we used and use 'author' instead of 'tagger'...
The upside of using 'author' is that the tag can then be dealt with
just like any other commit by the rest of the code, I think. I don't
know if this benefit is actually used anywhere in the code.
--
Giuseppe "Oblomov" Bilotta
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:47:00
2009/6/27 Jakub Narebski [off-list ref]:
On Sat, 27 June 2009, Giuseppe Bilotta wrote:
quoted
Signed-off-by: Giuseppe Bilotta <redacted>
I don't like it. In my opinion it is less readable that way, especially
if word wrapping gets involved:
5 hours |
ago |
5 hours |
ago |
5 days |
ago |
It is IMVHO a bit ugly.
You might have a point there.
That aside this is *not complete*. Take a look at 'summary' view.
You have there 'date' column in 'shortlog' section aligned to the right,
while in 'heads' and 'tags' section it is aligned to the left. Add to
that "Last Change" column in projects list view (which should probably
be aligned to the left, even with this patch completed).
NAK from me. (Alternate stylesheet? Just kidding...)
[...]
On the other hand: it is short and simple.
And totally independent from the rest of the patchset. I'll move it to
the top of the stack, so that the rest of the next iteration can be
applied in batch as soon as it's ready.
--
Giuseppe "Oblomov" Bilotta
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:47:00
2009/6/27 Jakub Narebski [off-list ref]:
This is well written commit message. Good work!
Thanks 8-)
quoted
+ # Avatar support. When this feature is enabled, views such as
+ # shortlog or commit will display an avatar associated with
+ # the email of the committer(s) and/or author(s).
+
+ # Currently only the gravatar provider is available, and it
+ # depends on Digest::MD5.
+
+ # To enable system wide have in $GITWEB_CONFIG
+ # $feature{'avatar'}{'default'} = ['gravatar'];
+ # To have project specific config enable override in $GITWEB_CONFIG
+ # $feature{'avatar'}{'override'} = 1;
+ # and in project config gitweb.avatar = gravatar;
+ 'avatar' => {
+ 'sub' => \&feature_avatar,
+ 'override' => 0,
+ 'default' => ['']},
);
So you have chosen [''] and not [] as "no avatar" value, to simplify
later code. All right.
By the way, we might want to accept 'none' instead of empty value
or no value to turn off avatar support for specific project (if
avatars are turned on globally, and project specific override is on).
We use this technique for 'snapshot' feature.
But this isn't terribly important.
Well, since 'none' is not a known provider, that trick should work
correctly anyway.
Thoughts for the future: this can lead to not very pretty if-elsif
chain. We have replaces such chain for selecting action (for dispatch)
by using %actions hash of subroutine refs (as a kind of 'switch'/'given'
statement). We could do the same for avatar provider initialization
and validation subroutines.
But for now it is clear enough. Don't worry about this issue now.
I'm also not terribly sure about how to implement _this_ through hash
calls. But I did consider the issue (how I wish Perl had an actual
switch statement ...)
Running t9500-gitweb-standalone-no-errors.sh with --debug option shows
failing of 15 among 87 tests, with error:
gitweb.perl: Use of uninitialized value in hash element at gitweb/gitweb.perl line 1524.
which is the above line. This line should read:
+ my $size = exists $opts{'size'}
+ ? $avatar_size{$opts{'size'}} || $avatar_size{'default'}
+ : $avatar_size{'default'};
or something like that, e.g.:
+ my $size = $avatar_size{ defined $opts{'size'} ? $opts{'size'} : 'default' }
+ || $avatar_size{'default'};
I did $opts{-size} ||= 'default'. Or is it bad form to modify the
passed option hash?
BTW. didn't we agree on '-size' and '-pad_before' convention?
Yes we did. I don't know why it went back to 'size' etc. Must have
messed up something with the patchsets.
From: Jakub Narebski <hidden> Date: 2016-06-15 22:47:00
On Sun, 28 Jun 2009, Giuseppe Bilotta wrote:
2009/6/27 Jakub Narebski [off-list ref]:
quoted
quoted
+ # Avatar support. When this feature is enabled, views such as
+ # shortlog or commit will display an avatar associated with
+ # the email of the committer(s) and/or author(s).
+
+ # Currently only the gravatar provider is available, and it
+ # depends on Digest::MD5.
[...]
quoted
By the way, we might want to accept 'none' instead of empty value
or no value to turn off avatar support for specific project (if
avatars are turned on globally, and project specific override is on).
We use this technique for 'snapshot' feature.
But this isn't terribly important.
Well, since 'none' is not a known provider, that trick should work
correctly anyway.
Ah, I have forgot about this. Perhaps it should be stated more
explicitly that unknown provider (type) = no avatar image? Or do I
simply not see it?
Thoughts for the future: this can lead to not very pretty if-elsif
chain. We have replaces such chain for selecting action (for dispatch)
by using %actions hash of subroutine refs (as a kind of 'switch'/'given'
statement). We could do the same for avatar provider initialization
and validation subroutines.
But for now it is clear enough. Don't worry about this issue now.
I'm also not terribly sure about how to implement _this_ through hash
calls. But I did consider the issue (how I wish Perl had an actual
switch statement ...)
Perl 5.10, Perl 6, Switch.pm... but it isn't something that we can use
in gitweb. Switch statement (spelled 'given / when') in Perl6 / Perl 5.10
is very, very powerfull (from what I have read).
But what I meant here is something like the following:
our %avatar_providers = (
'gravatar' => sub { eval { require Digest::MD5; 1; } },
'picon' => sub { 1; },
);
unless (defined $avatar_providers{$git_avatar} &&
$avatar_providers{$git_avatar}->()) {
$git_avatar = '';
}
Or something like that (we can use 'undef' for providers without need
for initialization). But that can be improved later, "in tree".
On the other hand side this solution wouldn't I think be able to deal
with specifying fallbacks using e.g. 'gravatar+picon' (gravatar using
picon URL as fallback i.e. "default" argument), or two images side by
side like GMane uses (if there are two), e.g. 'gravatar picon' or
'gravatar, picon'.
Food for thought. Not something to act on now.
quoted
Running t9500-gitweb-standalone-no-errors.sh with --debug option shows
failing of 15 among 87 tests, with error:
gitweb.perl: Use of uninitialized value in hash element at gitweb/gitweb.perl line 1524.
which is the above line. This line should read:
+ my $size = exists $opts{'size'}
+ ? $avatar_size{$opts{'size'}} || $avatar_size{'default'}
+ : $avatar_size{'default'};
or something like that, e.g.:
+ my $size = $avatar_size{ defined $opts{'size'} ? $opts{'size'} : 'default' }
+ || $avatar_size{'default'};
I did $opts{-size} ||= 'default'. Or is it bad form to modify the
passed option hash?
No, it is O.K. We do similar thing in href(), deleting keys from hash.
From: Jakub Narebski <hidden> Date: 2016-06-15 22:47:00
On Sat, 27 June 2009, Giuseppe Bilotta wrote:
Simple implementation of picon that only relies on the indiana.edu
database.
I'd like to see where you got information about picons. But it is
not necessary to have in commit message.
... Ah, I'm sorry, it is stated in comment. Nevermind then.
@@ -378,14 +378,17 @@ our %feature = (# shortlog or commit will display an avatar associated with# the email of the committer(s) and/or author(s).-# Currently only the gravatar provider is available, and it-# depends on Digest::MD5.+# Currently available providers are gravatar and picon.++# Gravatar depends on Digest::MD5.+# Picon currently relies on the indiana.edu database.# To enable system wide have in $GITWEB_CONFIG-# $feature{'avatar'}{'default'} = ['gravatar'];+# $feature{'avatar'}{'default'} = ['provider'];+# where provider is either gravatar or picon.
I wonder if it wouldn't be better to use "<provider>" (or "'<provider>'")
in place of "'provider'", as it is not about literal 'provider', but
about one of 'gravatar' or 'picon'.
# To have project specific config enable override in $GITWEB_CONFIG
# $feature{'avatar'}{'override'} = 1;
- # and in project config gitweb.avatar = gravatar;
+ # and in project config gitweb.avatar = provider;
@@ -1523,6 +1528,17 @@ sub format_subject_html { # given page, there's no risk for cache conflicts. our %avatar_cache = ();+# Compute the picon url for a given email, by using the picon search service over at+# http://www.cs.indiana.edu/picons/search.html+sub picon_url {+ my $email = lc shift;+ if (!$avatar_cache{$email}) {+ my ($user, $domain) = split('@', $email);+ $avatar_cache{$email} = "http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/single";
Hmmm... perhaps it would be better to break this very long URL (link)
in lines...
+ }
+ return $avatar_cache{$email};
+}
I wonder if it is worth caching picons, at least in this form. It isn't
as if splitting on '@' is expensive. But perhaps to not break pattern
(that avatar URLs are cached) it is a good thing.
Thoughts for the possible future enhancements: find final URL of an image
via http://www.cs.indiana.edu/cgi-pub/kinzler/piconsearch.cgi/$domain/$user/users+domains+unknown/up/off/1/order
by scrapping (parsing) it for .gif link, and store this URL in cache.
But that most probably isn't worth it. Just feel like mentioning it.
quoted hunk
+
# Compute the gravatar url for a given email, if it's not in the cache already.
# Gravatar stores only the part of the URL before the size, since that's the
# one computationally more expensive. This also allows reuse of the cache for
@@ -1546,6 +1562,8 @@ sub git_get_avatar { my $url = ""; if ($git_avatar eq 'gravatar') { $url = gravatar_url($email, $size);+ } elsif ($git_avatar eq 'picon') {+ $url = picon_url($email); } # Currently only gravatars are supported, but other forms such as # picons can be added by putting an else up here and defining $url
Very nice pattern, nice use of infrastructure.
I think that adding 'width' attribute to img tag of avatar should
perhaps be put here rather than in previous patch, which is about
adding gravatar URL cache. This chunk:
# Currently only gravatars are supported, but other forms such as
# picons can be added by putting an else up here and defining $url
# as needed. If no variant puts something in $url, we assume avatars
# are completely disabled/unavailable.
if ($url) {
- return $pre_white . "<img width=\"$size\" class=\"avatar\" src=\"$url\" />" . $post_white;
+ return $pre_white .
+ "<img width=\"$size\" " .
+ "class=\"avatar\" " .
+ "src=\"$url\" " .
+ "/>" . $post_white;
} else {
return "";
}
should be, I think, in this patch and not in previous one (or at least
in some other patch than the previous one).
BTW. you should have updated the comment here.
Should it be stated that <img width="$size" ...> is here because not
all kinds of avatars (not all avatar providers) support selecting size
of avatar, somewhere in this comment?
Very nice, well thought infrastructure, which makes adding new avatar
providers easy. Good work!
--
Jakub Narebski
Poland
From: Jakub Narebski <hidden> Date: 2016-06-15 22:47:00
On Sat, 27 June 2009, Giuseppe Bilotta wrote:
I think this issue is complicated enough to do it well and configurable
that it is out of scope of current patch series, and it would be better
to be left for next series.
.......................................................................
There are few issues connected with multiple avatar providers:
1. Avatar provider side fallback, i.e. what to do if given person
(usually given email address) is not registered with used avatar
provider.
Gravatar provider by default falls back to [G] gravatar default image,
and you can specify fallback using 'd'/'default' parameter, either to
one of defined providers: special values "identicon", "monsterid" and
"wavatar", or to specified image using URI-encoded URL as value of
'default' parameter. From the point of view of gitweb, you either
pass provider or URL literally (encoding), or you resolve fallback
provider avatar and pass encoded URI.
Picon provider has built-in many level fallback; current gitweb
implementation searches in 'users' database, then in 'domains'
database (there e.g. gmail.com domain addresses receive GMail
avatar), and last in 'unknown' database which would always provide
some default avatar for "person". But in general one can use
cs.indiana.edu database as fallback, and try personal / local site
database first.
2. Gitweb side fallback; for some kinds of avatars one can think about,
for example 'local' avatars (example URL: "avatar/user.gif"), or 'inrepo'
avatar provider (example URL: "?p=$project;a=blob_plain;f=avatar/user.gif"),
one can easily detect in gitweb if avatar for given email (given person)
exists or not (is available or not).
3. Multiple avatars. We can use here something like what Gmane does,
which shows gravatar if it is available (and nothing if it is not),
picon if it is available (and nothing if it is not), and both picon
and gravatar if both are available. See example in:
http://permalink.gmane.org/gmane.comp.version-control.git/122394http://permalink.gmane.org/gmane.comp.version-control.git/118058
This patch tries to address issue 1.) for gravatar provider (which allow
for specifying default avatar via its URL API). I think however that
this issue should be configurable; unconditional using picon provider
with very long URI (making gravatar-with-picon-fallback provider URI
very, very long) is not a good idea.
By configurable I mean that user should be able to specify e.g. gravatar
with wavatar fallback, gravatar with static image fallback, and gravatar
with arbitrary provider (resolved) fallback.
Unless you wanted to provide this patch as an example of having multiple
providers / providing avatar fallback (default value). But then it would
be better to have it marked as RFC explicitly...
Bit of bikeshedding: "alt=\"\" " (empty alt text) would also be
a good solution. This way text browser which do not display images
wouldn't take limited horizontal space for alt text of extraneous
avatar image.
--
Jakub Narebski
Poland
That was my first thought, but since piconsearch offers a link
directly, I decided it was much better to use that rather than html
scraping.
Should it be stated that <img width="$size" ...> is here because not
all kinds of avatars (not all avatar providers) support selecting size
of avatar, somewhere in this comment?
It is generally good form to have the size specified in advance in
HTML anyway, which is why on later iterations I put the width spec in
the first avatar service commit.
--
Giuseppe "Oblomov" Bilotta
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:47:00
2009/6/28 Jakub Narebski [off-list ref]:
Signoff?
Duh.
Bit of bikeshedding: "alt=\"\" " (empty alt text) would also be
a good solution. This way text browser which do not display images
wouldn't take limited horizontal space for alt text of extraneous
avatar image.
Oh, interesting. Let me try that.
--
Giuseppe "Oblomov" Bilotta
From: Jakub Narebski <hidden> Date: 2016-06-15 22:47:00
On Sat, 27 Jun 2009, Giuseppe Bilotta wrote:
One more attempt at adding avatar support to gitweb.
The most significant change since the previous iteration is the addition
of a picon provider, and its use as default from gravatar.
Most other patches were changed to follow Jakub's suggestions. In
particular, the two-line author/committer information was refactored
to reduce code duplication, with the benefit that it could also be used
in 'tag' view.
Giuseppe Bilotta (9):
gitweb: refactor author name insertion
gitweb: uniform author info for commit and commitdiff
gitweb: use git_print_authorship_rows in 'tag' view too
gitweb: right-align date cell in shortlog
gitweb: (gr)avatar support
gitweb: gravatar url cache
gitweb: picon avatar provider
gitweb: use picon for gravatar fallback
gitweb: add alt text to avatar img
I think this patch series shapes very nicely. A bit of refactoring
upfront, so that the following patches are not very large, and don't
need to repeat the same code in many places. Separate issues such
as right-align date cell, or making 'commitdiff' view use authorship
info layout from 'commit' view are put in separate patches in such
way that they can be accepted or rejected individually.
Very good work!
--
Jakub Narebski
Poland
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:47:00
2009/6/29 Jakub Narebski [off-list ref]:
quoted
Giuseppe Bilotta (9):
gitweb: refactor author name insertion
gitweb: uniform author info for commit and commitdiff
gitweb: use git_print_authorship_rows in 'tag' view too
gitweb: right-align date cell in shortlog
gitweb: (gr)avatar support
gitweb: gravatar url cache
gitweb: picon avatar provider
gitweb: use picon for gravatar fallback
gitweb: add alt text to avatar img
I think this patch series shapes very nicely. A bit of refactoring
upfront, so that the following patches are not very large, and don't
need to repeat the same code in many places. Separate issues such
as right-align date cell, or making 'commitdiff' view use authorship
info layout from 'commit' view are put in separate patches in such
way that they can be accepted or rejected individually.
Thanks. I'll send a cleaned up version of everything we agreed on.
--
Giuseppe "Oblomov" Bilotta