Review Request 112660: KPluginInfo rework for KPluginTrader

[Sebastian Kügler]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130912/b52e3641/attachment.html>