[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] [PATCH] Recurrences
From:       David Jarvie <lists () astrojar ! org ! uk>
Date:       2005-07-20 23:20:13
Message-ID: 200507202320.13346.lists () astrojar ! org ! uk
[Download RAW message or body]

On Wednesday 20 Jul 2005 22:52, Reinhold Kainhofer wrote:
> Am Donnerstag, 21. Juli 2005 00:11 schrieb David Jarvie:
> > > > The following methods only set or return values for the default
> > > > RRULE, rather than for all recurrence components combined.  This is
> > > > not what one would expect, and not what the comments in the header
> > > > file say. setEndDate()
> > > >     setEndDateTime()
> > > >     duration()
> > > >     durationTo()
> > > >     setDuration()
> > >
> > > With the same reason as all add set* and add* methods: The methods of
> > > the Recurrence class are meant as a shortcut only for the simple case
> > > where only one rrule is involved. If you use more than one RRULE, they
> > > will typically have different durations/enddates, so you can't use
> > > these methods anyway. Also, for multiple rrules, you'll have to go the
> > > direct way via RecurrenceRule anyway, so there's no point in providing
> > > methods for that case.
> > > I added a comment locally in the documentation of the Recurrence class
> > > to make this point clear.
> >
> > For duration() and durationTo(), I definitely think that the Recurrence
> > class should return the number of recurrences taking account of all
> > recurrence components.
>
> The problem is that this is possible only in the the most inefficient way
> you can imagine, namely by calculating all occurrences until the requested
> time.
>
> In the old Recurrence class it was simple, because exception dates weren't
> part of Recurrence. So you could just return the nr of recurrences of the
> rrule, which for those simple cases supported by the old code was not so
> hard. Now with all the correlations between all BY* rule parts, it's not
> even simple to return the nr of occurrences for one rrule.

OK, I am persuaded. But if things can be as inefficient as you say, it would 
be a good idea to put a warning in the relevant header comments about which 
methods can potentially be time consuming.  I know from my own coding of 
recurrences in the past that some operations on recurrences which extend a 
long way into the future can be far from simple, and I think it would be a 
benefit for coders who care about such things to tell them which methods are 
to be avoided when an alternative might exist.

> > The Recurrence class is the class which encapsulates
> > the overall recurrence (incorporating all its components), and it should
> > be capable of returning the overall number of occurrences for all
> > components combined.

[ snip ]

> > For setEndDate(), setEndDateTime() and setDuration() I can perhaps accept
> > what you say, although if duration() and durationTo() return the total
> > duration, setDuration() in particular becomes a bit doubtful.
>
> Well, see the above problem with
> recur->setDuration( recur->durationTo( date ) ) not ending on date. The
> method you want is just a one-way street, so I'm not so sure how to include
> that in the api.

I recognise that there would be an inconsistency. I think you have persuaded 
me to leave it as it is, as long as there are header comments to say things 
only apply to the default RRULE.

> Anyway, I'd like to commit this really huge patch finally. Any objections
> to committing it now? I changed the comments to fix the problems you
> observed, and i changed the RDATE to use the time from the DTSTART.
> We can still add such a method as you want it later on without problems
> (libkcal never was binary compatible anyway).

Yes, I think that it's time to commit it. I'll fix the KAlarm stuff once it's 
committed.

Good work.

Cheers,
David.

-- 
David Jarvie.
KAlarm author and maintainer.
http://www.astrojar.org.uk/linux/kalarm.html
_______________________________________________
kde-pim mailing list
kde-pim@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
kde-pim home page at http://pim.kde.org/

[prev in list] [next in list] [prev in thread] [next in thread]