[sip-comm-dev] Favicon patch


#1

Hello,
Favicon support for the RSS reader is finally here :smiley:
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 :slight_smile:

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

FaviconPatch.tar.gz (32.2 KB)


#2

Hi Mihai,

Your patch works like a charm.
It is very pleasant to have the favicon as the contact image.

A very good work,
Vincent

Mihai Balan wrote:

路路路

Hello,
Favicon support for the RSS reader is finally here :smiley:
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 :slight_smile:

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-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#3

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

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 :smiley:
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 :slight_smile:

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-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net