Emoticonslib moved to kdereview
ogoffart at kde.org
Tue May 13 23:08:03 BST 2008
Le jeudi 1 mai 2008, Carlo a écrit :
> 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
- 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?
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 189 bytes
Desc: This is a digitally signed message part.
More information about the kde-core-devel