TheAlmightyBob / Calendars

Cross-platform calendar API plugin for Xamarin and Windows
MIT License
101 stars 23 forks source link

Exception thrown for iOS 13 users when adding event(s) to a calendar #68

Closed stefan89 closed 5 years ago

stefan89 commented 5 years ago

We are using AppCenter to gather information about crashes for our app and since iOS 13 came out we suddenly see crashes thrown by this library. On other iOS versions we don't have this issue with this library.

Exception: Enumerable.Select[TSource,TResult] (System.Collections.Generic.IEnumerable1[T] source, System.Func2[T,TResult] selector) System.ArgumentNullException: Value cannot be null. Parameter name: source

Stacktrace: Main thread System.Linq Enumerable.Select[TSource,TResult] (System.Collections.Generic.IEnumerable1[T] source, System.Func2[T,TResult] selector) Plugin.Calendars CalendarsImplementation+d__8.MoveNext ()

So it seems it goes wrong in the AddOrUpdateEventAsync method in this class: https://github.com/TheAlmightyBob/Calendars/blob/master/Calendars/Calendars.Plugin.iOSUnified/CalendarsImplementation.cs

Is this a known issue?

I couldn't reproduce the issue myself on an iOS device.

TheAlmightyBob commented 5 years ago

@stefan89 Thanks for reporting. This is not a known issue, although might possibly be related to #66 (just a guess though). I assume that's the entire stack trace?

stefan89 commented 5 years ago

Unfortunately we don't have more information than the stacktrace. We only know for sure it goes wrong on line number 271 OR line number 273 in CalendarsImplementation.cs

TheAlmightyBob commented 5 years ago

Are you adding/updating reminders on events (or updating events that might already have reminders)?

bo3po commented 5 years ago

Yes. This is the issue i was getting. Sorry for the late feedback on this... I never create a calendar. Only adding or removing from existing if this is any help. I get the error local as well if you need more input.

TheAlmightyBob commented 5 years ago

@bo3po If you're seeing this issue locally then please share exactly what you're doing, as neither @stefan89 nor myself have been able to repro this error.

TheAlmightyBob commented 5 years ago

@stefan89 Okay, the one way I've found to repro this is if you are updating an existing event that has alarms, but you set reminders to null on the CalendarEvent object you pass in. That will throw this error from the line 273 you saw (line 271 is already protected via ?.). That needs fixing, but in the short-term if you set reminders to an empty list instead of null that should avoid this.

However I can't imagine how this would be related to iOS 13. I can't help but think that must just be a coincidence...

bo3po commented 5 years ago

this has been working for a long time. i switched to netstandard2.0 - and iOS13. i get this error all the time. but my code is protected by try catch to nobody gets thrown off by the exception. the adding just does not work. it works in the simulator. not on a real device. not sure if android is affected. i've only seen this on iOS. i basically just do what you do in your example code.

TheAlmightyBob commented 5 years ago

@bo3po I trust that adding new events is still working and only updating existing events fails?

I don't have a device to test with right at the moment. Can you try the workaround I posted above?

TheAlmightyBob commented 5 years ago

@stefan89 @bo3po I should warn however that if you do the above, you'll be removing reminders that the user may have wanted. If the scenario above is indeed what you're hitting, then I'd be curious what your flow is. Generally to update an existing event one would call GetEventByIdAsync first, tweak some properties, then pass the modified event object back to AddOrUpdateEventAsync. If the existing event had alarms, then the object returned from GetEventByIdAsync should reflect that and thus the modified event shouldn't have a null value for reminders unless you explicitly set it to null...

bo3po commented 5 years ago

No. Adding dos not work. That is the problem. I have a simple "add an event with startdate tomorrow at 10:00 and duration 1 hour"-method that is running in the OnStart for testing. On the simulator this works though. On a real device running iOS13 it does not. I only add in this case. And I never update in any of my cases. I only add or remove. I use AddOrUpdateEventAsync and reminders is always null in any case. We do not use it. I will try to throw a permission block on this also but I gave the app permission to access the calendar in an other place of the code.

TheAlmightyBob commented 5 years ago

@bo3po Does setting reminders to an empty list instead of null fix it? If not, then I think you're seeing a different issue.

TheAlmightyBob commented 5 years ago

(if it does fix it, then I wonder if new events are now being initialized with default alerts by iOS... might be worth checking the default alert configuration in the Settings app... that could explain why it doesn't repro on simulator, since that setting doesn't exist on simulator...)

bo3po commented 5 years ago

@TheAlmightyBob I will try this. Great input. I will be getting back to you with the results. I'll check the default alert configuration as well.

bo3po commented 5 years ago

@TheAlmightyBob success! it fixed it, by setting the reminders to calendarEvent.Reminders = new List<CalendarEventReminder>(); and not null. regarding the "default alert configuration in the Settings app" - what would I be looking for?

TheAlmightyBob commented 5 years ago

@bo3po Settings -> Calendar -> Default Alert Times Are they set? Or do they all say "None"? If they are set, does changing them to "None" also stop the error even if you set reminders to null?

bo3po commented 5 years ago

Indeed yes they are. And changing them to "None" also stop the error I have. Yes. That seems to be the fix for it :)

Very good, sir.

On Mon, Oct 14, 2019 at 4:40 PM Caleb Clarke notifications@github.com wrote:

@bo3po https://github.com/bo3po Settings -> Calendar -> Default Alert Times Are they set? Or do they all say "None"? If they are set, does changing them to "None" also stop the error even if you set reminders to null?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/TheAlmightyBob/Calendars/issues/68?email_source=notifications&email_token=ACMPY6MQBWKFIRV7YNEFZWDQOSAFTA5CNFSM4I63EW22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBE7UXI#issuecomment-541719133, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACMPY6L32DK6RYWYU5HCTFLQOSAFTANCNFSM4I63EW2Q .

TheAlmightyBob commented 5 years ago

@bo3po Thank you! That is great information.

So while using an empty list will avoid the exception, that still of course overrides the default alert times, which is probably undesirable. Rather than treating null the same as an empty list, I might want to make null mean "leave unchanged" so that it respects the defaults when creating a new event...

bo3po commented 5 years ago

@TheAlmightyBob you are most welcome! Yes. That is great news. This simple fix will make it work again. Explains also why @stefan89 gets the error on some devices but not all. In my case overriding the default alert times is not an issue. Good idea with the new way of treating the null.

Glad to be of assistance.

TheAlmightyBob commented 5 years ago

Should be fixed in latest release.