<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/112985/">http://git.reviewboard.kde.org/r/112985/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 9th, 2013, 5:05 p.m. UTC, <b>Kevin Ottens</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- David</p>
<br />
<p>On October 6th, 2013, 9:02 p.m. UTC, David Gil Oliva wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By David Gil Oliva.</div>
<p style="color: grey;"><i>Updated Oct. 6, 2013, 9:02 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It builds. It installs. Tests pass.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>staging/kemoticons/src/core/kemoticonsprovider.h <span style="color: grey">(85fc7efb8923d76476f0a16f70f8ebb15e451081)</span></li>
<li>staging/kemoticons/src/core/kemoticonsprovider.cpp <span style="color: grey">(d04c76e87b118f5d45717b3b2ccd9dea47dc2526)</span></li>
<li>staging/kemoticons/src/providers/adium/adium_emoticons.cpp <span style="color: grey">(a3aaa0fdc0b3dcc862f98865d2e6419e716f4f17)</span></li>
<li>staging/kemoticons/src/providers/kde/kde_emoticons.cpp <span style="color: grey">(5b5114a14dd94a6ebcba8a6f7dd163f73048189a)</span></li>
<li>staging/kemoticons/src/providers/pidgin/pidgin_emoticons.cpp <span style="color: grey">(e9f89eecce3d6e6407113a84cb5200ff66564c19)</span></li>
<li>staging/kemoticons/src/providers/xmpp/xmpp_emoticons.cpp <span style="color: grey">(0dc92ed092d87a559fe7fa1a40e804843ab73d59)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112985/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>