Review Request: KAuth support in KDM kcontrol module

[Oswald Buddenhagen]

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


hi, thanks, almost there. :)
some of the comments are attached to random instances of problems which appear multiple times. but you seem to be pretty good at extrapolating fixes anyway. :)

do you already know whether kdm-theme will need to change once knewstuff is kauth-ified or whether it will be completely self-contained?


/trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp
<http://reviewboard.kde.org/r/3631/#comment5227>

    any particular reason to postpone the deletion of directories?
    and you probably want to call the function recursively here.



/trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp
<http://reviewboard.kde.org/r/3631/#comment5226>

    i don't like such constructs. imo,
    
       else if (!...)
           res = false;
    
    would be a lot clearer



/trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp
<http://reviewboard.kde.org/r/3631/#comment5225>

    the implementation must not contain the default value. with stricter settings, the compiler will complain.



/trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp
<http://reviewboard.kde.org/r/3631/#comment5222>

    good catch.
    though i would just complain if the name contains a slash. seems less complicated.
    



/trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp
<http://reviewboard.kde.org/r/3631/#comment5224>

    else
       res = false;
    
    then you don't need the temporary, either.



/trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-theme.cpp
<http://reviewboard.kde.org/r/3631/#comment5229>

    "errors [while] installing" (our grammar fixers changed it in my code previously, so i guess it must be correct :).
    
    "... [themes]".
    



/trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-theme.cpp
<http://reviewboard.kde.org/r/3631/#comment5230>

    as usual in foreach loops: const QString &path
    



/trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-theme.cpp
<http://reviewboard.kde.org/r/3631/#comment5231>

    this kind of makes no sense ... you couldn't be trying to delete anything if the directory didn't exist.



/trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-theme.cpp
<http://reviewboard.kde.org/r/3631/#comment5232>

    just iterate using an integer index. qlist has O(1) random access (but use at(i), not [i] to avoid useless detach() calls).
    


- Oswald


On 2010-05-05 20:08:34, Igor Krivenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3631/
> -----------------------------------------------------------
> 
> (Updated 2010-05-05 20:08:34)
> 
> 
> Review request for kdelibs, usability, Dario Freddi, Oswald Buddenhagen, and Frederik Gladhorn.
> 
> 
> Summary
> -------
> 
> This is a patch for KDM kcontrol module.
> It implements Kauth support for changing kdm settings and managing user images. 
> It still doesn't support installation/deletion of themes (including installation via knewstuff).
> As far as I know there were plans to port KNewStuff3 to KAuth, so a more consistent approach is
> to rely on the KNewStuff's integration when it's available.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/CMakeLists.txt 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/background.h 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/background.cpp 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.h PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/helper.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kcmkdm_actions.actions PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-conv.h 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-conv.cpp 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-dlg.h 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-dlg.cpp 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-gen.h 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-gen.cpp 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-shut.h 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-shut.cpp 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-theme.h 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-theme.cpp 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-users.h 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-users.cpp 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/main.h 1121893 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/main.cpp 1121893 
> 
> Diff: http://reviewboard.kde.org/r/3631/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Saving KDM settings
>   http://reviewboard.kde.org/r/3631/s/360/
> Managing user images
>   http://reviewboard.kde.org/r/3631/s/361/
> 
> 
> Thanks,
> 
> Igor
> 
>