[Kde-pim] Replacement for KDateTime

[Sergio Martins]

On Monday, August 10, 2015 15:18:10 John Layt wrote:
> On 9 August 2015 at 21:06, Sergio Martins <iamsergio at gmail.com> wrote:
> > On Sunday, August 09, 2015 20:51:53 John Layt wrote:
> > > OK, next bout of insomnia I might try a few experiments.
> 
> Right, made a start, mostly expunged the dateOnly stuff without a
> public CalDateTime class with no ill effects.
> 
> Now I've hit the issue of what to do with KDateTime::Spec. In
> KDateTime the details of the time spec (offset, tz, utc, etc) are kept
> in a separate class KDateTime::Spec which gets passed around in the
> KCalCore api for things like setting the default spec to use in a
> calendar, or shifting all incidences into a different spec. QDateTime
> doesn't have a separate Spec class, it's all just embedded in
> QDateTime itself.
> 
> This creates an issue with the public api of methods like:
> 
>     Calendar(const KDateTime::Spec &timeSpec);
>     void setTimeSpec(const KDateTime::Spec &timeSpec);

Is the timeSpec even used by the calendar itself ? Not sure why it needs such 
a property. This comes from KDE3 times, iirc korg doesn't use it.

Documentation says: "The time specification is used as the default for creating 
or modifying incidences in the Calendar", but that's not entirely true, 
because the calendar doesn't create incidences, it's the caller code that 
does, and the caller code knows which timespec to use (not sure about KAlarm 
though).

>     KDateTime::Spec timeSpec() const;
>     void shiftTimes(const KDateTime::Spec &oldSpec, const
> KDateTime::Spec &newSpec);
> 
> Calls to these methods usually look like this
> 
>     cal.setTimeSpec(KDateTime::Spec(KTimeZone(tz)));
>     cal.setTimeSpec(KDateTime::Spec(KDateTime::OffsetFromUtc, 3600));
>     cal.setTimeSpec(kdt.timeSpec());
>     cal.setTimeSpec(m_spec);
> 
> I see 3 choices for handling this.
> 
> 1) Replace KDateTime::Spec in the api with QDateTime and document that
> the actual datetime value will be ignored. Quick and easy to achieve,
> but possibly confusing. e.g.:
> 
>     Calendar(const QDateTime &timeSpec);
>     void setTimeSpec(const QDateTime &timeSpec);
>     QDateTime timeSpec() const;
>     void shiftTimes(const QDateTime &oldSpec, const QDateTime &newSpec);
> 
>     cal.setTimeSpec(QDateTime(QDate(), QTime(), QTimeZone(tz)));
>     cal.setTimeSpec(QDateTime(QDate(), QTime(), Qt::OffsetFromUtc, 3600));
>     cal.setTimeSpec(qdt);
>     cal.setTimeSpec(m_spec);

1.1) typedef QDateTime DateTimeSpec (to make it less confusing)

> 2) Replace the api with each of the possible specs that can be used.
> Fairly easy to achieve, but bloats the api and makes the calling code
> check for itself which to call (but it often does that anyway when
> creating a new Spec). The shiftTime api is particularly bad in this
> option, e.g.
> 
>     Calendar(const Qt::TimeSpec &timeSpec, offsetFromUtc=0);
>     Calendar(const QTimeZone::Spec &timeZone);
>     void setTimeSpec(const Qt:TimeSpec &timeSpec, offsetFromUtc=0);
>     void setTimeSpec(const QTimeZone::Spec &timeZone);
>     Qt::TimeSpec timeSpec() const;
>     int offsetFromUtc() const;
>     QTimeZone timeZone() const;
>     void shiftTimes(const Qt::TimeSpec &oldTimeSpec, oldOffsetFromUtc,
>                       const Qt::TimeSpec &newTimeSpec, newOffsetFromUtc);
>     void shiftTimes(const Qt::TimeSpec &oldTimeSpec, oldOffsetFromUtc,
>                       const QTimeZone::Spec &newTimeZone);
>     void shiftTimes(const QTimeZone::Spec &oldTimeZone,
>                       const Qt::TimeSpec &newTimeSpec, newOffsetFromUtc);
> 
>     cal.setTimeSpec(QTimeZone(tz));
>     cal.setTimeSpec((Qt::OffsetFromUtc, 3600);
>     if (kdt.timeSpec() == Qt::TimeZone)
>         cal.setTimeSpec(qdt.timeZone());
>     else
>         cal.setTimeSpec(qdt.timeSpec(), qdt.offsetFromUtc());
>     if (kdt.timeSpec() == Qt::TimeZone)
>         cal.setTimeSpec(m_timeZone);
>     else
>         cal.setTimeSpec(m_imeSpec, m_offsetFromUtc);

Not so bad if we decide to remove Calendar::setTimeSpec(), just the 
shiftTimes() remaining.

> 3) Create a new class CalTimeSpec based on KDateTime::Spec but using
> QDateTime stuff instead, e.g.
> 
>     class CalTimeSpec {
>         CalTimeSpec(const QDateTime &timeSpec);
>         CalTimeSpec(const Qt::TimeSpec &timeSpec, offsetFromUtc=0);
>         CalTimeSpec(const QTimeZone::Spec &timeZone);
> 
>         Qt::TimeSpec timeSpec() const;
>         int offsetFromUtc() const;
>         QTimeZone timeZone() const;
>    }
> 
>     cal.setTimeSpec(CalTimeSpec(QTimeZone(tz)));
>     cal.setTimeSpec(CalTimeSpec(Qt::OffsetFromUtc, 3600));
>     cal.setTimeSpec(CalTimeSpec(kdt));
> 
> Any thoughts on preferred method, or some hybrid of some or all of
> them? I suspect CalTimeSpec is the cleanest approach, but it makes
> some calls unnecessarily verbose, so perhaps supplement it with
> convenience methods that take the base spec components or a QDateTime
> and creates CalTimeSpec in the background?

I think I like 2) and 3) more than 1)


> > We are releasing the applications (branch Applications/15.08, already
> > frozen) but the libraries are just a private implementation detail at
> > this time, there's no kf5 kdepim libraries release, no ABI or source
> > compat promisses.
> Ummm, that's an interesting approach. So what version number will the
> libraries carry? I sure hope the communication is clear these are not
> to be used externally!

No idea, I'm not driving this. Laurent or Dan might know.

Regards,
Sérgio Martins
_______________________________________________
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/