KEmoticons splitting task

[Kevin Ottens]

Hello,

On Wednesday 28 August 2013 22:32:50 David Gil Oliva wrote:
> 2013/8/27 Kevin Ottens <ervin at kde.org>
> > 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.

OK, indeed you make it sound like they're proper candidates to turn into pure 
virtual methods.

> The problem is that KEmoticonsTheme uses this KEmoticonsProvider class and
> I'll have to see what to do to have everything working.

Yes but AFAICT it doesn't try to instantiate it, so you should be fairly OK 
there.

Regards.
-- 
Kévin Ottens, http://ervin.ipsquad.net

KDAB - proud supporter of KDE, http://www.kdab.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130829/924e12d4/attachment.sig>