----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112660/#review39866 -----------------------------------------------------------
General approach looks good. staging/kservice/src/services/kplugininfo.cpp <http://git.reviewboard.kde.org/r/112660/#comment29402> Why create such constant strings in *every* instance of KPluginInfo? This seems quite costly to me (both cpu and memory) (not to mention the readability, I also got a bit confused by hidden vs _hidden). Prefer QStringLiteral("..") directly in code, or if this would lead to duplication of the string constants, either static const char[] s_authorKey = "X-KDE-PluginInfo-Author" or file-static functions that return a QString (one per string). staging/kservice/src/services/kplugininfo.cpp <http://git.reviewboard.kde.org/r/112660/#comment29403> I think toBool() already takes care of returning false for invalid variants. staging/kservice/src/services/kplugininfo.cpp <http://git.reviewboard.kde.org/r/112660/#comment29404> space after if, while you're at it - David Faure On Sept. 10, 2013, 11:32 p.m., Sebastian Kügler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/112660/ > ----------------------------------------------------------- > > (Updated Sept. 10, 2013, 11:32 p.m.) > > > Review request for KDE Frameworks and David Faure. > > > Description > ------- > > This patch prepares KPluginInfo for usage with KPluginTrader (to be submitted > in a separate patch). It basically makes KPluginInfo's API a little bit more > like KService by adding a property(QString) accessor to the plugin info. This > allows us on the one hand > > Part of this patch, and much of its churn, is the internal change from > independent QStrings and QStringLists to a QVariantMap. This is how the > metadata comes in from KPlugin*, and it allows us to make properties > accessible by name. > > There's also a fair bit of moving from QLatin1String to QStringLiteral in > there, most of these lines needed changes anyway. Additionally, the keys of > properties are now shared in the d-pointer. > > This change is source compatible to the old version. > > > Diffs > ----- > > staging/kservice/src/services/kplugininfo.cpp 21e0882 > > Diff: http://git.reviewboard.kde.org/r/112660/diff/ > > > Testing > ------- > > All tests still pass, no regressions encountered otherwise. > > > Thanks, > > Sebastian Kügler > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
