From: Giuseppe Bilotta Date: Sat, 20 Jun 2009 06:30:58 GMT Subject: Re: [PATCH] gitweb: gravatar support Message-ID: In-Reply-To: <200906200031.55576.jnareb@gmail.com> On Sat, Jun 20, 2009 at 12:31 AM, Jakub Narebski 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