Review Request 112985: Adjust API in KEmoticons framework: map and index methods

[David Gil Oliva]

> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20131009/261d8cd2/attachment.html>