Re: [PATCH] gitweb: gravatar support

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Jun 20, 2009, 06:30 UTC
Message-ID: <cb7bb73a0906192330j7c7a8ed3o39f0eb65d29e334c@mail.gmail.com>
In-Reply-To: <200906200031.55576.jnareb@gmail.com>

On Sat, Jun 20, 2009 at 12:31 AM, Jakub Narebski<jnareb@gmail.com> wrote:

Show 12 quoted lines

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

Show 21 quoted lines

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

Show 8 quoted lines

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

Show 13 quoted lines

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