Ability to set location, add reminder, and set android calendar to sync.

[TheAlmightyBob]

1. Was the addition of project.lock.json files intentional? If so, why?
2. What is the SyncEvents necessary for? Were you not seeing created calendars/events without it? I researched that attribute when first writing the Android implementation, and the little bit of documentation I found seemed to suggest that it was specific to syncing with an online calendar source. In my own tests I hadn't seen any difference with/without it.

[jamesmontemagno]

Hey there! Blarg, that shouldn't have gotten added. Let me delete and add .ignore file.

So what I have found is that when creating a new calendar it is created fine, but doesn't show up by default because sync is off and you have to turn it on. Tested on device and simulator. Also read documentation saying that it is recommended when creating a calendar. Let me find a link. On Feb 4, 2016 11:01 PM, "Caleb Clarke" notifications@github.com wrote:

1. Was the addition of project.lock.json files intentional? If so, why?
2. What is the SyncEvents necessary for? Were you not seeing created calendars/events without it? I researched that attribute when first writing the Android implementation, and the little bit of documentation I found seemed to suggest that it was specific to syncing with an online calendar source. In my own tests I hadn't seen any difference with/without it.

— Reply to this email directly or view it on GitHub https://github.com/TheAlmightyBob/Calendars/pull/9#issuecomment-180228585 .

[jamesmontemagno]

Alright, now we are looking better :) only +20 lines of change seems more like it :)

Yeah, so reading through the docs and testing on Android if you don't set it to true then it wont get synced to the device, i have verified that I have to manually add the calendar.

SYNC_EVENTS A boolean indicating whether the calendar should be synced and have its events stored on the device. A value of 0 says do not sync this calendar or store its events on the device. A value of 1 says sync events for this calendar and store its events on the device.

http://developer.android.com/guide/topics/providers/calendar-provider.html

[jamesmontemagno]

Just added the ability to set a reminder on a event. Let me know what you think.

[jamesmontemagno]

Also, can you email me james@xamarin.com, just want to make sure you have everything you need to build and distribute and such.

[TheAlmightyBob]

Thanks for updating the ignore file.

We both read the same documentation on SyncEvents, just interpreted it differently. I trust that your interpretation is correct since your tests back it up, although I don't understand why my tests were passing if it's necessary... makes me wonder if the tests are somehow flawed... (I mostly just tested in simulator because I only have my actual phone to test with, and I made the mistake of running an earlier development test on there once and screwing up my calendars..)

Excited to see reminder support! But questions:

1. Although I hadn't gotten far into researching reminders yet, I had expected to implement them as properties of CalendarEvent, not separate classes/methods. That's how they work on iOS/Windows anyway, no? Is there a reason for preferring/needing this separated approach?
2. It looks like only Android supports CalendarReminderMethod. Seems like that should be made clearer to consumers of the API? Either through name or at least comment?
3. ...does iOS let you set any reminder time you want programmatically? Even though the UI only presents a limited set of options? Curious how it appears in the UI if you've set a time that isn't in the list...

[jamesmontemagno]

Yeah, SyncEvents is a super tricky one and they do get added correctly, but in the calendar app (probably different per device and app) it needs to be marked as Sync from what I have read and tested. We can always change too if it isn't.

Let's see if I can go through the Qs 1.) Yeah I thought about this at first too, however I then thought about all the additional work to return that data and each are different. I thought about just adding a "reminder" section to the AddCalendarEvent, but then though why add additional arguments. iOS and Windows have just a single reminder compared to Android that can have multiple reminders set on it, so the current implementation is to update the current reminder on iOS/Windows or add a new one on Android. However, I can change it to being passed in when we create the event not to cause confusion.

2.) This is true, I was figuring just a comment in there would be good. I can add it in.

3.) Reading the docs it seems just fine: https://developer.apple.com/library/ios/documentation/EventKit/Reference/EKAlarmClassRef/#//apple_ref/occ/clm/EKAlarm/alarmWithRelativeOffset: This is the same with Android though the UI only presents simple options, which makes sense from a UX stand point.

[TheAlmightyBob]

I'm definitely glad you didn't go with the additional parameters route. My concern is mainly that I feel like making the alarms a property of CalendarEvent is the better long-term solution, supporting retrieving/editing, and I was thinking about the transition from this version to that one... but I guess it's not a big deal. I do agree that if you're not going to implement retrieval now, having it as a property would be misleading, so the separate method seems reasonable.

However: iOS also supports multiple reminders (EKEvent has an array of Alarms..). And if I'm reading this right, AddReminder will update any existing reminder on Windows, but just keep adding new reminders on Android/iOS? Hm..

Good point re: 3. I quickly tried that on Android and saw that the custom time was just added to the list of options in the Google calendar app. That's good, I was worried there was more of a difference between platforms on this.

[jamesmontemagno]

Fix the comments up.

Ahhh yes, you are correct actually that both iOS and Android have a List of Alarms, so that is pretty great :)

So I was thinking of just updating the Windows documentation for the plugin that there can only be one alarm on an event, so the current would just be updated. Here are the docs: https://msdn.microsoft.com/en-us/library/windows/apps/windows.applicationmodel.appointments.appointment

[jamesmontemagno]

I also have it as Minutes for the reminder.. could change that to seconds too, not sure what you think, or maybe even a TimeSpan?

[TheAlmightyBob]

I think TimeSpan would be great.

Updating the docs is probably fine, as long as the difference is addressed somehow.

[TheAlmightyBob]

Can you speak more to the color exception? When were you seeing an exception? Was this related to having iCloud enabled?

[jamesmontemagno]

Oh sure, well in this case I was connected to a phone and inserting it into the default calendar. In that instance the calendar color is simply null, so that would throw an exception. I just added a null check.

[jamesmontemagno]

I should note, this is when you pass in null as the color to set on the new calendar you are creating. If you pass in any valid color then it is not null and is set correctly.

[TheAlmightyBob]

"inserting it into the default calendar"

Maybe a terminology mixup here. This is the calendar creation logic... I assume you mean creating a calendar using the default calendar's source? Strange that would affect whether a color was returned.

(I have no problem with the change, just trying to understand the scenario that required it)

[jamesmontemagno]

Yeah, how you phrases it is what I meant. I am grabbing the default calendar's source for the source of the new calendar. On Feb 10, 2016 9:59 PM, "Caleb Clarke" notifications@github.com wrote:

"inserting it into the default calendar"

Maybe a terminology mixup here. This is the calendar creation logic... I assume you mean creating a calendar using the default calendar's source? Strange that would affect whether a color was returned.

(I have no problem with the change, just trying to understand the scenario that required it)

— Reply to this email directly or view it on GitHub https://github.com/TheAlmightyBob/Calendars/pull/9#issuecomment-182732105 .

[jamesmontemagno]

Just ran some testsI think now finally iOS is good :)

[TheAlmightyBob]

Thanks!