New Date/Time Widgets in kdelibs/kdeui

Review Request #101575 - Created June 10, 2011 and submitted

Information
John Layt
kdelibs
Reviewers
kdelibs, kdepim, kphotoalbum, skrooge, zanshin
djarvie, ervin
[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.
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.
Sergio Martins

   
kdeui/widgets/kdatecombobox.h (Diff revision 1)
 
 
const
kdeui/widgets/kdatecombobox.h (Diff revision 1)
 
 
const
kdeui/widgets/kdatecombobox.cpp (Diff revision 1)
 
 
Use if ( format != d->m_displayFormat ).

And similiar in other setters
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
const
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
const
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
const
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
const
I recommend using virtual hooks, to help with BC/BIC issues: ( grep virtual_hook kdepimlibs/kcalcore/* )
Jarosław Staniek

   
kdeui/widgets/kdatecombobox.h (Diff revision 1)
 
 
missing space before const here and 4 other places
kdeui/widgets/ktimecombobox.cpp (Diff revision 1)
 
 
perhaps no need to have m_ prefix for attributes in the private impl class? d->m_* looks a bit too verbose
  1. ... except if there is actually code in d.
kdeui/widgets/ktimecombobox.cpp (Diff revision 1)
 
 
couldn't ki18nc() be used in such places and then KLocalizedString::subs() instead of QString::replace()?
David Jarvie
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 (Diff revision 1)
 
 
Enlarge the description to clarify what a "minimum warning message" is for
kdeui/widgets/kdatecombobox.h (Diff revision 1)
 
 
Clarify what this is for
kdeui/widgets/kdatecombobox.h (Diff revision 1)
 
 
Clarify what this is for
kdeui/widgets/kdatecombobox.h (Diff revision 1)
 
 
Clarify what this is for
kdeui/widgets/kdatecombobox.h (Diff revision 1)
 
 
Spelling: "separator"
kdeui/widgets/kdatecombobox.cpp (Diff revision 1)
 
 
Use i18nc("@info",...)
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
Needs clarification. The description in KTimeComboBox is more understandable - use it unless the meaning is supposed to be different.
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
const
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
space before 'const' - same on various other lines too
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
Need to say something about the effects of supplying a date-only KDateTime parameter
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
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 (Diff revision 1)
 
 
Enlarge the description to clarify what a "minimum warning message" is for
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
Enlarge the description to clarify what a "maximum warning message" is for
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
Clarify what the message is for
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
Clarify what the message is for
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
maximum
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
Spelling: "separator"
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
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 (Diff revision 1)
 
 
Clarify what the message is for
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
Clarify what the message is for
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
Mention what the default set of time zones is
kdeui/widgets/kdatetimeedit.cpp (Diff revision 1)
 
 
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 (Diff revision 1)
 
 
Use i18nc() semantic markup
kdeui/widgets/kdatetimeedit.cpp (Diff revision 1)
 
 
Use i18nc() semantic markup, and provide context for translators to make "Floating" meaning clear
kdeui/widgets/kdatetimeedit.cpp (Diff revision 1)
 
 
Leave a blank line, or something, to help readers distinguish between if() and the following code block
kdeui/widgets/ktimecombobox.h (Diff revision 1)
 
 
const
kdeui/widgets/ktimecombobox.h (Diff revision 1)
 
 
const
kdeui/widgets/ktimecombobox.h (Diff revision 1)
 
 
Clarify
kdeui/widgets/ktimecombobox.h (Diff revision 1)
 
 
Clarify
kdeui/widgets/ktimecombobox.h (Diff revision 1)
 
 
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 (Diff revision 1)
 
 
ForceInterval isn't in the enum - is this a typo for ForceTime?
kdeui/widgets/ktimecombobox.cpp (Diff revision 1)
 
 
Use i18nc("@info", ...)
Kevin Ottens
Looks good already, just a couple of API nitpicks.
kdeui/widgets/kdatecombobox.h (Diff revision 1)
 
 
Hm, IIRC you should have the full enum name here, otherwise you won't be able to make a signal connectable to this slot. Besides I'm not sure it makes sense to make a slot out of this one. I'd expect the usage pattern to be mostly about calling that once after creation.
kdeui/widgets/kdatecombobox.h (Diff revision 1)
 
 
What's the default value you're referring to? What about using an invalid QDate for the reset, and then have only the setter?
kdeui/widgets/kdatecombobox.h (Diff revision 1)
 
 
Same comment than for minimum.
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
Same comments than for KDateComboBox::setOptions
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
Same comment than for KDateComboBox
kdeui/widgets/kdatetimeedit.h (Diff revision 1)
 
 
See other comments I added on reset*()
kdeui/widgets/ktimecombobox.h (Diff revision 1)
 
 
Always the same old song. (see other setOptions comments) ;-)
Torgny Nyblom
Any progress?
  1. This is the monthly reminder that you cannot disable :P
  2. Hee hee :-)  Sorry, long since implemented, was in the 4.7 release.  I just need to finish the apidox which are a bit sparse, but I won't keep it open just for that.
Loading...