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