Hello,
On Wednesday 28 August 2013 22:32:50 David Gil Oliva wrote:
> 2013/8/27 Kevin Ottens <er...@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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
