> On Oct. 9, 2013, 5:05 p.m., Kevin Ottens wrote:
> > I'm not sure what this patch is trying to achieve. It doesn't look much
> > more clearer API to me than what we had before. :-)
> >
> > Could you share a bit more of the rationale? I guess I'm missing something.
Ok, let's see. We have a method in KEmoticonsProvider that creates a map and
another method that creates an index:
/**
* Returns a QHash that contains the emoticon path as keys and the text as
values
*/
QHash<QString, QStringList> emoticonsMap() const;
/**
* Returns a QHash that contains emoticons indexed by the first char
*/
QHash<QChar, QList<Emoticon> > emoticonsIndex() const;
We have a method that clears the map:
/**
* Clears the emoticons map
*/
void clearEmoticonsMap();
And then we have the original methods that manage the map and the index. The
problem I see with them is that addEmoticonsMap(), for example, doesn't
actually do what it says. If you don't read the documentation,
addEmoticonsMap() seems to add (somewhere) a new (or existing) emoticon map.
Instead, it inserts a NEW ITEM in the emoticon map:
/**
* Inserts a new item in the emoticon map
*
*/
void addEmoticonsMap(QString key, QStringList value);
That's why I've changed it to addMapItem(). I could've used
addEmoticonsMapItem(), but I thought it was too long and since there's only one
map and there's only one index in KEmoticonsProvider, addMapItem() is clear
enough (and clearer than clearEmoticonsMap()). Besides, it now points to the
documentation of emoticonsMap(), which explains where does that map come from:
/**
* Inserts a new item in the emoticon map
* @since 5.0
* @see emoticonsMap()
*/
void addMapItem(QString key, QStringList value);
The same applies to the rest:
Deprecated:
/**
* Removes an item from the emoticon map
*
*/
void removeEmoticonsMap(QString key);
New:
/**
* Removes an item from the emoticon map
* @since 5.0
* @see emoticonsMap()
*/
void removeMapItem(QString key);
--------------------------
Deprecated:
/**
* Adds an emoticon to the index
* @param path path to the emoticon
* @param emoList list of text associated with this emoticon
*
*/
void addEmoticonIndex(const QString &path, const QStringList &emoList);
New:
/**
* Adds an emoticon to the index
* @param path path to the emoticon
* @param emoList list of text associated with this emoticon
* @since 5.0
* @see emoticonsIndex()
*/
void addIndexItem(const QString &path, const QStringList &emoList);
---------------------------
Deprecated:
/**
* Removes an emoticon from the index
* @param path path to the emoticon
* @param emoList list of text associated with this emoticon
*/
void removeEmoticonIndex(const QString &path, const QStringList &emoList);
New:
/**
* Removes an emoticon from the index
* @param path path to the emoticon
* @param emoList list of text associated with this emoticon
* @since 5.0
* @see emoticonsIndex()
*/
void removeIndexItem(const QString &path, const QStringList &emoList);
-----------------------------
When I began to work with KEmoticons I found much code that wasn't very good,
so I want to try to make the API better, because is now or never :-) . Anyway,
I understand it's a delicate issue.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112985/#review41450
-----------------------------------------------------------
On Oct. 6, 2013, 9:02 p.m., David Gil Oliva wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112985/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2013, 9:02 p.m.)
>
>
> Review request for KDE Frameworks.
>
>
> Repository: kdelibs
>
>
> Description
> -------
>
> Adjust API in KEmoticons framework: map and index methods
>
> -Make map and index KEmoticons API slightly clearer and deprecate
> the methods that are confusing.
> -Use the new methods in the plugins.
>
>
> Diffs
> -----
>
> staging/kemoticons/src/core/kemoticonsprovider.h
> 85fc7efb8923d76476f0a16f70f8ebb15e451081
> staging/kemoticons/src/core/kemoticonsprovider.cpp
> d04c76e87b118f5d45717b3b2ccd9dea47dc2526
> staging/kemoticons/src/providers/adium/adium_emoticons.cpp
> a3aaa0fdc0b3dcc862f98865d2e6419e716f4f17
> staging/kemoticons/src/providers/kde/kde_emoticons.cpp
> 5b5114a14dd94a6ebcba8a6f7dd163f73048189a
> staging/kemoticons/src/providers/pidgin/pidgin_emoticons.cpp
> e9f89eecce3d6e6407113a84cb5200ff66564c19
> staging/kemoticons/src/providers/xmpp/xmpp_emoticons.cpp
> 0dc92ed092d87a559fe7fa1a40e804843ab73d59
>
> Diff: http://git.reviewboard.kde.org/r/112985/diff/
>
>
> Testing
> -------
>
> It builds. It installs. Tests pass.
>
>
> Thanks,
>
> David Gil Oliva
>
>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
