Emoticonslib moved to kdereview

[Olivier Goffart]

Le lundi 19 mai 2008, Carlo a écrit :
> On Sun, May 18, 2008 at 7:24 PM, Olivier Goffart <ogoffart at kde.org> wrote:
> > Le dimanche 18 mai 2008, Carlo a écrit :
> >> Ok so I've fixed some of the issue reported by Olivier Goffart and I'm
> >> gonna move it  to kdelibs/kemoticons today, is it ok?
> >>
> >> Carlo
> >
> > More comments:
> >
> > * KEmoticonProvider still expose way to much internal stuff   (it returns
> > pointer to QHash: bad)  making it impossible to eventualy change the
> > matching algoritms later.
>
> *emoticonsMap() is protected so I can modify the qhash inside each plugins

The problem is that if later we want to use some super string matching 
algorithm using another data structure, we can't
(it can be require if we need more performence, or if we want to support some 
other kind of emoticon, or whatever, we never know.)

The rules is that we should never expose internals structure if possible.  and 
that QHash is the internal data.

the engine could just add emoticon to the theme using 
EmoticonTheme::addEmoticon, and then there is no need of exposing the QHash.

> > KEmoticonProvider::Emoticon must not be part of the public API
> > A beter api wold be to have just KEmoticonProvider::addEmoticon
>
> I can make it private and make KEmoticonsTheme friend of KEmoticonsProvider

the C++ private is not enough.  It can be a part of KEmoticonThemePrivate and 
be exposed to the KEmoticonProvider by putting it in kemoticontheme_p.h

> > * the KEmoticonTheme::Token class has a bad API.
> >    Why do we need the html in there while it is used specialy when we
> > don't use the html?
> >    picPath should probably be named emoticonPath
>
> the html code is copied from the Emoticon struct, so if I remove the
> html code from the token I should either have a pointer to this struct
> inside the token or generate the html when parseEmoticons is called
> but this isn't really good

The Token could probably have a private d ptr.

The Kopete::Emoticon code had not a library quality API. that's probably why 
it was not even in the libkopete public API

> > * I still think the KEmoticons class has no interest (method should be
> > moved in KEmoticonTheme as static,  or in a namespace.   It provides no
> > interest (see the need of Kopete::Emoticons in your patch to Kopete)
>
> I don't know how can I change this in a good way

Just put all the function in KEmoticonTheme as static. and all the caches in a 
private K_GLOBAL_STATIC object.


> > * My concens about html are still there.  parseEmoticons is ONLY for
> > HTML, and the SkipHTML MUST to be set (by the parseEmoticons function)
>
> So I just have to force SkipHtml inside parseEmoticons?

Yes, definitively.

> > * I don't like the exclude argument to the parseEmoticons function.  If
> > you don't want some emoticons, just don't add them to the theme!!
>
> but since users are dumb they will complain about things like (c) that
> renders as a coffe cup in kmail but they want it with kopete since
> it's used by msn and maybe other protocols
> and anyway since it's an optional argument if you don't like it just
> don't use it :P it shouldn't be a performance problem to lookup an
> empty qlist

But what if i create a (c) emoticon which looks like ©, and that i want it in 
kmail as well.

If users want the MSN emoticon theme for kmail, they will get thoses stupid 
cup smeley, but this is probably what they want.   otherwhise, they would 
choose a better theme.


> > * in KEmoticonTheme::addEmoticon, the param text could maybe be a
> > QStringList
>
> I can add an overloaded addEmoticon that takes a QStringList as parameter

yes, if you remove the fact that you can split them with spaces.
And that would allow emoticons with spaces.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20080519/70df2124/attachment.sig>