Hey Mihai,
I've just completed review-ing your patch! Very good job,
really, and it
works without a glitch! Nice! Your code is very mature, and
you respect
coding conventions. I like it for example when you do stuff
like this
URL location = new URL( *feedLocation.getProtocol()* +
"://"
+ feedLocation.getHost() +
"/favicon.ico");
While many would have settled for "http://" instead of trying to
look
for a method that returns the protocol and this would have
lead to a
problem for https flows.
I've committed your code and ack-ed the effort. I've also
added
AC.lib-ICO to the list of libs we're using.
I also have some (minor) comments and questions
log4j:
* log4j.properties - I've currently removed them as they
were trying to
store a log file in the current directory and this would
cause problems
in situations where the user doesn't have the right to do
this (on
debian for example). It would possibly be a better idea to
specify these
properties in our RssActivator and make AC.lib-ICO's store
logs in
sip-communicator.home... but let's leave this for another
day.
* log4j.jar - I've removed references to log4j.jar, created
a separate
bundle for it and also moved it out of jain-sip, so that we
don't have
it in double
AC.lib-ICO:
* This one is probably worth sending on their mailing list.
It appears
there are problems with some favicon formats, like for
example the one
from blogsport. http:
//sc-contact-info.blogspot.com/favicon.ico
This is not a big problem though since you are gracefully
handling all
such errors.
build.xml:
* Removed the comment containing the user.agent header since
we do this
in the rss activator. Was there any other reason you had
placed it in there?
* Removed the line that was importing log4j (since it was
already
commented).
ContactRssImpl:
* (this one is really minor ... but it doesn't hurt
mentioning it) right
now icon selection is kind of arbitrary as it would depend
on
the order. we would pick the biggest image if it is located
after the
one that's most color rich, and we'll pick the most colorful
one if it's
located after the one with the maximum size. Shouldn't we
have a
specific policy? For example - we prefer colors to size or
vice versa?
* I added exceptions as a second parameter to the
logger.error() prints
in getImage() so that we also have the stack trace for debug
purposes.
* Some day in the future we might also want to pick the
favicon of the
default website page (the one defined in the link tags) in
case no
favicon.ico file exists (as for example is the case with
sip-communicator.org and sip-communicator.dev.java.net).
This is clearly
not an emergency though, since there are many other more
important
things, and besides I couldn't find anyone other than us
that was
specifying a favicon through the link tags without also
having one in
the root (and I did try hard ;) ).
felix.client.run.properties:
* We don't really need to import the package from AC.lib-ICO
do we?
We already have it in the bundle where we need it.
rss.provider.manifest.mf:
* You don't need to import com.ctreber.aclib.image.ico
because it is
inside your bundle. Besides, importing it here would mean
that someone
else has to be exporting it, which probably explains why you
felt you
needed to add it to felix.client.run.properties.
That's all. Once again, all my comments are minor, you have
really done
a very good job!
Cheers
Emil
Mihai Balan wrote:
> Hello,
> Favicon support for the RSS reader is finally here 
> Attached to the mail you'll find an archive containing
the patch files
> as well as the jar containing the ICO deconding
library. This library
> must be copied to
lib/installer-exclude/aclibico-2.1.jar in order for
> the patch to work as advertised.
> In two words, it works like this: if the site has an
favico.ico file
> in / (e.g. no HTTP/404 error is returned upon
requesting this file),
> the icon is loaded and the best image in the icon is
used (remember
> that ICO files can contain more than one image). If no
such file can
> be found or is invalid, the standard RSS icon is used
to add a little
> more color to the dialog displaying the feed 
>
> Hope I haven't missed anything and you'll enjoy using
it as much as I
> enjoyed writing it!
>
> A wonderful evening to you all,
> Mihai
>
>
>
------------------------------------------------------------
------------
>
>
------------------------------------------------------------
---------
> To unsubscribe, e-mail: dev-unsubscribe sip-communicator.dev.java.net
> For additional commands, e-mail: dev-helpsip-communicator.dev.java.net
------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesip-communicator.dev.java.net
For additional commands, e-mail: dev-helpsip-communicator.dev.java.net
|
