Review Request: Add a new KPasswordLineEditidget

Charles Ghislain charlyghislain at gmail.com
Fri Jan 15 11:48:09 GMT 2010



> On 2010-01-15 00:42:42, Michael Pyne wrote:
> > 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. ;)

Thanks you and pino a lot for reviewing !

I'll merge your corrections tomorrow and submit a new patch.
In the meanwhile, Ill start a thread on the list to see if it should be considered for inclusion in kdelibs.

Thank you good night!


> On 2010-01-15 00:42:42, Michael Pyne wrote:
> > trunk/KDE/kdelibs/kdeui/widgets/kpasswordlineedit.h, line 98
> > <http://reviewboard.kde.org/r/2591/diff/1/?file=17069#file17069line98>
> >
> >     I /think/ some compilers choke on declaring a class and then declaring a friend class. IIRC you can simply declare a friend class.

Since I copied this one from another widget, can i include corrections to other files in this review?


- Charles


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


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
> 
>





More information about the kde-core-devel mailing list