Hello,
thanks for the comments, I'll send a new patch taking them into
consideration soon.
A few additional replies:
>> + # Gravatar support. When this feature is enabled, views such as
>> + # shortlog or commit will display the gravatar associated with
>> + # the email of the committer(s) and/or author(s). Please note that
>> + # the feature depends on Digest::MD5.
>> +
>> + # To enable system wide have in $GITWEB_CONFIG
>> + # $feature{'gravatar'}{'default'} = [1];
>> + # Project specific override is not supported.
>> + 'gravatar' => {
>> + 'override' => 0,
>> + 'default' => [0]},
>
> Yet another global feature without project specific override. Hmmm...
> I wonder if project specific and global (non-overridable) features
> should be separated. But it is question for a separate commit.
>
> Question: why it is not overridable (why project specific override
> is not supported for this feature)? Some projects may use Gravatars,
> some might not, although I guess that usually it is deployment specific
> feature.
I see it as a deployment feature, and considering that it adds an
(admittedly small) extra load on the server, I thought it was sensible
to make it non-overridable. OTOH, since the load is small, it might be
possible to make it per-project without big issues.
> You would probably want to protect against $size being undefined:
>
> + $size = 32 if (!defined($size) || $size <= 0);
>
> Because currently when you are not passing size parameter to use
> default size you would get the following warning:
>
> Use of uninitialized value in numeric le (<=) ...
Oh right.
> Did you run the t9500, adding test enabling gravatars?
Ehrm, no 8-P
> Hmmm... perhaps we should add test with all possible features turned
> on (unless they need extra configuration... oh, so it isn't as easy
> as I initially thought to add this...).
If gravatars can be added easily, I can try doing it.
>> + print "<tr><td>author</td><td>" . esc_html($co{'author'}) . "</td>".
>> + "<td rowspan=\"2\">" .git_get_gravatar(lc $co{'author_email'}) . "</td></tr>\n" .
>
>> + print "<tr><td>committer</td><td>" . esc_html($co{'committer'}) . "</td>".
>> + "<td rowspan=\"2\">" .git_get_gravatar(lc $co{'committer_email'}) . "</td></tr>\n";
>
> Hmmm... why use 'lc' here and not in other places?
That's a good question. I'm actually not sure if the gravatar service
is case sensitive or not ... the reference implementation uses lc. I
should probably move the lowercasing to git_get_gravar itself.
> Also gravatars are
> not shown in 'history' view, but I guess that could wait for proper
> refactoring of all log-like views/actions to use common infrastructure.
That's part of the reason, the other being that I couldn't find a
satisfactory way to do it 8-P
> Those all look nice with the *default* font sizes. But as the size of
> gravatar is used when constructing gravatar URL, to pass to gravatar.com
> I don't see how this problem can be resolved... Beside making it
> configurable, I guess...
That's something I hadn't thought about, honestly. The problem of
course is that the font sizes get customized via CSS, but the gravatar
size would get customized at the cgi level ... so unless we parse the
CSS from the cgi it cannot be done automatically.
--
Giuseppe "Oblomov" Bilotta
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html