Re: Review Request 112660: KPluginInfo rework for KPluginTrader
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112660/#review40233 --- This review has been submitted with commit 808e6625c3964517cb901114eaec7a7dcc65561b by Sebastian Kügler to branch frameworks. - Commit Hook On Sept. 12, 2013, 7:07 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. 12, 2013, 7:07 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.h c2e5bab 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
Re: Review Request 112660: KPluginInfo rework for KPluginTrader
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112660/ --- (Updated Sept. 17, 2013, 3:49 p.m.) Status -- This change has been marked as submitted. 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.h c2e5bab 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
Re: Review Request 112660: KPluginInfo rework for KPluginTrader
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112660/#review39935 --- Ship it! ... after fixing the last two small issues. staging/kservice/src/services/kplugininfo.h http://git.reviewboard.kde.org/r/112660/#comment29479 missing docu for libraryPath staging/kservice/src/services/kplugininfo.h http://git.reviewboard.kde.org/r/112660/#comment29480 can be removed now, no? - David Faure On Sept. 12, 2013, 7:07 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. 12, 2013, 7:07 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.h c2e5bab 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
Re: Review Request 112660: KPluginInfo rework for KPluginTrader
--- 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
Re: Review Request 112660: KPluginInfo rework for KPluginTrader
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112660/ --- (Updated Sept. 12, 2013, 7:07 p.m.) Review request for KDE Frameworks and David Faure. Changes --- * Changed keys to static const char[], used s_camelCaseKey for their names * Added libraryPath = QString() in the args-based ctor (from other review) * Coding style++ 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 (updated) - staging/kservice/src/services/kplugininfo.h c2e5bab 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
Re: Review Request 112660: KPluginInfo rework for KPluginTrader
On Sept. 11, 2013, 6:22 a.m., David Gil Oliva wrote: staging/kservice/src/services/kplugininfo.cpp, line 61 http://git.reviewboard.kde.org/r/112660/diff/1/?file=188728#file188728line61 I think that two variables (hidden and _hidden) so similar are confusing. Sebastian Kügler wrote: Hm, I think it's pretty clear, especially since _hidden is defined above it. The _ signifies here that it's just caching a string. I don't see any part in the code where it's ambigious. Question: How long did it take you to understand it? Do others mind this part? (If not, I'd rather drop this issue, since it's really clear in every call either of these vars is involved.) David Gil Oliva wrote: Hmmm, I think you're taking it too seriously... I wouldn't have two variables so similar to each other in my code, but if you think it's ok, please drop it! Yes, you're right, I didn't take it too long to understand your code, but I don't think reviews are meant to be so profound. If I see something that I don't see clear, I say so. I think I don't deserve your tone, because I did it with my best intention. Sebastian Kügler wrote: David, I'm sorry if I came over as unfriendly, rude or condescending. I much appreciate your review and the comments. (Doesn't necessarily mean I agree with all of them, but for discussing this, we have this review. :)) Please don't feel dealt with undeservedly, it was far from my intention. I've changed those name to s_hiddenKey, for example. They're also now static const char[]. - Sebastian --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112660/#review39793 --- 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
Re: Review Request 112660: KPluginInfo rework for KPluginTrader
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112660/#review39793 --- staging/kservice/src/services/kplugininfo.cpp http://git.reviewboard.kde.org/r/112660/#comment29340 I think that two variables (hidden and _hidden) so similar are confusing. staging/kservice/src/services/kplugininfo.cpp http://git.reviewboard.kde.org/r/112660/#comment29339 In kdelibs I don't usually see private members in the _variable form, but m_variable. Nevertheless, members of a private class don't usually have any underscore or m_, because when accessing them as d-name makes it clear that they are private. Probably you have a reason, though :-) . - David Gil Oliva 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
Re: Review Request 112660: KPluginInfo rework for KPluginTrader
On Sept. 11, 2013, 6:22 a.m., David Gil Oliva wrote: staging/kservice/src/services/kplugininfo.cpp, line 61 http://git.reviewboard.kde.org/r/112660/diff/1/?file=188728#file188728line61 I think that two variables (hidden and _hidden) so similar are confusing. Hm, I think it's pretty clear, especially since _hidden is defined above it. The _ signifies here that it's just caching a string. I don't see any part in the code where it's ambigious. Question: How long did it take you to understand it? Do others mind this part? (If not, I'd rather drop this issue, since it's really clear in every call either of these vars is involved.) On Sept. 11, 2013, 6:22 a.m., David Gil Oliva wrote: staging/kservice/src/services/kplugininfo.cpp, line 67 http://git.reviewboard.kde.org/r/112660/diff/1/?file=188728#file188728line67 In kdelibs I don't usually see private members in the _variable form, but m_variable. Nevertheless, members of a private class don't usually have any underscore or m_, because when accessing them as d-name makes it clear that they are private. Probably you have a reason, though :-) . Members in the d-pointer usually don't have the m_ prefix, they're just written in lower case. Maybe you're confusing this with members of the actual class (which, indeed, use m_ normally)? - Sebastian --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112660/#review39793 --- 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
Re: Review Request 112660: KPluginInfo rework for KPluginTrader
On Sept. 11, 2013, 6:22 a.m., David Gil Oliva wrote: staging/kservice/src/services/kplugininfo.cpp, line 61 http://git.reviewboard.kde.org/r/112660/diff/1/?file=188728#file188728line61 I think that two variables (hidden and _hidden) so similar are confusing. Sebastian Kügler wrote: Hm, I think it's pretty clear, especially since _hidden is defined above it. The _ signifies here that it's just caching a string. I don't see any part in the code where it's ambigious. Question: How long did it take you to understand it? Do others mind this part? (If not, I'd rather drop this issue, since it's really clear in every call either of these vars is involved.) Hmmm, I think you're taking it too seriously... I wouldn't have two variables so similar to each other in my code, but if you think it's ok, please drop it! Yes, you're right, I didn't take it too long to understand your code, but I don't think reviews are meant to be so profound. If I see something that I don't see clear, I say so. I think I don't deserve your tone, because I did it with my best intention. On Sept. 11, 2013, 6:22 a.m., David Gil Oliva wrote: staging/kservice/src/services/kplugininfo.cpp, line 67 http://git.reviewboard.kde.org/r/112660/diff/1/?file=188728#file188728line67 In kdelibs I don't usually see private members in the _variable form, but m_variable. Nevertheless, members of a private class don't usually have any underscore or m_, because when accessing them as d-name makes it clear that they are private. Probably you have a reason, though :-) . Sebastian Kügler wrote: Members in the d-pointer usually don't have the m_ prefix, they're just written in lower case. Maybe you're confusing this with members of the actual class (which, indeed, use m_ normally)? Members in the d-pointer usually don't have the m_ prefix Yes, I know, that's because I was confused with the _ prefix. Maybe you're confusing this with members of the actual class (which, indeed, use m_ normally)? No, I'm not :-) - David --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112660/#review39793 --- 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
Re: Review Request 112660: KPluginInfo rework for KPluginTrader
On Sept. 11, 2013, 6:22 a.m., David Gil Oliva wrote: staging/kservice/src/services/kplugininfo.cpp, line 61 http://git.reviewboard.kde.org/r/112660/diff/1/?file=188728#file188728line61 I think that two variables (hidden and _hidden) so similar are confusing. Sebastian Kügler wrote: Hm, I think it's pretty clear, especially since _hidden is defined above it. The _ signifies here that it's just caching a string. I don't see any part in the code where it's ambigious. Question: How long did it take you to understand it? Do others mind this part? (If not, I'd rather drop this issue, since it's really clear in every call either of these vars is involved.) David Gil Oliva wrote: Hmmm, I think you're taking it too seriously... I wouldn't have two variables so similar to each other in my code, but if you think it's ok, please drop it! Yes, you're right, I didn't take it too long to understand your code, but I don't think reviews are meant to be so profound. If I see something that I don't see clear, I say so. I think I don't deserve your tone, because I did it with my best intention. David, I'm sorry if I came over as unfriendly, rude or condescending. I much appreciate your review and the comments. (Doesn't necessarily mean I agree with all of them, but for discussing this, we have this review. :)) Please don't feel dealt with undeservedly, it was far from my intention. - Sebastian --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112660/#review39793 --- 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
Review Request 112660: KPluginInfo rework for KPluginTrader
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/112660/ --- 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
