Review Request: Corona::exportLayout

[Aaron Seigo]

> On 2010-09-26 20:22:05, Aaron Seigo wrote:
> > trunk/KDE/kdelibs/plasma/corona.cpp, line 340
> > <http://svn.reviewboard.kde.org/r/5451/diff/1/?file=38404#file38404line340>
> >
> >     this should probably take a KConfigGroup, since KConfigGroup can refer to the top level item, e.g. the whole config file, using the magic QString() name for the group.
> >     
> >     unfortunately, Corona::importLayout() requires a KConfigBase which would make this assymetrical. iirc that bit of the API was written before i was aware of that KConfigGroup trick (which was actually undocumented until i found out about it :).
> >     
> >     perhaps we should add an importLayout that takes a KConfigGroup, mark the importLayout which takes a KConfigBase as deprecaton and have it call the new one.
> >     
> >     then we have a nice api with symmetry?
> 
> Chani Armitage wrote:
>     so... void Corona::exportLayout(KConfigGroup *config, QList<Containment*> containments)
>     and void Corona::importLayout(KConfigGroup *config)
> 
> Aaron Seigo wrote:
>     KConfigGroup &, but otherwise, yes...
> 
> Chani Armitage wrote:
>     I thought it was bad form to use & without const...?
> 
> Chani Armitage wrote:
>     oh, for import is would be const KConfigGroup &, duhh. :)
>     but for export, KConfigGroup* is more correct, right?

it's usually considered bad form, but in the case it's fine and prevents having to beware of null pointers in the lib code and keeps the API as symmetric as possible. it's also how we've handled KConfigGroup elsewhere in libplasma, and there are other areas of kde that do similarly. for KConfigGroup it simpler and sensible to pass it around by reference in these cases.


- Aaron


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5451/#review7824
-----------------------------------------------------------


On 2010-09-27 14:38:42, Chani Armitage wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5451/
> -----------------------------------------------------------
> 
> (Updated 2010-09-27 14:38:42)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> this adds exportLayout to corona, which saves a group of containments to a config file and deletes them from the main config.
> 
> Activity::close() becomes a lot shorter by calling it, like this: m_corona->exportLayout(external, m_containments.values());
> 
> I feel a bit out of it today though, so tell me if I've missed anything obvious...
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/plasma/corona.h 1179499 
>   trunk/KDE/kdelibs/plasma/corona.cpp 1179499 
> 
> Diff: http://svn.reviewboard.kde.org/r/5451/diff
> 
> 
> Testing
> -------
> 
> closing an activity while locked is perfectly safe now :)
> 
> 
> Thanks,
> 
> Chani
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20100927/e5c25388/attachment-0001.htm