[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Emoticonslib moved to kdereview
From:       Olivier Goffart <ogoffart () kde ! org>
Date:       2008-05-13 22:08:03
Message-ID: 200805140008.09713.ogoffart () kde ! org
[Download RAW message or body]


Le jeudi 1 mai 2008, Carlo a écrit :
> 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:

- The code contains lots of code from Kopete, please keep the copyright 
header, (and mention where the code come from so it can be looked in the svn 
history)

- Lots of the internals structure should be hidden. For exemple, all thoses 
protected: should be private:  And some data structure (such as QMap) should 
not be exposed in public API.

- Consider using QHash instead of QMap

- The only purpose of the KEmoticons object is the signal that could be moved 
to KGlobalSettings or something similair.  All the other method are static, 
and could be placed as static members in the KEmoticonTheme.

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

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

- SkipHTML is meant only for tokenize function. It means that the input string 
is in Html.  The parseEmoticon function should only accept html (and it 
should be documented), and should ensure that skipHTML flag is in use.

- What is the use case of the exclude parameter?

- The Kopete code had some unit tests for emoticon parsing.  It is very 
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 that 
some stuff may be broken (choose the largest emoticon, ...)

- 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?   

-- 
Olivier

["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]