[KDE Usability] 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/#review5349
-----------------------------------------------------------


hi. closer again. :)


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

    this looks strange. is it for i18n? why isn't it in the other function? isn't there a way to put it in main()?



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

    fwiw, this should use toLocalFile()



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

    the variable name should start with a lowercase letter.
    anyway, this looks like dead code?



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

    it's terrible style to special-case the behavior of a function like that. obviously the pattern isn't as simple as i thought. you'll need two functions: a generic one to handle the errors, and one to execute the face management actions (and call the error handler). the save action is needed only once, so it needs no function.


- Oswald


On 2010-04-25 20:30:08, Igor Krivenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3631/
> -----------------------------------------------------------
> 
> (Updated 2010-04-25 20:30:08)
> 
> 
> 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 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/background.h 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/background.cpp 1118718 
>   /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 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-conv.cpp 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-dlg.h 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-dlg.cpp 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-gen.h 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-gen.cpp 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-shut.h 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-shut.cpp 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-theme.h 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-theme.cpp 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-users.h 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/kdm-users.cpp 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/main.h 1118718 
>   /trunk/KDE/kdebase/workspace/kcontrol/kdm/main.cpp 1118718 
> 
> 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
> 
>