Add way to get end_time for occurrences

[dlee]

Right now, when you specify a duration, there's no simple way to get the end_time for all occurrences. You have to calculate it yourself, and that's error-prone for all-day events (especially across daylight saving changes). Even more error prone is when the user provides end_time instead of duration, and the user wants to calculate the end times for other occurrences.

I propose Schedule#end_time_for_occurrence(occurrence) that will automatically return the end_time for a given occurrence, taking into account duration or end_time.

[dlee]

I think this might be a good start:

  def end_time_for_occurrence(occurrence)
    if all_day?
      occurrence.to_date + (start_time.to_date - end_time.to_date)
    else
      occurrence + (end_time.to_i - start_time.to_i)
    end
  end

[jfelchner]

@dlee much better issue on this one.

But I think the problem is the same as your previous issue, you're confusing "end_time" with "duration". The end_time (if I'm reading it correctly) is not the end time per occurrence, it's the end time of the schedule, which I do not think is what you want.

[dlee]

@jfelchner end_time is supposed to be the end_time for the first occurrence. With that, ice_cube should be able to deduce the end_times for all other occurrences.

[avit]

end_time is supposed to be the end_time for the first occurrence. With that, ice_cube should be able to deduce the end_times for all other occurrences.

@dlee I don't get it. What should be used to set the bounding limit for the schedule's occurrences if you don't want them to repeat forever? Say I want my weekly, hour-long event to stop after Dec 31? That's what schedule.end_time is for.

Occurrences are just time objects pointing to the start of each occurrence; they don't have an "end time".

I haven't had any problems with the existing: schedule.next_occurrence + schedule.duration and I don't think schedule.end_time_for_occurrence(schedule.next_occurrence) would be better.

That said, what about methods like:

next_occurrence_end
all_occurrence_ends
occurrence_ends
...

These could simply advance the occurrence times by the duration.

Either that, or implement an Occurrence class as a Time decorator so it can have an end_time and duration directly:

workday = schedule.next_occurrence
# => 2012-11-30 09:00:00 PST

workday.end_time
# => 2012-11-30 17:00:00 PST

That's what I would do...

[dlee]

@avit end_time is the end time for the first occurrence, not the "bouding limit for the schedule's occurrences".

[jfelchner]

@avit @dlee You two are both right. The iCal spec specifies that you can specify the end time for an occurrence but you can ALSO specify the end time of a schedule. They are two totally separate things. If you specify the end time of the occurrence it calculates the difference between the start time of the schedule and the end time of the occurrence and determines the duration which is then used to calculate the end time of all other occurrences.

How that's specified when creating and IceCube::Schedule is a little bit murky and should be something that @seejohnrun can weigh in on.

But I definitely feel like there should be an Occurrence object that has the start time, end time and time zone associated with it.

[dlee]

@jfelchner How can both be right? The schedule.end_time property is the end time of the first occurrence, as it maps directly to DTEND, and nothing more. @avit is claiming schedule.end_time is the cut-off for repeating occurrences.

@avit to answer your question:

What should be used to set the bounding limit for the schedule's occurrences if you don't want them to repeat forever?

You have to use the recurrence rules to add either a repetition count or a last occurrence date (rule.count(...) and rule.until(...) respectively) to stop occurrences from repeating forever.

[jfelchner]

@dlee I wasn't claiming that end_time did both things, I was saying that both things existed. It seemed like you two were arguing that if end_time was the end time of the occurrence, that you couldn't also set the end time of the schedule, which is incorrect.

But I didn't know specifically what the method calls were do it. Thanks for explaining.

[avit]

The schedule.end_time property is the end time of the first occurrence, as it maps directly to DTEND, and nothing more. @avit is claiming schedule.end_time is the cut-off for repeating occurrences.

That's because it is, in IceCube, at least. The schedule.end_time property applies to the whole schedule, not its first occurrence: it behaves like a global UNTIL for the whole recurrence set...

Honestly, I use IceCube purely in Ruby, not for outputting to iCal so I'm not too familiar with the details of the iCalendar spec. I think @dlee is right about it though: if you read the second paragraph of RRULE description, it's pretty clear about DTEND and IceCube seems to be using it differently:

• IceCube uses DTEND and DURATION together. The iCalendar spec calls for using either one alone.
• IceCube uses end_time for bounding the schedule, not for the end of the first occurrence (hence, it has a mismatch for the exported DTEND value).

Given I want lunch 12:00 to 13:00 every day in December:

# @dlee's way
s = IceCube::Schedule.new(Time.new(2012, 12, 1, 12, 0, 0))
s.end_time = s.start_time + 3600
s.rrule IceCube::Rule.daily.until(Time.new(2013,1,1))

s.all_occurrences
# => [2012-12-01 12:00:00 -0800]

s.occurring_at? s.start_time
# => true

s.occurring_at? s.start_time + 30
# => false

You can see this is limited to only one occurrence, and it has no duration. However, this works:

# @avit's way
s = IceCube::Schedule.new(Time.new(2012, 12, 1, 12, 0, 0))
s.end_time = Time.new(2013,1,1)
s.duration = 3600
s.rrule IceCube::Rule.daily

s.all_occurrences
# => [2012-12-01 12:00:00 -0800, ...,  2012-12-31 12:00:00 -0800]

s.occurring_at? s.start_time
# => true

s.occurring_at? s.start_time + 30
# => true

The .to_ical conversion for @dlee's version looks more correct, it matches the use of DTEND and UNTIL that an event exported from iCal.app would have, but it's not the correct schedule that my version creates in Ruby.

There is probably a more fundamental bug or misunderstanding of the iCalendar spec here...

[avit]

Ok, I tried wrapping an Occurrence class around Time, see https://github.com/avit/ice_cube/tree/issues/119-occurrence

In order to do time arithmetic transparently (Time.now - occurrence) I monkey-patched Time. Not thrilled about that solution, but comments are welcome.

[onemanstartup]

Great. Voting both hands for @avit solution.

[jfelchner]

@avit I'm totally fine with your implementation in the way you're calculating the end time when duration in set (ref #120), but monkey-patching Time is not an option in my opinion. If there are places in IceCube where arithmetic like this is being done:

some_time - some_occurrence

then, again in my opinion, it should be refactored to

some_occurrence - some_occurrence

or

some_time - some_occurrence.start_time

Also, in Occurrence, if you override <=> and add include Enumerable, you get :cover?, :include?, :each, :first and :last for free.

[seejohnrun]

Agreed - we need to remove the Time patch - preferably by adding a method to IceCube::TimeUtil

[avit]

I wanted to see how transparently "Time-like" I could make it so as not to change the existing interface. Unfortunately core Time's minus method doesn't do coercion so there's no built-in to_time to leverage here. I'm glad to remove it, but we should be clear about needing occurrence.start_time for diffing times. Internally it's easy to change, but it does change the API for users.

However, with ActiveSupport loaded, it should work transparently without yet another monkey patch: their patch of minus_with_coercion has a hook for comparable_time.

@seejohnrun, what method were you thinking of adding to TimeUtil? Are you thinking of a time_diff just for internal use?