KEmoticons splitting task

[David Gil Oliva]

Hi,


2013/8/27 Kevin Ottens <ervin at kde.org>

> Hello,
>
> On Monday 26 August 2013 23:36:13 David Gil Oliva wrote:
> > I took the KEmoticons splitting task. I'm studying the code and I have
> > found many issues:
> >
> > --KEmoticonsProvider should be abstract, since its virtual methods either
> > don't do anything or their code is not safe.
>
> Could you be more specific? I don't think I would turn most of them as pure
> virtual. At a glance obvious candidate to be turned pure virtuals are
> save()
> and createNew()... could be added loadTheme(), addEmoticon() and
> removeEmoticon() with extra care.
>

No, I didn't mean to convert to pure virtual but the ones that already are
virtual. As you say, save() and createNew(), but also these:

bool KEmoticonsProvider::loadTheme(const QString &path)
{
    QFileInfo info(path);
    d->m_fileName = info.fileName();
    d->m_themeName = info.dir().dirName();
    d->m_themePath = info.absolutePath();
    return true;
}

loadTheme won't ever return false, it doesn't verify whether the path is
valid and directly assigns values to private variables. This makes the test
I'm modifying right now unusable, because it accepts an empty theme name.

bool KEmoticonsProvider::addEmoticon(const QString &emo, const QString
&text, AddEmoticonOption option)
{
    if (option == Copy) {
        KIO::CopyJob* job = KIO::copy(QUrl::fromLocalFile(emo),
QUrl::fromLocalFile(d->m_themePath));
        job->exec();
    }

    Q_UNUSED(text);
    return false;
}

addEmoticon also won't return any other value than false.

bool KEmoticonsProvider::removeEmoticon(const QString &emo)
{
    Q_UNUSED(emo);
    return false;
}

What can I say about this method? :-D

 All three just don't seem right and they need to be reimplemented. In
fact, PidginEmoticons, XmppEmoticons and KdeEmoticons just reimplement all
five methods. The problem is that KEmoticonsTheme uses this
KEmoticonsProvider class and I'll have to see what to do to have everything
working.



> > --Many KEmoticons tests don't pass (I haven't actually changed anything
> > yet).
>
> Well the autotests seem to pass. There's a few XFAIL but that seems
> intended.
>

Ok, I'll look closer.


> > And kdelibs-frameworks/staging/kemoticons/tests/main.cpp doesn't
> > parse correctly the emoticons. It says 'kemoticonstest(2650)/default
> > KEmoticonsPrivate::loadProvider: Invalid plugin factory for
> > "emoticonstheme_kde"'. I suspect that the uses of KPluginLoader have to
> be
> > substituted by QPluginLoader in order to work correctly. Could I be
> right?
>
> Not substituted, but some porting to the new KPlugin* API introduced by
> sebas
> is indeed needed. There should be porting notes in KDE5PORTING.
>

Ok.


> > --The API method names could be improved to be clearer. I don't know
> > whether to deprecate the old ones or simply substitute them. What's
> better?
>
> I wouldn't mess around too much with the API for now. What about fixing the
> other issues and then revisiting that?
>
>
I agree.


> > --KEmoticonsProvider includes <kio/copyjob.h> and <kio/jobuidelegate.h>.
> Is
> > this OK?
>
> It's OK for a tier 3. Now it seems to be a really minor use, and it's only
> for
> local use, so if we can somehow spare it that'd be a nice win for this
> framework.
>
> I will.


> > First I'll try to fix the
> > kdelibs-frameworks/staging/kemoticons/tests/main.cpp test to be able to
> > know where the problems are when parsing. Then I'll modify the framework
> to
> > be as well written as I can and to have all tests passed. Finally I'll
> > split it into tier3 folder. I'll send little patches to RB.
>
> Sounds good. Except for the API changes, keep them for after the rest is
> done,
> they'll need to be evaluated on a case by case basis.
>
>
Ok.

Regards,

David Gil



> Regards.
> --
> Kévin Ottens, http://ervin.ipsquad.net
>
> KDAB - proud supporter of KDE, http://www.kdab.com
>
> _______________________________________________
> Kde-frameworks-devel mailing list
> Kde-frameworks-devel at kde.org
> https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130828/38d68ee9/attachment-0001.html>