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.

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

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