> 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

Reply via email to