Review Request: New Date/Time Widgets in kdelibs/kdeui

David Jarvie djarvie at kde.org
Mon Jun 13 14:05:58 BST 2011 

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


There are no class descriptions.

To be comprehensive, KDateTimeEdit should have the option to be able to handle date-only KDateTime values. This would require an extra checkbox to select date-only. (KAlarm implements this in its AlarmTimeWidget class, if that's any help.)


kdeui/widgets/kdatecombobox.h
<http://git.reviewboard.kde.org/r/101575/#comment3172>

    Enlarge the description to clarify what a "minimum warning message" is for



kdeui/widgets/kdatecombobox.h
<http://git.reviewboard.kde.org/r/101575/#comment3173>

    Clarify what this is for



kdeui/widgets/kdatecombobox.h
<http://git.reviewboard.kde.org/r/101575/#comment3174>

    Clarify what this is for



kdeui/widgets/kdatecombobox.h
<http://git.reviewboard.kde.org/r/101575/#comment3175>

    Clarify what this is for



kdeui/widgets/kdatecombobox.h
<http://git.reviewboard.kde.org/r/101575/#comment3176>

    Spelling: "separator"



kdeui/widgets/kdatecombobox.cpp
<http://git.reviewboard.kde.org/r/101575/#comment3168>

    Use i18nc("@info",...)



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3157>

    Needs clarification. The description in KTimeComboBox is more understandable - use it unless the meaning is supposed to be different.



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3139>

    const



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3140>

    space before 'const' - same on various other lines too



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3146>

    Need to say something about the effects of supplying a date-only KDateTime parameter



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3143>

    State what happens when one valid and one invalid date are specified? Can this be used to set only a minimum or only a maximum?



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3144>

    Enlarge the description to clarify what a "minimum warning message" is for



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3145>

    Enlarge the description to clarify what a "maximum warning message" is for



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3147>

    Clarify what the message is for



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3148>

    Clarify what the message is for



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3149>

    maximum



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3150>

    Spelling: "separator"



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3154>

    This isn't in the Options enum. ForceTime is, though - is this a typo? Mind you, I think the name ForceInterval is more understandable than ForceTime if that is indeed what it's about.



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3151>

    Clarify what the message is for



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3152>

    Clarify what the message is for



kdeui/widgets/kdatetimeedit.h
<http://git.reviewboard.kde.org/r/101575/#comment3153>

    Mention what the default set of time zones is



kdeui/widgets/kdatetimeedit.cpp
<http://git.reviewboard.kde.org/r/101575/#comment3158>

    Rather unconventional spacing - looks at first sight like an address-of operator. More easily understandable with a space after '&'.
    Same comment applies to succeeding lines too.



kdeui/widgets/kdatetimeedit.cpp
<http://git.reviewboard.kde.org/r/101575/#comment3160>

    Use i18nc() semantic markup



kdeui/widgets/kdatetimeedit.cpp
<http://git.reviewboard.kde.org/r/101575/#comment3159>

    Use i18nc() semantic markup, and provide context for translators to make "Floating" meaning clear



kdeui/widgets/kdatetimeedit.cpp
<http://git.reviewboard.kde.org/r/101575/#comment3161>

    Leave a blank line, or something, to help readers distinguish between if() and the following code block



kdeui/widgets/ktimecombobox.h
<http://git.reviewboard.kde.org/r/101575/#comment3162>

    const



kdeui/widgets/ktimecombobox.h
<http://git.reviewboard.kde.org/r/101575/#comment3163>

    const



kdeui/widgets/ktimecombobox.h
<http://git.reviewboard.kde.org/r/101575/#comment3165>

    Clarify



kdeui/widgets/ktimecombobox.h
<http://git.reviewboard.kde.org/r/101575/#comment3164>

    Clarify



kdeui/widgets/ktimecombobox.h
<http://git.reviewboard.kde.org/r/101575/#comment3167>

    There should be an option to allow the user to enter times greater than 24 hours, so that arbitrary time intervals can be entered. This needs extra methods taking and returning 'int' values instead of QTime values.



kdeui/widgets/ktimecombobox.h
<http://git.reviewboard.kde.org/r/101575/#comment3156>

    ForceInterval isn't in the enum - is this a typo for ForceTime?



kdeui/widgets/ktimecombobox.cpp
<http://git.reviewboard.kde.org/r/101575/#comment3171>

    Use i18nc("@info", ...)


- David


On June 10, 2011, 9:18 p.m., John Layt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101575/
> -----------------------------------------------------------
> 
> (Updated June 10, 2011, 9:18 p.m.)
> 
> 
> Review request for kdelibs, KDEPIM, KPhotoAlbum, Skrooge, Zanshin, Kevin Ottens, and David Jarvie.
> 
> 
> Summary
> -------
> 
> [Sorry this is a post-commit review and took so long to remember to post. Bad coder, no cookie for you!]
> 
> This review is for some new replacement widgets for the popular KDEPIM KDateEdit and KTimeEdit widgets which were copied into a number of other projects.  These new widgets are clean rewrites (the original widgets have history back to KDE2 days) with slightly changed api but otherwise should replicate the same functionality with a couple of new features.  They will be available for use by any apps using kdelibs 4.7.
> 
> The 3 new widgets are:
> 
> KDateComboBox: A date entry widget derived from KComboBox, the drop-down menu can display a date picker and list of "fancy" dates to choose from.  The list of dates can be configured.
> 
> KTimeComboBox: A time entry widget derived from KComboBox, the drop-down menu can display a list of times to choose from.  The list of times can be configured.
> 
> KDateTimeEdit: A KDateTime entry widget combining KDateComboBox and KTimeComboBox with optional combo's to select the calendar system and time spec as well. This widget should only be used if you want time spec aware data entry.
> 
> All widgets can accept a null or invalid input, it is up to the coder to check for validity of input using isValid() if required.  All feature options of the widgets can be configured.  All widgets can optionally display a warning box on focus out if the entry is invalid.  All widgets can be used in Qt Designer.
> 
> I'm particularly looking for input on the api, and what QWidget event virtual methods I should be reimplementing to make the classes BIC future-proof.
> 
> 
> Diffs
> -----
> 
>   kdeui/CMakeLists.txt 1e8b259 
>   includes/KDateComboBox PRE-CREATION 
>   includes/KDateTimeEdit PRE-CREATION 
>   includes/KTimeComboBox PRE-CREATION 
>   includes/CMakeLists.txt 7a8bc5c 
>   kdeui/tests/CMakeLists.txt c7b8026 
>   kdeui/tests/kdatecomboboxtest.h PRE-CREATION 
>   kdeui/tests/kdatecomboboxtest.cpp PRE-CREATION 
>   kdeui/tests/kdatetimeedittest.h PRE-CREATION 
>   kdeui/tests/kdatetimeedittest.cpp PRE-CREATION 
>   kdeui/tests/ktimecomboboxtest.h PRE-CREATION 
>   kdeui/tests/ktimecomboboxtest.cpp PRE-CREATION 
>   kdeui/widgets/kdatecombobox.h PRE-CREATION 
>   kdeui/widgets/kdatecombobox.cpp PRE-CREATION 
>   kdeui/widgets/kdatetimeedit.h PRE-CREATION 
>   kdeui/widgets/kdatetimeedit.cpp PRE-CREATION 
>   kdeui/widgets/kdatetimeedit.ui PRE-CREATION 
>   kdeui/widgets/ktimecombobox.h PRE-CREATION 
>   kdeui/widgets/ktimecombobox.cpp PRE-CREATION 
>   kdewidgets/kde.widgets 9040538 
> 
> Diff: http://git.reviewboard.kde.org/r/101575/diff
> 
> 
> Testing
> -------
> 
> Unit tests written for non-gui functionality.  Gui functionality tested in Qt Designer.  KDateTimeEdit still has a couple of minor bugs, but I didn't want to hold the review up any longer.
> 
> 
> Screenshots
> -----------
> 
> Qt Designer Preview
>   http://git.reviewboard.kde.org/r/101575/s/180/
> KDateComboBox
>   http://git.reviewboard.kde.org/r/101575/s/181/
> KTimeComboBox
>   http://git.reviewboard.kde.org/r/101575/s/182/
> 
> 
> Thanks,
> 
> John
> 
>

_______________________________________________
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