[Kde-pim] Re: KDateEdit and KTimeEdit

[David Jarvie]

On Thu, May 12, 2011 12:51 am, John Layt wrote:
> On Tuesday 10 May 2011 00:26:31 John Layt wrote:
>> Some queries:
>> 1) Do we stick with the KDateTimeEdit class name?  It implies it's a
>> drop-in replacement for QDateTimeEdit which the api is but its's not a
>> derived class.
>> 2) Is there a nicer way to do the date picker pop-up?
>> 3) Any more timezone api needed, such as manually adding extra tz's to
>> display?
>> 4) Any virtuals I need to add to all kdepim to derive new classes from
>> it?
>
> OK, I'm getting close on this now, it mostly works, but it needs a bit of
> polishing and the date/time validation isn't all there yet.  I'll do a
> little more cleaning Thursday and try break out the separate date/time
combo
> classes if time allows, then push it to master.
>
> If anyone could spare 15 minutes to look at the api in kdatetimeedit.h [1]
> and let me know of any issues they spot, while we do have a beta cycle to
> shake out any issues I'd rather not be changing the api too often during
the
> soft api freeze.

Good work John! Here are some comments from scanning the code.

In the Option enum, need to clarify the difference between EditTime and
SelectTime, etc. Does the SelectTime option only allow selection from a
list of predetermined times instead of manually editing the time, or what?

The method name dateTimeSpec() isn't very intuitive, and for people
familiar with KDateTime is positively misleading. Not sure what would be
better - perhaps absoluteDateTime()?. Its name doesn't sit easily with
methods whose names contain timeSpec.

Wouldn't it be better to use timeZone instead of timeSpec in the method
naming scheme - apart from the Floating option, it only handles time zones
so even for KDateTime aficionados seems more intuitive.

Time zone is two separate words - should be separated in api docs and
method names should have "zone" capitalised.

setMaximumDateTime() and setMinimumDateTime() should have the option of a
KDateTime parameter so that people can't enter a date/time outside the
intended limits by changing the time zone.

Do the set...(), set...Range(), setMaximum...() and setMinimum...()
methods need to specify Time, Date, Datetime in their names? Their
parameter types define what they do. Perhaps it's beneficial to have a
one-to-one naming correspondence between the get and set methods, though.

For the eventual KTimeEdit class, in addition to entering a time of day,
it should be able to handle entering a time period of greater than 24
hours. This doesn't seem to be catered for in KDateTimeEdit currently.

The api docs for the assign...() methods should state what the distinction
is between them and the equivalent set...() methods. There's nothing to
indicate whether they do the same job or what.

Clarification is needed on what the setSelectTimeInterval() and
selectTimeInterval() methods are for. Do they set a time interval which
all returned times are a multiple of? If so, what happens at the end of
the day if someone sets 7 minutes, say, and they keep pressing the up
arrow?

selectTimeInterval() is badly named since its name suggests that it's a
setter method.

i18n("Floating") needs context supplied to the translators.

All new strings should really use semantic markup, e.g.
i18nc("@item:inlistbox Floating time zone", "Floating")

Cheers,
-- 
David Jarvie.
KDE developer.
KAlarm author - http://www.astrojar.org.uk/kalarm

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