make_all_day() issues

[perette]

Support for all-day events is ugly and confusing.

make_all_day treats an event's begin/end times as an inclusive date range, and converts it to half-open. For begin, it's set to the start of its day, but for end it's bumped to the start of the next day, even if it's already at the start of a day. Additionally, if the begin time is subsequently changed, the event unexpectedly reverts to a not-full-day event. This is not documented and has a side effect that calling make_all_day again causes the end to be bumped up by one, as it is re-converted from inclusive to half-open range. However, setting the end time does not effect the all-day status, nor is the value rounded or bumped; you can make an all-day event that's 1 day, 3 hours and 5 minutes.

I'd like to suggest a combination of behavioral and documentation improvements. I'm willing to commit time to make these happen, but want to gather input and confirm I'm sane in my approach:

Definite code changes:

• If an event has a duration instead of an end time, make_all_day should round the duration up to the next 24-hour interval. If it's already an even day, the duration should be unchanged. (Currently, a duration is converted to a fixed end-time, subject to bumping on successive make_all_day.)

Definite documentation changes:

• Describe how how start/end times/dates are transformed by make_all_day.

Possible code changes:

• Instead of having begin's setter implicitly change events to non-all-day event, instead have it round the provided begin time to the start of day when it's an all-day event.
• Modify end's setter to also round, but round up rather than down. Setting to start of day should not increment to next day (to retain compatibility with current behavior), but that should be clearly documented.
• Add a boolean parameter ala make_all_day(ad = True), with default True which would make the event all-day. If the parameter is false, revert to a non-all-day event.

That seems sane to me, as it's changing already-blurry areas for behavior. But it's still changing behavior and risks breaking code using the library. Alternately, there's the documentation route:

• Document that the event loses it's all-day property if the begin time is set.
• Indicate that make_all_day() can be called to restore all-day status, but doing so will bump the end date again. Assuming the change for all-day durations is in place, suggest using duration instead of end date to avoid bumping.

Thoughts?

[C4ptainCrunch]

Hi @perette !

Half a year later, i'm answering to your post, sorry about the delay :/
Thank you for the well constructed and augmented post.

Changing the behaviour looks to me the way to go, after all, we are not yet in 1.0, breaking changes can and will happen :)

[N-Coder]

The issue I found in #174 (comment) would probably be fixed by your PR. As the exact behaviour of the all-day is already quite blurry (see your proposed code changes and my comment on rounding), we maybe should agree on some expected behaviour first and then do the implementation accordingly.

It might be useful to formulate the assumptions on the semantics of make_all_day one would make as test cases first and then do the final implementation test-driven. Following the test-case boilerplate from my PR, the following test-case assumptions should always hold with arbitrary tz, hour and duration and events defined in the following way:

start = arrow.Arrow(2019, 5, 29, tzinfo="%+03d:00" % tz).shift(hours=hour)
precission_hour = Event(begin=start, duration=td(hours=duration))
precission_day = precission_hour.clone()
precission_day.make_all_day()

The now following is my opinion/suggestion on how this should work to follow the principle of least surprise.

Generally, the resulting timespan should be the shortest timespan from midnight to midnight (i.e. day resultion) that completely encompasses the previous event. This means that the new start time must be less than or equal to the initial start time, the end time and duration bigger than or equal to the initial ones. Formally:

self.assertTrue(precission_day.includes(precission_hour))
self.assertLessEqual(precission_day.begin, precission_hour.begin)
self.assertGreaterEqual(precission_day.end, precission_hour.end)
self.assertGreaterEqual(precission_day.duration, precission_hour.duration)
# to ensure minimality
self.assertLess(precission_hour.begin - precission_day.begin, td(hours=24))
self.assertLess(precission_day.end - precission_hour.end, td(hours=24))
self.assertLess(precission_day.duration - precission_hour.duration, td(hours=24 * 2))

There is a conflict: if the start time is very late (e.g. 22:00) and the end time is very early (e.g. 03:00) we need to add more than 24 hours (here 22+(24-3) = 43h) to fullfill the basic assumption of encompassing the initial timespan. But if the just round up the duration to a multiple of 24, this won't be possible, which would probably lead to different behaviour when making a duration-based vs. an end-time based event all-day. Thus, I'd also enforce identical behaviour via duration and via end-time:

precission_hour2 = Event(begin=start, end=start.shift(hours=duration))
precission_day2 = precission_hour2.clone()
precission_day2.make_all_day()

self.assertEqual(str(precission_hour), str(precission_hour2))
self.assertEqual(str(precission_day), str(precission_day2))

Additionally, if we have an event with a duration being a multiple of 24, but starting in the middle of the day, we can shift the start time to the beginning of the day, but we will have to add another 24 hours of duration (which was already divisible by 24) to also encompass the initial end-time. So here, we need to change an already valid duration to keep up with the basic assumption of always encompassing the initial timespan. But as the duration can only grow in this way, I'd say this is less surprising to the developer/user than suddenly decreasing the end-time and thus having an all-day event that doesn't include it's initial event.

Note: if the start- or end-times are already aligned to midnight, I would assume that they can't change:

if precission_hour.begin == precission_hour.begin.floor("day"):
    self.assertEqual(precission_day.begin, precission_hour.begin)
if precission_hour.end == precission_hour.end.floor("day"):
    self.assertEqual(precission_day.end, precission_hour.end)

But due the problem mentioned above, the following will not hold:

if precission_hour.duration % td(days=1) == td(0): # is duration a multiple of 24 hours?
    self.assertEqual(precission_day.duration, precission_hour.duration)

[C4ptainCrunch]

Hi @N-Coder !
Thank you also for your well detailed post 🤗
I've read it twice, clearly defining the wanted behaviour is indeed the way to go.
Your proposed behaviour looks good to me :)

If i can find some time i'll try all those cases on google calendar to see how they handle it and what tradeoff they make to see if ours look the same.

[N-Coder]

It seems that there a quite a few issues here revolving around the topic of all-day events.
So here's a table to keep track of them:

PR	Issue	Description	Author
#174	#173	don't convert timezone of dates	N-Coder
#159	#155	Improve all-day event handling	perette
#94	#92 / #150	Event.end property adds extra day to Event.end	marianobrc / wom-bat
#144 / #152		Fix all-day events lasting multiple days	raspbeguy
	#161	Time zone conversion support	perette

VTIMEZONEs might also influence this, so I'm also linking #129 (esp. with regard to #161).
I don't know anything about how repeating events are handled, do we have to watch out for those, too?

Regarding the assumptions in my post above (and also the ones in #161 (comment)), we should check that the tests cover all the problems in the linked issues/PRs. Once they all pass, we can close all those issues together. 😊

---

Now that we have #222 to tackle all these issues, I'm also linking the timezone/timestamp parsing/serialization related issues #181, #182, and #188 to be closed with the above issues.

[C4ptainCrunch]

Now that ed14cde / #159 is merged, i started adding some test-cases you proposed above in c42afcf and they are succeeding.
Let's move the discussion over there :)

By the way, @perette and @N-Coder would you like a write access to the repo ? It could make collaboration easier :)