`OrderedDistinct()` in `Calendar.GetOccurrences()` not working as expected

[minichma]

Follow up of the discussion in https://github.com/ical-org/ical.net/pull/864/files#r2359270375

Calendar.GetOccurrences() is expected not to return duplicate occurrences, so it uses OrderedDistinct() for removing duplicates. The operator is used instead of just .Distinct() for performance reasons because it leverages the assumed fact that the sequence of occurrences is ordered. Due to the order it assumes that duplicate values are always returned by the enumerable adjacent to each other but this is not necessarily the case. Instances of Occurrence are considered equal if both, the Period and the Source are equal. However, the sequence of occurrences is only ordered by Occurrence.Period.StartTime but not by the other properties that are used for determining equality.

In particular

• Period.Equals compares all, a Period's Start, End and Duration but CompareTo only compares the StartTime property
• Occurrence.Equals compares the Period and Source property using Object.Equals while CompareTo only compares the Period and therefore only the Period.StartTime property.

(also compare this discussion https://github.com/orgs/ical-org/discussions/606#discussioncomment-11867080)

IMHO it is not clear what the best is to solve this issue. I see two main approaches:

1. Fix Equals vs CompareTo so they are consistent (i.e. equality of Equals matches that of CompareTo() == 0
2. Let OrderedDistinct() deal with non-perfect ordering.

While having a consistent implementation of Equals and CompareTo, this might not always be possible (as discussed here), so probably 1) will not be sufficient and 2) will be needed.

Ical.Net 5.1.0

[axunonb]

While having a consistent implementation of Equals and CompareTo, this might not always be possible

Agree. So you would modify the method CollectionHelper.OrderedDistinct<T>(this IEnumerable<T> items, IEqualityComparer<T> comparer) and change the way how the previous and current item are compared?

[minichma]

Basically yes. I.e. I would specify two comparers, one for defining the window to be observed and one for determining whether items are actually equal, like so:

    /// <summary>
    /// Returns a sequence containing the items of the ordered input sequence, with duplicates removed
    /// if they reside in the same observation window.
    /// </summary>
    /// <remarks>
    /// The operator iterates over the input sequence and maintains a set of items seen so far within the current
    /// observation window. The observation window is defined by the <paramref name="windowComparer"/>. Within
    /// the observation window, duplicates are removed according to the <paramref name="equalityComparer"/>.
    /// 
    /// The input sequence is expected to be ordered according to <paramref name="windowComparer"/>, such
    /// that equal items are adjacent to each other. Within the observation window, no specific ordering
    /// is required.
    ///
    /// The method operates in a streaming manner, meaning it only enumerates the input sequence while the
    /// output sequence is enumerated and can therefore handle indefinite sequences.
    /// </remarks>
    public static IEnumerable<T> OrderedDistinct<T>(this IEnumerable<T> items, IEqualityComparer<T> windowComparer, IEqualityComparer<T> equalityComparer)
    {
        var prev = default(T);
        var window = new HashSet<T>(equalityComparer);

        foreach (var item in items)
        {
            if (window.Any() && !windowComparer.Equals(prev, item))
                window.Clear();

            if (window.Add(item))
                yield return item;

            prev = item;
        }
    }

In case of Occurrences this would be used in a way that the windowComparer would correspond to the type's CompareTo, which defines the order, while the equalityComparer would comare according to Equals. But it should be considered this the outcome again isn't ordered perfectly, so downstream operators might have to implement similar handling.