Emoticonslib moved to kdereview
brandon.ml at gmail.com
Wed May 14 00:00:55 BST 2008
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
> 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
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
More information about the kde-core-devel