> 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
