richardtop / CalendarKit

📅 Calendar for Apple platforms in Swift
https://www.youtube.com/watch?v=cJ63-_z1qg8
MIT License
2.49k stars 334 forks source link

fix app crash issue for larger differences #307

Closed kevalvadoliya closed 3 years ago

kevalvadoliya commented 3 years ago

This PR has the following changes:

kevalvadoliya commented 3 years ago

App getting crash if the difference between start date and end date is greater than 68 years.

richardtop commented 3 years ago

Hi, 68 years is definitely a corner case. Curious, why you didn't go with this solution instead, I think it's simpler:

      let longestEvent = overlappingEvents.sorted { (attr1, attr2) -> Bool in
        var period = attr1.descriptor.datePeriod
        let period1 = period.upperBound.timeIntervalSince(period.lowerBound)

        period = attr2.descriptor.datePeriod
        let period2 = period.upperBound.timeIntervalSince(period.lowerBound)

        return period1 > period2
        }
        .first!

Since the lowerBound will always be smaller than the upperBound, this should work just fine.

kevalvadoliya commented 3 years ago

Thanks for the suggestion @richardtop, I didn't think lowerBound smaller than the upperBound. It's working fine. Let me update the code.

richardtop commented 3 years ago

Also, please take a look at the Apple documentation: https://developer.apple.com/documentation/foundation/nsdate/1410206-timeintervalsince

I think, the proper way to use that method is:

period.upperBound.timeIntervalSince(period.lowerBound)

since we want to get the positive value, how many seconds are from the beginning of the event to its end.

In your updated code, the values would be negative, which would actually sort the events in the inverse order (from the event with smaller duration to the longest event).

kevalvadoliya commented 3 years ago

Very true @richardtop, loweBound should be inside parameter. Thanks for the detailed explanation. I have updated the code. Could you please review it again?

richardtop commented 3 years ago

Now it looks good, I've merged it to the repository. Thanks for the fix!