Review Request: KDateEdit moving into kdelibs

John Layt johnlayt at googlemail.com
Tue Nov 30 01:38:06 GMT 2010 

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


Various points:
1) Doesn't quite use kdelibs coding standard (yeah, I prefer more spaces too but that's the standard)
2) Doesn't localize the date maths anywhere, I'd suggest using KLocalizedDate internally to make this easy.
3) The KDateValidator fixup() looks broken to me for anything other than dd/mm/yyyy format.
4) You may want to consider adding api to KDateEdit and KDateValidator to set the KCalendarSystem to be used.  While it's a corner case, it would allow the use of a different calendar system or date format than the global.  KDatePicker supports this, and using KLocalizedDate internally would make it easy.
5) Looking at QDateEdit, do we want api for minimum/maximum date and enable/disable pop-up?
6) Given the widget doesn't extend QDateEdit or even QSpinBox, should it be named something else?
7) Unit tests?

Being based on QComboBox, I just wonder what happens if the dev starts messing with the underlying api like add/remove items?

If we don't get this into 4.6, then we can look at doing something like Qt does with QDateTimeEdit being the base class and QDateEdit and QTimeEdit just being special case sub-classes of that.


/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.h
<http://svn.reviewboard.kde.org/r/6014/#comment9802>

    No it's not :-)



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.h
<http://svn.reviewboard.kde.org/r/6014/#comment9803>

    kdelibs coding standard is no spaces after/before brackets.



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9843>

    No it isn't



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9816>

    Consider making this a KLocalizedDate, it will save having to refer to the global calendar KGlobal::locale()->calendar() all the time (see all points below were you wouldn't have to change code!).  But leave the api as QDate.  See http://www.layt.net/john/blog/odysseus/calendar_systems_in_46



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9804>

    Needs to be checked against the global calendar KGlobal::locale()->calendar()->isValid( d->mDate ), or just use KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9805>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9837>

    What's the purpose of the bool replaced, it always gets set to false?  The whole method could be reduced to a single line.



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9810>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9806>

    Needs to use global calendar, either KGlobal::locale()-calendar()->addMonths(date, 1), or better yet change declaration at line 251 from QDate to KLocalizedDate and you won't have to change another line.



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9809>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9807>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9811>

    Use global calendar or KLocalizedDate.  Actually, you can skip the isValid() and just do the addMonths() as it does a validity check internally.



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9808>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9812>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9813>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9814>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9815>

    Use global calendar or KLocalizedDate, also not really needed as formatDate() does a validity test and returns QString() if not valid.



/trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9817>

    If using KLocalizedDate can just be mDate.formatDate(KLocale::ShortDate)



/trunk/KDE/kdelibs/kdeui/widgets/kdatepickerpopup.h
<http://svn.reviewboard.kde.org/r/6014/#comment9842>

    No it isn't :-)



/trunk/KDE/kdelibs/kdeui/widgets/kdatepickerpopup.h
<http://svn.reviewboard.kde.org/r/6014/#comment9840>

    Why is this disabled?



/trunk/KDE/kdelibs/kdeui/widgets/kdatepickerpopup.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9841>

    No it isn't :-)



/trunk/KDE/kdelibs/kdeui/widgets/kdatepickerpopup.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9844>

    Do we want more options like Yesterday, weekday names (Monday etc), Last Week, Last Month, Next Year, Last Year, etc?  See also Fancy Date format in KLocale.  Would using an enum stored as the item data be a more flexible approach allowing adding more options without adding more and more slots?



/trunk/KDE/kdelibs/kdeui/widgets/kdatepickerpopup.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9818>

    Needs to use the global calendar addMonths()



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.h
<http://svn.reviewboard.kde.org/r/6014/#comment9819>

    No it isn't :-)



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.h
<http://svn.reviewboard.kde.org/r/6014/#comment9846>

    Perhaps FixupFuture / FixupPast or FixupNext / FixupPrevious?



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9820>

    No it isn't :-)



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9834>

    This whole routine needs a lot more inline comments explaining the magic, or use something more obvious.



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9826>

    Change QDate to KLocalizedDate, or fix all references below to use global calendar



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9821>

    Change QDate to KLocalizedDate, or change all maths below to use global calendar



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9824>

    This use of magic numbers is confusing, can't you use an enum instead?



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9822>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9823>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9825>

    If I understand this right, then this is seriously wrong.  If it's expecting the user to enter a date in dd/mm/yyyy format then this will not work in most countries.  You cannot rely on either teh separator being a / or the it being day/month/year order.



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9836>

    Use global calendar or KLocalizedDate for both day() and addDays()



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9827>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9828>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9829>

    Use global calendar or KLocalizedDate for both month() and addMonths()



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9830>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9831>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9832>

    Use global calendar or KLocalizedDate



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9833>

    This is a rather confusing, sure you can't use a private enum?



/trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp
<http://svn.reviewboard.kde.org/r/6014/#comment9835>

    Needs to use global calendar to check isValid() instead


- John


On 2010-11-29 22:11:34, Kevin Ottens wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6014/
> -----------------------------------------------------------
> 
> (Updated 2010-11-29 22:11:34)
> 
> 
> Review request for kdelibs and KDE PIM.
> 
> 
> Summary
> -------
> 
> For a very long time now there's been a KDateEdit widget cooking up in kdepim. It's in fact a popular one, and I'm aware of around half a dozen (sometimes modified) copies of that widget. One of the most advanced fork is in Skrooge ATM. During the latest KDE Hacking Session in Toulouse, I sat down with the Skrooge maintainer and produced a refactored version of KDateEdit which also cover their needs.
> 
> This patch is about introducing this widget in kdeui. It basically comes with three new classes:
>  - KDateEdit itself;
>  - KDatePickerPopup used by KDateEdit to popup a calendar picker;
>  - KDateValidator used by KDateEdit to validate the input, it's made public as some of the fixup behavior can be tuned.
> 
> Note that KDateTable already exposed a class named KDateValidator (much less advanced), so I took care of copying the extra method the former KDateValidator was exposing for BC reasons.
> 
> I'm aiming for inclusion in 4.6, so a prompt review would be welcome. Thanks in advance. ;-)
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/includes/CMakeLists.txt 1201925 
>   /trunk/KDE/kdelibs/includes/KDateEdit PRE-CREATION 
>   /trunk/KDE/kdelibs/includes/KDatePickerPopup PRE-CREATION 
>   /trunk/KDE/kdelibs/includes/KDateValidator 1201925 
>   /trunk/KDE/kdelibs/kdeui/CMakeLists.txt 1201925 
>   /trunk/KDE/kdelibs/kdeui/widgets/kdateedit.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kdeui/widgets/kdateedit.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/kdeui/widgets/kdatepickerpopup.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kdeui/widgets/kdatepickerpopup.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/kdeui/widgets/kdatetable.h 1201925 
>   /trunk/KDE/kdelibs/kdeui/widgets/kdatetable.cpp 1201925 
>   /trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.h PRE-CREATION 
>   /trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator.cpp PRE-CREATION 
>   /trunk/KDE/kdelibs/kdeui/widgets/kdatevalidator_p.h PRE-CREATION 
> 
> Diff: http://svn.reviewboard.kde.org/r/6014/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Kevin
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

More information about the kde-core-devel mailing list