lfos / calcurse

A text-based calendar and scheduling application
https://calcurse.org
BSD 2-Clause "Simplified" License
990 stars 94 forks source link

Daily repetition of overnight events only shows evening segment in appointment view #315

Open alisterpino opened 4 years ago

alisterpino commented 4 years ago

Calcurse v4.6.0

When you create an overnight event two segments are created: one for the evening and one for the morning. When an overnight event is repeated daily with a frequency of 1 only the evening segment appears on the Appointments.

  1. Create an overnight event
  2. Make the event repeating: type daily, frequency 1 Observed: one appointment for the event is shown per day, just the start time -> ..:.. Expected: two appointments are shown per day start time -> ..:.. and ..:.. -> end time (next day)

An easy workaround exists:

  1. Create the overnight event
  2. Make the event repeating: type daily, frequency 2
  3. Copy-paste this event to the next day Observed: two appointments are shown per day start time -> ..:.. and ..:.. -> end time (next day)
lhca commented 4 years ago

Your observation is correct.

It is an example of a "recurrent (repeating) appointment with overlap", i.e. it has end time on or after the day of the next occurrence (repetition). The implication is that the appointment has two occurrences on the same day. Calcurse cannot handle appointments with more than one occurrence per day. Fixing it is difficult. Maybe it should be disallowed (detected when it is turned into a repeating appointment)?

lhca commented 4 years ago

I have had a closer look. The way calcurse finds the occurrences of a repeating appointment makes it impossible to report/display more than one occurrence per day.

Such overlapping occurrences can be detected and rejected when loading, importing or editing. Rejection on load would mean that calcurse will not start. That would break backward compatibility if such an (incorrectly displayed) appointment have been inserted.

Any thoughts, Lukas?

lfos commented 4 years ago

Thanks for the summary, Lars. Unfortunately, it's been a while since I have worked on the code for occurrence detection. Do you have a rough estimate on how much work it would be to extend it to detect two occurrences on the same day? Is there a (not too hacky) workaround, given that we're looking at at most two intervals per day?

lhca commented 4 years ago

As already said, I think it is impossible with the current implementation. It touches on the basic design concept of the "day vector".

lfos commented 4 years ago

Yes, hence my question how much work it would be to change the implementation such that it's possible to detect two occurrences. Is there a chance it's not too much work with a clever idea?

lhca commented 4 years ago

I'm not sure; the idea hasn't occurred yet.

In any case, it may be necessary to put a limit on the length of appointments. The example of an "overnight" appointment is only the tip of the iceberg. Among the test files there is an example (in test/data/apts-recur) of a daily recurring appointment with a six-day duration. This appointment will have one occurrence on the start day, two occurrences on the day after, three on the next day, etc.

Does a daily repeating appointment with a duration longer than a day serve any purpose? Likewise for the other base periods (week, month, year). On the other hand, may an appointment that repeats every other day, last for two days, but not longer?

lfos commented 4 years ago

I think it's reasonable to disallow events that overlap with themselves (i.e. the repetition interval is shorter than the length), even though that may be hard to detect (and may not even always be well-defined for some repetition types such as monthly)? If possible, I think we could not have more than two segments of an event on any day.

I can imagine useful use cases (e.g. a week-long event that happens every month/year) and I definitely had such events in my personal calendar before. The alternative is to emulate the desired behavior with a long of chained single day events but that seem much more like a workaround.

lhca commented 4 years ago

I have had a closer look. The way calcurse finds the occurrences of a repeating appointment makes it impossible to report/display more than one occurrence per day.

There is an unexplained problem. If the appointment has an until day, the last occurrence should have the PM segment on the until day and the AM segment on the day after. However the AM segment is missing, even though it is the only occurrence on that day.

The reason is a bug in the calculation of the most recent occurrence on or before the day in question. This is easily fixed, though.

lhca commented 4 years ago

I think it's reasonable to disallow events that overlap with themselves (i.e. the repetition interval is shorter than the length), even though that may be hard to detect (and may not even always be well-defined for some repetition types such as monthly)? If possible, I think we could not have more than two segments of an event on any day.

If the duration of repeating appointments with base period DAILY or WEEKLY is limited as suggested, it is easily detected: duration <= frequency * base period. That would indeed imply that an appointment can have at most two occurrences on any day.

I can imagine useful use cases (e.g. a week-long event that happens every month/year) and I definitely had such events in my personal calendar before.

That was and is no problem. If limits are necessary for MONTHLY and YEARLY they could be set to 28 and 365 days respectively.

lhca commented 4 years ago

Yes, hence my question how much work it would be to change the implementation such that it's possible to detect two occurrences. Is there a chance it's not too much work with a clever idea?

It will require quite a lot of work to correct this very specific problem. The only way I can think of is the following. Whenever an occurrence of a repeating appointment is added to the list ("day items vector") to be displayed, an overlap check must be performed. When needed, the previous occurrence must be found and added to the list as well.

lfos commented 4 years ago

Thanks a lot for looking into this, Lars!

Your proposal to restrict to length of items with monthly recurrence to 28 days and with yearly recurrence to 365 days makes sense. I think adding those restrictions is a good idea, independent from the resolution of the "two segments" problem.

For the segments problem, once we found an occurrence, can we easily compute the end time of the previous occurrence and check whether that's on the same day?

lhca commented 4 years ago

@lfos My original proposal was to disallow recurring appointments with overlap. Did you ever consider that?

... proposal to restrict to length of items with monthly recurrence to 28 days and with yearly recurrence to 365 days makes sense. I think adding those restrictions is a good idea, independent from the resolution of the "two segments" problem.

Limiting the duration of recurring items as discussed is not difficult(?), but the implementation is full of boring details. Items should be rejected on load, which may break backwards compatibility, and on import. A check must be performed when an item is created or edited (start time, end time, type and frequency). Tests must be added.

For the segments problem, once we found an occurrence, can we easily compute the end time of the previous occurrence and check whether that's on the same day?

Finding the previous occurrence is the (interesting) problem.

In conclusion. I am looking into it and at some point a solution may be ready for release, but not the next release.

lfos commented 4 years ago

I did consider your original suggestion but I do not think we should disallow something that is perfectly reasonable and has some practical use cases, only because our current implementation is broken and a fix is nontrivial. Doing that as a temporary workaround while working on a proper fix is fine but we shouldn't think of it as a solution.

We can slowly phase out support for self-overlapping events. In a first step, a check is added to the UI but already existing events are still loaded. A deprecation warning is added to the release notes. Then, in a later release, we start rejecting those items when loading and we add tests to make sure all cases are covered.

Out of curiosity, regarding finding the previous occurrence: Is that conceptually very different from finding the next occurrence, other than a few roles being swapped (e.g. start date and until date)? I wouldn't think so but may be missing something.