[Kde-pim] Replacement for KDateTime

[John Layt]

On 9 August 2015 at 16:25, Sergio Martins <iamsergio at gmail.com> wrote:

>
> I think QDateTime is fine, it has a date and a time part just like
> KDateTime.
> We just need to decorate it a bit to get the semantics we need.
>
> Like:
>
> namespace KCalCore {
> class DateTime : public QDateTime { // Or composition instead of
> inheritance
>     bool isValid() const { return date.isValid(); }
>     bool isDateOnly() const { return date.isValid() && !time.isValid(); }
> };
> }
>
> This is imho the simplest solution, isValid() is called *all* over the
> place
> in kdepim, any another approach will be a lot of work.
>

It's not the isValid() part that concerns, you'll have to have a valid
QTime of 00:00:00 for most of QDT to even work now, it's the dateOnly()
part and how to represent that as you can't use an invalid QTime to do it.
I really don't think it is needed and not that much more work to remove the
need for it, but I am concerned about possible downstream implications
which leaves me in two minds over it.

The quick pragmatic solution *is* to define a KCalCore::CalDateTime using
composition that behaves almost the same as KDateTime currently
(inheritance looks like trouble to me). It could remove a lot of subtle
bugs that could creep in unnoticed, and you wouldn't need KLocale,
KTimeZone et al as public api or even privately. Given the time frame we
have this is may be the only workable option (what is the planned freeze
date?).

OTOH, it is continuing a bad practice we should try get rid of given the
opportunity of such a major api version break, and still means a
non-standard barrier to others using KCalCore (albeit a far smaller one
now). Well, all I can suggest is at least to replace calls to isDateOnly()
with event->isAllDay() where-ever possible (which is most places) and make
setting allDay explicit and required rather than it being implicit in the
CalDateTIme passed in. Then we can see if we really need CalDateTime or
not. I'm really not convinced it is that much more work (see my earlier
mail that only went to kde-core-devel about the 16 places dateOnly is used,
most are only safety checks because KDT could be dateOnly with an invalid
QTime, not because it adds any actual functionality).

If we do implement it I'd suggest something like:

class CalDateTime {
    CalDateTime(QDateTime dateTime, bool isDateOnly = false);
    CalDateTime(QDate date, /*some time spec stuff */);

    QDate date() { return m_dateTime.date(); }
    QTime time() { return m_dateTime.time(); }

    bool isValid() { return m_dateTime.isValid(); }
    bool isDateOnly { return m_isDateOnly; }

    QDateTime m_dateTime;
    bool m_isDateOnly;
}

More convenience getters can be added as you find you need them, I suspect
most QDT methods are not actually needed as it's mostly just a container
for QDate and QTime, the real work is done in the recurrence calculator?
Note I've not defined a way to change the QDT value, immutable I think is a
good thing. I've also not included a dateTime() method for now as I'm not
convinced access is needed or a good thing (although may be needed for
formatting with QLocale?).

Note that the m_dateTime must *always* have a valid QTime (so 00:00:00 for
date only) if you want most functions on the QDT to work, it has validity
checks everywhere now instead of just blindly returning rubbish results.

The other point to note is that the addition of the needed QTimeZone
support won't happen until 5.7 now, Thiago didn't have time to look at the
code before the 5.6 feature freeze, so general availability of the feature
is now a year away, two by the time we bump dependency versions, so can't
be relied upon. We'll need a different solution, probably copying my tz
code into the recurrence calculator instead and generating one-off
CalDateTime instances as required with OffsetFromUTC. Later when QTZ gets
support we can clean things up. It does mean not using a CalDateTime
instance to do date maths with, you will always need to use the recurrence
editor to get the right one but I suspect that is the way it currently is
anyway? Or perhaps embed the code in CalDateTime instead? I need to think
about that.

I think KAlarm depends on KCalCore already? So by building this into
KCalCore that would also take care of KAlarm's needs?

Anyway, if you want any help from me in doing any of this, grab me now
while I have 2 weeks holiday and some time to spare. Do you have a branch
in the repo that this is happening in?

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/