-
Notifications
You must be signed in to change notification settings - Fork 97
caldav: add support for more props on MKCOL and PROPFIND#150
caldav: add support for more props on MKCOL and PROPFIND#150DeepDiver1975 wants to merge 1 commit intoemersion:masterfrom
Conversation
Todo:
- add support for these props on PROPFIND
- build the same for CardDAV MKCOL
|
@emersion please review |
36d2981 to
c2ad716
Compare
|
@emersion how can we get this moving? THX |
caldav/server_test.go
Outdated
| "encoding/xml" | ||
| "fmt" | ||
| "github.com/emersion/go-webdav/internal" | ||
| "github.com/stretchr/testify/assert" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to not add the testify dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testify allows to write a bit more compact test code.
Which improves readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a whole new library to learn. I prefer to write tests like the Go standard library does. Also, compact readable (quite the contrary in general).
| calendarName = xml.Name{namespace, "calendar"} | ||
| calendarDataName = xml.Name{namespace, "calendar-data"} | ||
| calendarColorName = xml.Name{ | ||
| Space: "http://apple.com/ns/ical/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The standard way to set the color of a calendar is: https://datatracker.ietf.org/doc/html/rfc7986#section-5.9
This is a non-standard, Apple-specfic property. I'm not sure I want to support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never saw this one in real life. But happy to add this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just adding my 2 cents: I have indeed also observed the proliferation of this pseudo-standard (apple.com/ns/ical) into many open-source clients (e.g. DavX5 sets it on MKCOL). On the other hand, from my experience, after initial calendar creation, this is treated as a pure client-side property by pretty much all clients (i.e. it never gets updated again), so not sure how necessary it really is...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so not sure how necessary it really is...
agreed - this is for sure a pretty low feature.
Never the less if the color is stored server side it will be "synced" once opening the calendar with another client which has some benefits of recognizing calendars.
e.g. on the owncloud 10 codebase (php+sabredav) colors are the same on all clients (owncloud calendar app in web, DAVx and iOS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, actually it doesn't seem like there is any standard way to fetch a VCALENDAR for the calendar itself in CalDAV. So RFC 7986 is only useful to set the calendar color when exporting a whole calendar as .ics.
Oh well, let's just do like everybody else then...
caldav/elements.go
Outdated
| DisplayName string `xml:"set>prop>displayname"` | ||
| Description string `xml:"set>prop>calendar-description"` | ||
| CalendarColor string `xml:"set>prop>calendar-color"` | ||
| CalemdarTimeZone string `xml:"set>prop>calendar-timezone"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "Calendar"
|
Some more general notes:
|
89545b3 to
46d5e86
Compare
46d5e86 to
96422a0
Compare
96422a0 to
ab37619
Compare
|
@emersion @bitfehler please review again - did not yet change the color prop name ... as explained above this is the property I see in real life .... |
| ResourceType internal.ResourceType `xml:"set>prop>resourcetype"` | ||
| DisplayName string `xml:"set>prop>displayname"` | ||
| Description string `xml:"set>prop>calendar-description"` | ||
| CalendarColor string `xml:"set>prop>calendar-color"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably specify the http://apple.com/ns/ical/ namespace for this prop?
| Color string | ||
| MaxResourceSize int64 | ||
| SupportedComponentSet []string | ||
| Timezone string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be a parsed *ical.Calendar?
We should probably check that the parsed value contains exactly one VTIMEZONE component?
|
I have a huge need for this feature in my project. How could I contribute to continue development of this? |
|
Maybe open a new PR? Ideally changes would be logically split to ease review. |
Not talking about the git aspect. Just wondering what is still missing or what's blocking this from being merged. Once I know which direction to go in, then I can branch off and create a PR later. |
|
There are unresolved comments above. |
|
my 2 cents: one can split into:
|
|
Here is the output of interaction between iOS calendar app and MWE. One can see which properties iOS asks for. 2025/01/11 18:27:55 Starting CalDAV server on :8181 |