Re: [PATCH] gitweb: gravatar support

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

On Sat, Jun 20, 2009 at 12:31 AM, Jakub Narebski<jnareb@gmail.com> wrote:
>>> 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.
>
> Well, I was not asking you to change it; I was asking about
> justification behind making it non-overridable.

I decided to go for the override because it was little effort and it
whether or not the override can take place can still be decided at a
central place.

>>> 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.
>
> Well, as you use 'undef' (do not pass parameter) for default value
> and you _do not_ use negative (or zero) value, then
>
> +             $size = 32 if !defined($size);
>
> would be enough (in Perl 6 / Perl 5.10 it would be "$size //= 32" ;-)
>
> The question is: does gravatar.com accepts any size?  What does it do
> if it gets negative size passed?  I have not used gravatar as
> developer...

If you pass a negative size, the image comes up blank. Using undef for
the default value makes sense. The question rather becomes whether it
makes sense or not to provide a safety fallback for when the size
comes up negative.

>>> 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
>
> What is the problem?

I really don't remember, I created the patch a long time ago. As you
can probably seen from v2, I just decided to go with the same code
everywhere (which asks for a refactoring).

>>> 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.
>
> What I though here was to use %gravatar_size hash, with keys such as
> 'default' and 'double' (for "double line height").  If you change or
> add CSS, changing configuration, you can change also this in config.

And since the sices could be used for other things than gravatar, we
could call it just %avatar_size_hash or whatever, and future support
for picons or whatever else can use it too.


-- 
Giuseppe "Oblomov" Bilotta