Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding iCloud Support (WIP) #60

Closed
wants to merge 4 commits into from
Closed

Adding iCloud Support (WIP) #60

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 1, 2017

This is my first attempt to make calcurse-webdav play with the Apple iCloud calendar. It's supposed to solve issue #30.

...but not yet. The download works, sync works, upload does not yet work.

I'll post this pull request already. It would be interesting to know if the download still works with other caldav servers. Does it break google?

Issues:

  • iCloud sends the base URL as resource: Solved by ignoring resources that do not end in "/"
  • iCloud does not allow REPORT without an VEVENT filter: Solved by refactoring the download to use PROPFIND instead of REPORT.
  • iCloud sends multiple events in one .ical resource. Unfortunately the .ical resource is the key in the database: Solved by concatenating the OBJHASHES spearated by ";" and saving them on one .ical resource. Expanding the list to figure out what needs to be synced.
  • iCloud expects a special UID form in a PUT request: To be done
  • iCloud can not handle DTSTART without DTEND: To be done

@watersalesman
Copy link
Contributor

This is some good work! I'm looking forward to seeing the increased compatibility.

I'm not sure if it is specific to syncing with Google Calendar or not, but OAuth2 syncing fails with the new changes. I receive the following error: https://gist.github.com/watersalesman/bb6bb6e3557977407904c9937ad172aa

This happens with normal syncing, as well as any of the --init types.

@ghost
Copy link
Author

ghost commented Nov 2, 2017

@watersalesman Thanks for testing. Looks like I need to revive my google account and test this with iCloud side by side. Interesting, that I read apple, calendar server, mobileme etc in your error trace. This is not coming from calcurse-webdav... I doubt it's coming from google. Is your google calendar somehow connected to icloud or did you migrate from icloud to google in the past? I'm just curious...

@watersalesman
Copy link
Contributor

@xkpd3 Nope, this account is in no way shape or form connected to any Apple products/accounts. A quick search shows other people getting things such as mobileme within their responses from Google Calendar, so it's likely from there.

@lfos
Copy link
Owner

lfos commented Nov 2, 2017

Thanks so much for working on this. I know this is a non-trivial task and requires quite some patience and debugging skills, so your work is much appreciated!

Some remarks:

  • While putting all into a single commit is fine for a draft/RFC, we should really split each of the points addressed into a single commit with a much more detailed explanation in the commit message when merging this. We can either do the splitting later or you can do it while you are working on it -- whatever you prefer.
  • I would like to avoid switching from REPORT to PROPFIND unless it is really necessary (mainly because of potential breakages with other servers that were supported before). Is there really no way to tune the REPORT query such that iCloud CalDAV server likes it? What is the exact error message you get when filtering for a combination of VEVENT and VTODO, alongside with test="anyof"?
  • The fix for "multiple events per file" is a very good one. It definitely needs to go in a separate commit (even more than the others) because I think we also saw this issue with other servers but could not pinpoint the cause -- thanks for looking into this and coming up with a fix. With regard to the implementation, I personally prefer , to ; as a separator but that is probably just a matter of taste.
  • The "special UID" thing is something to investigate in more details before "fixing". Again, I would really try to avoid any changes here unless absolutely necessary because of potential regressions.
  • As mentioned in the other bug report, we need to discuss what to do with DTEND when exporting all day events and punctual appointments. Issue All day events are exported as zero duration events #46 is related.

@ghost
Copy link
Author

ghost commented Nov 3, 2017

@lfos: I agree with all points. I didn't start with the idea of implementing iCloud support. I started with the intention to make it work somehow for me. Then it turned out to be a more complicated task.

That aside, I can spend some more time on this and fix things up and create proper commits to allow a productive collaboration (and allow you to merge parts of it).

WebDav / CalDav are new protocols for me (from a developer perspective), so there is a good cance that iCloud can do more and I did just not find the solution (yet). CalDav is based on WebDav and my conclusion was, that PROPFIND should work everywhere. But I see your point and maybe we should be open to allow multiple calls. This would be required for multiple calendar support anyway.

  • test="anyof" was discussed on a mailing list but never part of the protocol. No it doesn't make a difference.
  • Regarding the special UID - more investigation is indeed needed.
  • I see the DTEND issue as a separate topic as it would be a fix in calcurse itself and not calcurse-caldav (correct me if I'm wrong here).

@lfos
Copy link
Owner

lfos commented Nov 3, 2017

You are totally right about anyof. It is only in "Collected Extensions to CalDAV". Not sure how I missed that. Let us do two separate calls then. And yes, the DTEND issue needs to be fixed in the export routine of calcurse itself.

@ghost
Copy link
Author

ghost commented Nov 5, 2017

  • Split commit into smaller chunks: ✓
  • Use multiple REPORT calls instead if PROPFIND: ✓

I can see a weird sync bahavior right now. Download works fine. When a local event gets added it gets synced. The next sync will remove a lot of local entries even they have not been removed remotely.

I assume that the syncdb expand must be done in multiple places. I tried to move it to the get_syncdb function, but this won't work as it breaks href as key. So the expansion must be done when accessing the data aftet it has been retrieved via href key.

On the positive side, I have successfully synced a few events to iCloud - so the UID issue might not exist - or was caused by something else.

Stefan Hagen added 3 commits November 5, 2017 20:09
When querying the server for VTODO and VEVENT, iCloud returns the base
URL in the list of URLs well. This leads to an error as no iCal
information is provided. This base URL can safely be skipped.

Signed-off-by: Stefan Hagen <github@textmail.me>
If the server returns multiple events within an ical file, multiple
objhashes have to be saved. Because the URL is the key field, multiple
database entries are not an option.

With this commit, a semicolon separated list of hashes is created in the
database. When syncing, the list is temporarily expanded in order to
find out what needs to be synced.

The change is backwards compatible.

Signed-off-by: Stefan Hagen <github@textmail.me>
In order to support iCloud, we need two queries - one for EVENTs and one
for TODOs. iCloud does not allow to query without a event or todo
filter. These filters can not be combined with a logical OR.

One solution would be to use webdavs `PROPFIND` method.

In order to minimize the risk of breaking existing functionality, it's
better to stick with the webcals `REPORT` method and duplicate the
query.

Signed-off-by: Stefan Hagen <github@textmail.me>
@ghost ghost changed the title Adding iCloud Support Adding iCloud Support (WIP) Nov 6, 2017
@lfos
Copy link
Owner

lfos commented Nov 6, 2017

Well, currently, the internal data structure used to represent the synchronization database is a dictionary which maps each URL to an ETag and to a calcurse object hash. To implement the "multiple events per URL" feature properly, I guess we should change this to be a dictionary of 2-tuples, where the first entry in the tuple is the ETag and the second entry is a list of calcurse object hashes.

Then, the expansion of lists needs only be done when reading (writing) the synchronization database from (to) the disk. And, of course, one must check all uses of the syncdb variable in the script and make adjustments where required.

@lfos
Copy link
Owner

lfos commented Nov 6, 2017

Regarding the two REPORT calls, I would like to reduce code duplication as much as possible. These two queries are almost the same, so it makes sense to have a single hard-coded template and insert the requested event type using format().

iCloud requries DTEND to be present when adding an all day event.
This commits duplicates the DTSTART as DTEND if no DTEND is specified.

Signed-off-by: Stefan Hagen <github@textmail.me>
@ghost
Copy link
Author

ghost commented Nov 6, 2017

Reg. your first comment:
Changing the db structure this way, would this allow to acces data by hash as well as by href? If yes, this would be a good move.

Reg. your second comment:
I'll put it on the todo list :)

@lfos
Copy link
Owner

lfos commented Nov 6, 2017

Depends on what you mean by "allow to acces data by hash as well as by href". Of course, the data structure allows accesses in both directions because it contains all information from the synchronization database. Lookups by href are more efficient, though, while each access by object hash requires O(n) time (linear scan). I think this is not noticable, unless you have thousands of appointments, though.

@dasJ
Copy link

dasJ commented Nov 7, 2017

So, is it safe to test this?

@lfos
Copy link
Owner

lfos commented Nov 8, 2017

I don't think so. This is pretty incomplete and, as Stefan mentioned, currently broken...

@dasJ
Copy link

dasJ commented Nov 8, 2017

Oh, sad it's broken. The incompleteness wouldn't matter, because this seems to fix some bug with Google Calendar and that is what I actually need.

@ghost
Copy link
Author

ghost commented Nov 13, 2017

@dasJ: I'm a bit short with free time right now. I'll continue on the weekends when possible.

The next step is a bigger one, because it involves a database structure change.

Which google calendar bug are you referring to? Maybe we can fix this earlier. Please open a separate issue for it.

@dasJ
Copy link

dasJ commented Nov 13, 2017

@xkpd3 Thank you for your work! There is already #20 which seems to be my problem

@lfos
Copy link
Owner

lfos commented Nov 17, 2017

Maybe we can do smaller steps. The directory href patch can be included as-is, I guess. I renamed issue #20 -- we should fix that next in a separate PR.

@lfos
Copy link
Owner

lfos commented Nov 17, 2017

I added some ideas and open problems to #20 -- it would be great to have more people join that discussion!

@lfos
Copy link
Owner

lfos commented Nov 26, 2017

What's the status of this PR? Do you plan on continuing your work on this?

@ghost
Copy link
Author

ghost commented Nov 27, 2017

I'm planning to continue, I just have trouble finding the time for it.

@lfos
Copy link
Owner

lfos commented May 19, 2018

Any progress? Would you like somebody else to pick this up?

@ghost
Copy link
Author

ghost commented May 19, 2018

If anybody wants to pick this up - please do so. I can only reproduce the issues with my real calendar. And I lost many entries because of it. I was not able to reproduce the issue in my test calendar. This multi-event-entries never showed up.

@lfos
Copy link
Owner

lfos commented May 19, 2018

The problem is that I cannot reproduce the behavior at all... Maybe somebody else can help isolate the issue?

@rslindee
Copy link

Is this stalled because the developer is unable to reproduce? Please let me know if this is the case and I can test on my 100% consistently-failing setup.

@lfos
Copy link
Owner

lfos commented Feb 13, 2019

It is stalled because the original PR submitter stopped working on it. Missing pieces are described in the comments above. Feel free to pick it up.

@mtreca
Copy link
Contributor

mtreca commented Feb 28, 2019

Note that vdirsyncer supports icloud remotes (see #188)

@lfos
Copy link
Owner

lfos commented Oct 23, 2020

Closing due to inactivity. I would be really happy if somebody with iCloud access picks this up and completes the work though!

@lfos lfos closed this Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants