Review Request: Add a new KPasswordLineEditidget

Fri Jan 15 00:42:35 GMT 2010

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

I only had time to review this far but here goes. Keep in mind though that it's still probably easier just to port the applications in question to the password dialog than to make a new widget and then still have to port all the applications to the new widget.

Even though I don't think this would be a good fit for kdelibs though I don't want you to come away empty-handed without anyone looking at your code. ;)

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.h
<http://reviewboard.kde.org/r/2591/#comment3046>

    should be "keyboard"

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.h
<http://reviewboard.kde.org/r/2591/#comment3047>

    This isn't an LED widget, make sure the description in quotes is accurate.

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.h
<http://reviewboard.kde.org/r/2591/#comment3048>

    No need to put the year by your name (copyright is recorded in the source code).

    On the other hand you should add a @since 4.5 annotation if you were to try to add this to KDE to document what version it became available.

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.h
<http://reviewboard.kde.org/r/2591/#comment3050>

    If you're not going to add this property don't have it in the header, even commented out (likewise for the other mentions below).

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.h
<http://reviewboard.kde.org/r/2591/#comment3049>

    You should prefer Qt-style naming instead of Java-style.

    i.e. simply passwordKLineEdit(), likewise for the other getters.

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.h
<http://reviewboard.kde.org/r/2591/#comment3052>

    Should be a const method (see below for an example).

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.h
<http://reviewboard.kde.org/r/2591/#comment3051>

    This should be a const-method since it simply returns a value.

    i.e. QString password() const;

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.h
<http://reviewboard.kde.org/r/2591/#comment3053>

    I /think/ some compilers choke on declaring a class and then declaring a friend class. IIRC you can simply declare a friend class.

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.h
<http://reviewboard.kde.org/r/2591/#comment3055>

    Private classes do not get defined in the header file for the matching public class. (i.e. move this to either kpasswordlineedit_p.h and include from the .cpp or simply move it to kpasswordlineedit.cpp directly)

    There are many examples in Qt and KDE to see how we do it.

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.h
<http://reviewboard.kde.org/r/2591/#comment3054>

    You have extra whitespace every so often for no reason (easy to see in the review board diff output)

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.cpp
<http://reviewboard.kde.org/r/2591/#comment3056>

    Your coding style is in general pretty good but here you mix up ( spacing ) with (spacing).

    See http://techbase.kde.org/Policies/Kdelibs_Coding_Style for the coding style to prefer for kdelibs code, there's other issues (e.g. use 4 spaces for each level of indentation)

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.cpp
<http://reviewboard.kde.org/r/2591/#comment3057>

    Is there any reason not to move this to the private class's init() so that it is shared?

    If you're worried about having to optionally setText or not then just pass the string to init in the 2-arg constructor and pass an empty string (QString()) in the single argument constructor.

    Also you mentioned issues with memory leaks: Here's one right here, you create a KModifierKeyInfo with no parent. Instead pass "this". (e.g. info = new KModifierKeyInfo(this))

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.cpp
<http://reviewboard.kde.org/r/2591/#comment3058>

    Another mem leak. (IIRC QLayouts do not take ownership of widgets, they only arrange them)

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.cpp
<http://reviewboard.kde.org/r/2591/#comment3059>

    Memory leak.

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.cpp
<http://reviewboard.kde.org/r/2591/#comment3060>

    I don't do color changes much but I think the "palette" property of the QWidget is the best way to change the color (and you may be able to find a suitable color role for any color theme instead of hardcoding red this way as well)

trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.cpp
<http://reviewboard.kde.org/r/2591/#comment3063>

    mem leak.

- Michael

On 2010-01-14 13:56:08, Charles Ghislain wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2591/
> -----------------------------------------------------------
> 
> (Updated 2010-01-14 13:56:08)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> Hi,
> 
> I started adding a warning label to kpassworddialog, then i noticed that password fields were used at various place outside a kpassworddialog or a knewpassworddialog. Take for instance kmail, konversation,
> kdm, ... I was told the idea is that applications should use those dialogs, but the reallity seems different. Implementing the capslock warning in a new kpasswordlineedit allow all those apps to get the benefits.
> 
> This is my first contribution, I'm new to C++ and KDE and Qt in general. I tried copying from other sources i found, but there are still issues i can't resolve by myself yet :
> - Memory leaks : i don't know were is it - should i delete the childeren widgets in the destructor?
> - Formatting : i guess it refers to the widget having differenc height when the label is shown or not. I don't know how to make sure the widget keeps a fixed height even if one of his childeren is hidden. I could just set the text to label to a whitespace and keep is shown, but i guess there is a proper way.
> - Warning color : I used hardcoded red, i didn't find how to use themed colors
> - Others issues i don't think of right know.
> 
> Thanks,
> 
> Charly
> 
> 
> This addresses bug 91970.
>     https://bugs.kde.org/show_bug.cgi?id=91970
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kdeui/CMakeLists.txt 1074640 
>   trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.h PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kdewidgets/kde.widgets 1074640 
> 
> Diff: http://reviewboard.kde.org/r/2591/diff
> 
> 
> Testing
> -------
> 
> I tested in designer and replaced the password field of kpassworddialog with it.
> 
> 
> Thanks,
> 
> Charles
> 
>