Review Request: Add support for subgroups to kconf_update

Aurélien Gâteau agateau at kde.org
Mon Apr 19 13:47:00 BST 2010



> On 2010-04-17 09:31:44, Oswald Buddenhagen wrote:
> > having never used kconf_update myself and not knowing the code, this is mostly only a formal review as before. therefore you won't get a "ship it" from me - commit at your own peril. :)

I guess the only developer who knows the code is Waldo Bastian... :/


> On 2010-04-17 09:31:44, Oswald Buddenhagen wrote:
> > trunk/KDE/kdelibs/kconf_update/kconf_update.cpp, line 77
> > <http://reviewboard.kde.org/r/3623/diff/1/?file=23727#file23727line77>
> >
> >     copyMove*(), etc. are confusing. maybe copyOrMove*()

Fixed


> On 2010-04-17 09:31:44, Oswald Buddenhagen wrote:
> > trunk/KDE/kdelibs/kconf_update/kconf_update.cpp, line 739
> > <http://reviewboard.kde.org/r/3623/diff/1/?file=23727#file23727line739>
> >
> >     here you could use the method factored out above

Fixed


> On 2010-04-17 09:31:44, Oswald Buddenhagen wrote:
> > trunk/KDE/kdelibs/kconf_update/kconfigutils.cpp, line 68
> > <http://reviewboard.kde.org/r/3623/diff/1/?file=23729#file23729line68>
> >
> >     not that it would matter here, but you could save an allocation by rewriting this as
> >       str.chop(1);
> >       str.remove(0, 1);
> >

Fixed


> On 2010-04-17 09:31:44, Oswald Buddenhagen wrote:
> > trunk/KDE/kdelibs/kconf_update/kconf_update.cpp, line 544
> > <http://reviewboard.kde.org/r/3623/diff/1/?file=23727#file23727line544>
> >
> >     of course, the way this splitting is done prevents group names containing commas (except if somebody had the idea to hex-escape it). i'm not too worried by that, though ...

Good point, but this is not a regression from the original code.

Maybe I should add something like this to the README: "To specify a group name with a comma you have to hex-escape it: turn "foo,bar" into "foo\x2cbar".  ".


- Aurélien


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


On 2010-04-19 12:43:00, Aurélien Gâteau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3623/
> -----------------------------------------------------------
> 
> (Updated 2010-04-19 12:43:00)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This huge patch adds the ability to manipulate subgroups to kconf_update. It is now possible to specify a group either as "group" or as "[group][subgroup]".
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kconf_update/CMakeLists.txt 1115427 
>   trunk/KDE/kdelibs/kconf_update/README.kconf_update 1115427 
>   trunk/KDE/kdelibs/kconf_update/kconf_update.cpp 1115427 
>   trunk/KDE/kdelibs/kconf_update/kconfigutils.h PRE-CREATION 
>   trunk/KDE/kdelibs/kconf_update/kconfigutils.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kconf_update/tests/CMakeLists.txt 1115427 
>   trunk/KDE/kdelibs/kconf_update/tests/test_kconf_update.cpp 1115427 
>   trunk/KDE/kdelibs/kconf_update/tests/test_kconfigutils.h PRE-CREATION 
>   trunk/KDE/kdelibs/kconf_update/tests/test_kconfigutils.cpp PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/3623/diff
> 
> 
> Testing
> -------
> 
> Unit-tests for kconf_update I wrote before adding support for subgroups still pass. I also extended the existing tests and added new ones to test the subgroup support.
> 
> 
> Thanks,
> 
> Aurélien
> 
>





More information about the kde-core-devel mailing list