Emoticonslib moved to kdereview

[Carlo]

On Wed, May 14, 2008 at 12:08 AM, Olivier Goffart <ogoffart at kde.org> wrote:
> 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)
Ok I'll add that
>  - 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.
I've changed protected to private in kemoticonstheme.h, I forgot to do
that because kemoticonstheme and kemoticonsprovider was one class
before and I've used a shared d pointer
>  - 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.
it's not just a signal, there are other things like a cache for the
emoticons theme already loaded
>  - 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.
I've copied that straight from kopete :P
>  - 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)
I tought that this wasn't really useful, I'll readd it
>  - 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.
I don't understand this problem
>  - What is the use case of the exclude parameter?
for example in linklocator in kdepimlibs that's used by kmail, they
want to exclude "(c)" "(C)" and some other strings
>  - 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, ...)
ok I'll copy them
>  - 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?
inside kdelibs as standalone like knewstuff/phonon etc.. I can't think
of any other solutions
>  --
>  Olivier
>