-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Calendar API for apps #6840
Calendar API for apps #6840
Conversation
@tcitworld @nickvergessen @MorrisJobke Any comment on the API design? :) if u give me a 👍 I will go ahead and connect it to the dav app. |
The first version of this API will only allow read-only access, which should already cover >95% of the use cases. If demanded, we can still add write support in a later revision. |
267390e
to
5b04846
Compare
Codecov Report
@@ Coverage Diff @@
## master #6840 +/- ##
============================================
+ Coverage 50.74% 50.79% +0.04%
- Complexity 24423 24474 +51
============================================
Files 1580 1583 +3
Lines 93366 93537 +171
Branches 1359 1359
============================================
+ Hits 47380 47513 +133
- Misses 45986 46024 +38
|
* @return string defining the technical unique key | ||
* @since 13.0.0 | ||
*/ | ||
public function getKey(); |
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.
Maybe change this name to something more obvious?
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 chose this name for consistency reasons with the contacts api: https://github.com/nextcloud/server/blob/master/lib/public/IAddressBook.php#L46
What other name would u suggest?
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.
Well, I guess I would have gone with something telling it's the ID of the calendar but let's be consistent with the contact api.
This is just a public interface for existing stuff? Or will it implement new features? |
I will implement new stuff. The calendar API does not exist yet. Edit: I just want to discuss the API design before going ahead and implementing everything :) |
lib/public/Calendar/ICalendar.php
Outdated
|
||
/** | ||
* In comparison to getKey() this function returns a human readable (maybe translated) name | ||
* @return mixed |
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.
Why mixed?
lib/public/Calendar/ICalendar.php
Outdated
* @return array an array of events/journals/todos which are arrays of key-value-pairs | ||
* @since 13.0.0 | ||
*/ | ||
public function search($pattern, $searchProperties, $options, $limit, $offset); |
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.
Typehint all the arrays
lib/public/Calendar/ICalendar.php
Outdated
* @param array $options - optional parameters: | ||
* ['timerange' => ['start' => new DateTime(...), 'end' => new DateTime(...)]] | ||
* @param integer|null $limit - limit number of search results | ||
* @param integer $offset - offset for paging of search results |
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.
Can that also be null? Because the param above is. Also, you may want to add a default = null
in the function definition then.
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.
Or maybe just 0?
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.
Having $offset = null
would be semantically equivalent to $offset = 0
, which is already covered by integer.
Do we usually allow both null and integer in other APIs?
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.
Ok, according to https://github.com/nextcloud/server/search?utf8=✓&q=integer%7Cnull+%24limit&type= we usually do. So I'll allow both int and null for consistency
lib/public/Calendar/ICalendar.php
Outdated
public function search($pattern, $searchProperties, $options, $limit, $offset); | ||
|
||
/** | ||
* @return mixed |
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.
Mixed is so horrible. What can be in here? ;-) - Explaiiin ;-)
lib/public/Calendar/IManager.php
Outdated
namespace OCP\Calendar; | ||
|
||
/** | ||
* This class provides access to the calendar app. |
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.
Is it really the calendar app or just the nextcloud caldav backend?
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.
CalDAV backend indeed, will change
89be19e
to
d403d30
Compare
@LukasReschke If you are happy, I will go ahead with implementing the Calendar Manager and connecting it to the dav app. |
lib/public/Calendar/ICalendar.php
Outdated
* @return array an array of events/journals/todos which are arrays of key-value-pairs | ||
* @since 13.0.0 | ||
*/ | ||
public function search($pattern, $searchProperties=[], $options=[], $limit, $offset); |
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.
From http://php.net/manual/en/functions.arguments.php#functions.arguments.default
Note that when using default arguments, any defaults should be on the right side of any non-default arguments; otherwise, things will not work as expected.
Also I'd suggest to type-hint the two array parameters with array
.
d403d30
to
f83215b
Compare
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.
Is there a way to add shares information easily here ?
lib/public/Calendar/IManager.php
Outdated
* @return array | ||
* @since 13.0.0 | ||
*/ | ||
public function getCalendars(); |
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.
Array of ICalendar I guess (please precise :) )
* @since 13.0.0 | ||
*/ | ||
public function getDisplayName(); | ||
|
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.
Maybe add getDisplayColor()
or getColor()
too since that will probably be used a lot.
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.
done ✅
f83215b
to
9814821
Compare
fbef205
to
26a474d
Compare
What kind of information do you want to request here? |
I was thinking more of the people with whom the calendar is shared, but public shares are good too. |
a4e1206
to
360e9d9
Compare
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
360e9d9
to
0b0b85b
Compare
Requested changes applied
ff9ad26
to
1684ee5
Compare
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
1684ee5
to
7784672
Compare
please review @tcitworld @MorrisJobke @LukasReschke :) |
implements / fixes #5282