From kde-core-devel  Tue May 13 22:08:03 2008
From: Olivier Goffart <ogoffart () kde ! org>
Date: Tue, 13 May 2008 22:08:03 +0000
To: kde-core-devel
Subject: Re: Emoticonslib moved to kdereview
Message-Id: <200805140008.09713.ogoffart () kde ! org>
X-MARC-Message: https://marc.info/?l=kde-core-devel&m=121071679915201
MIME-Version: 1
Content-Type: multipart/mixed; boundary="--nextPart1302599.9oF6NRVM0c"

--nextPart1302599.9oF6NRVM0c
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

Le jeudi 1 mai 2008, Carlo a =E9crit=A0:
> Hi,
> I've moved emoticonslib from playground to kdereview, it's a simple
> library that can be used to parse emoticons in a text, choose and
> modify themes etc..
> Since there are many kind of emoticons theme it uses plugin to manage
> each type of file
>
> I've made this library because every application have written their
> own implementation to handle emoticons and there isn't a global config
> file like for icons or style, if you have any feature requests please
> ask before the feature freeze

Hi.
Pushing emoticons libs into kdelibs has always been one of my wish.

Here is my review:

=2D The code contains lots of code from Kopete, please keep the copyright=20
header, (and mention where the code come from so it can be looked in the sv=
n=20
history)

=2D Lots of the internals structure should be hidden. For exemple, all thos=
es=20
protected: should be private:  And some data structure (such as QMap) shoul=
d=20
not be exposed in public API.

=2D Consider using QHash instead of QMap

=2D The only purpose of the KEmoticons object is the signal that could be m=
oved=20
to KGlobalSettings or something similair.  All the other method are static,=
=20
and could be placed as static members in the KEmoticonTheme.

=2D the Token struct should just contains the token type, the text, and the=
 path=20
to the emoticon.  'emoticonPath' is maybe a better name.
The HTML should not be part of the token itself. because if html is used, t=
hen=20
the parse function can be used.

=2D Performence!!!  The original Kopete code did maintain an index of emoti=
cons=20
by the first character of it.   Looking thought each emoticon on each=20
character of the message doesn't give acceptable performences. (noticable=20
when you have a big emoticon set, and you have a big text to parse (eg.=20
browsing the history)

=2D SkipHTML is meant only for tokenize function. It means that the input s=
tring=20
is in Html.  The parseEmoticon function should only accept html (and it=20
should be documented), and should ensure that skipHTML flag is in use.

=2D What is the use case of the exclude parameter?

=2D The Kopete code had some unit tests for emoticon parsing.  It is very=20
important that this code is tested. Maybe you could use QTestLib for that.
Also check that uses case that are in the kopete test still pass. I fear th=
at=20
some stuff may be broken (choose the largest emoticon, ...)

=2D In which kde library do you plan to move those class?
It make few sens to have them as a stand alone library? (Or does it?)
It can't be moved into kdeui because it uses kio.
Maybe in kio or kdeutils?  =20

=2D-=20
Olivier

--nextPart1302599.9oF6NRVM0c
Content-Type: application/pgp-signature; name=signature.asc 
Content-Description: This is a digitally signed message part.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iD8DBQBIKhFFz58lY8jWrL0RAvo5AKCGflR0DucyjvvA5MfgW61SicbdkgCeKDcG
8ZDydNSbq43YE5isiBMj2tA=
=D1XF
-----END PGP SIGNATURE-----

--nextPart1302599.9oF6NRVM0c--