<div dir="ltr">Hi,<br><div><div class="gmail_extra"><br><br><div class="gmail_quote">2013/8/27 Kevin Ottens <span dir="ltr"><<a href="mailto:ervin@kde.org" target="_blank">ervin@kde.org</a>></span><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Hello,<br>
<div><br>
On Monday 26 August 2013 23:36:13 David Gil Oliva wrote:<br>
> I took the KEmoticons splitting task. I'm studying the code and I have<br>
> found many issues:<br>
><br>
> --KEmoticonsProvider should be abstract, since its virtual methods either<br>
> don't do anything or their code is not safe.<br>
<br>
</div>Could you be more specific? I don't think I would turn most of them as pure<br>
virtual. At a glance obvious candidate to be turned pure virtuals are save()<br>
and createNew()... could be added loadTheme(), addEmoticon() and<br>
removeEmoticon() with extra care.<br></blockquote><div><br>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:<br></div><div><br>bool KEmoticonsProvider::loadTheme(const QString &path)<br>
{<br> QFileInfo info(path);<br> d->m_fileName = info.fileName();<br>
d->m_themeName = info.dir().dirName();<br> d->m_themePath = info.absolutePath();<br> return true;<br>}<br><br></div><div>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.<br>
</div><div><br>bool KEmoticonsProvider::addEmoticon(const QString &emo, const QString &text, AddEmoticonOption option)<br>{<br> if (option == Copy) {<br> KIO::CopyJob* job = KIO::copy(QUrl::fromLocalFile(emo), QUrl::fromLocalFile(d->m_themePath));<br>
job->exec();<br> }<br><br> Q_UNUSED(text);<br> return false;<br>} <br><br></div><div>addEmoticon also won't return any other value than false. <br><br>bool KEmoticonsProvider::removeEmoticon(const QString &emo)<br>
{<br> Q_UNUSED(emo);<br> return false;<br>}<br><br></div><div>What can I say about this method? :-D<br><br> 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.<br>
</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
> --Many KEmoticons tests don't pass (I haven't actually changed anything<br>
> yet).<br>
<br>
</div>Well the autotests seem to pass. There's a few XFAIL but that seems intended.<br>
</blockquote><div><br></div><div>Ok, I'll look closer.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
> And kdelibs-frameworks/staging/kemoticons/tests/main.cpp doesn't<br>
> parse correctly the emoticons. It says 'kemoticonstest(2650)/default<br>
> KEmoticonsPrivate::loadProvider: Invalid plugin factory for<br>
> "emoticonstheme_kde"'. I suspect that the uses of KPluginLoader have to be<br>
> substituted by QPluginLoader in order to work correctly. Could I be right?<br>
<br>
</div>Not substituted, but some porting to the new KPlugin* API introduced by sebas<br>
is indeed needed. There should be porting notes in KDE5PORTING.<br>
</blockquote><div><br></div><div>Ok.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
> --The API method names could be improved to be clearer. I don't know<br>
> whether to deprecate the old ones or simply substitute them. What's better?<br>
<br>
</div>I wouldn't mess around too much with the API for now. What about fixing the<br>
other issues and then revisiting that?<br>
<div><br></div></blockquote><div><br></div><div>I agree.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
> --KEmoticonsProvider includes <kio/copyjob.h> and <kio/jobuidelegate.h>. Is<br>
> this OK?<br>
<br>
</div>It's OK for a tier 3. Now it seems to be a really minor use, and it's only for<br>
local use, so if we can somehow spare it that'd be a nice win for this<br>
framework.<br>
<div><br></div></blockquote><div>I will.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
> First I'll try to fix the<br>
> kdelibs-frameworks/staging/kemoticons/tests/main.cpp test to be able to<br>
> know where the problems are when parsing. Then I'll modify the framework to<br>
> be as well written as I can and to have all tests passed. Finally I'll<br>
> split it into tier3 folder. I'll send little patches to RB.<br>
<br>
</div>Sounds good. Except for the API changes, keep them for after the rest is done,<br>
they'll need to be evaluated on a case by case basis.<br>
<br></blockquote><div><br></div><div>Ok.<br><br></div><div>Regards,<br></div><div><br></div><div>David Gil<br></div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Regards.<br>
<span><font color="#888888">--<br>
Kévin Ottens, <a href="http://ervin.ipsquad.net" target="_blank">http://ervin.ipsquad.net</a><br>
<br>
KDAB - proud supporter of KDE, <a href="http://www.kdab.com" target="_blank">http://www.kdab.com</a><br>
</font></span><br>_______________________________________________<br>
Kde-frameworks-devel mailing list<br>
<a href="mailto:Kde-frameworks-devel@kde.org" target="_blank">Kde-frameworks-devel@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/kde-frameworks-devel" target="_blank">https://mail.kde.org/mailman/listinfo/kde-frameworks-devel</a><br>
<br></blockquote></div><br></div></div></div>