Re: [PATCH] gitweb: gravatar support
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Jun 19, 2009, 22:31 UTC
- Message-ID
- <200906200031.55576.jnareb@gmail.com>
- In-Reply-To
- <cb7bb73a0906190757o4d083735t1a3f37396af76c2f@mail.gmail.com>
On Fri, 19 June 2009, Giuseppe Bilotta wrote:
> thanks for the comments, I'll send a new patch taking them into > consideration soon.
Errr... I had a few additional comments to this reply, but I was a bit late I think with replying.
Show 28 quoted lines
>
> 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.Well, I was not asking you to change it; I was asking about justification behind making it non-overridable.
Show 11 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...
> >> Did you run the t9500, adding test enabling gravatars? > > Ehrm, no 8-P
Well, it is always nice to have test for new feature. Unfortunately due to the smart way it is done currently unmodified (not extended) t9500 wouldn't catch above issue.
[...]
Show 6 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?
Show 10 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.
More comments to follow in v2 and replies.
-- Jakub Narebski Poland