-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[WIP] Introduce public calendar api #11461
Conversation
* 'from' : defines the starting point in time for the query | ||
* 'to' : defines the end | ||
* | ||
* @return array of events/tasks which are arrays of key-value-pairs |
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.
events/journals/tasks
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.
When using recurrenceIds one UID might contain multiple VEvent blocks. This might cause problems when using a simple key-value pair structure
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 see - can you please post some sample data? Let's see if we find an nice abstraction - THX
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.
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.
THX - so the true unique id is a combination of UID and RECURRENCE-ID ??
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 unique id used by caldav is the uri. In most cases the uri equals the uid, but that's not always the case. The caldav standard says that all objects with the same uid have to be stored in the same resource. So the example I posted would be stored in one single db row.
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.
okay - we better mimic the behavior in this case - THX
* @param \OCP\Calendar\ICalendar $calendar | ||
* @return void | ||
*/ | ||
function registerCalendar(\OCP\Calendar\ICalendar $calendar); |
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 \OCP\ICalendar, it's also \OCP\IAddressBook, not \OCP\Contacts\IAddressBook
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'm aware of this inconsistency ... let me fix this
Would it be better if in the future apps are able to register generic managers ? If we had something like Hmmmm... maybe something to be thought about separately. |
Well - the basic idea if to nail down the 'protocol' of interapp communication within core. As an example: have a look at the tasks app as of today - it simply reuses classes of the calendar app. This works as long as there are no changes in that specific calendar classes. And this is a very simple scenario as we know all devs and apps involved - but in a larger scope this will not work. We need these php interface based api approach to ensure proper operations between apps. I see no other possibility. |
75fc4e9
to
f5e3dd4
Compare
The inspection completed: 5 new issues, 18 updated code elements |
@georgehrke @PVince81 @MorrisJobke please review |
Is the $pattern parameter for search supposed to be a regular expression? I can imagine similar problems here, e.g. when the maps app wants to search for all events with Location or GEO in the next 30 days. |
interface IManager { | ||
|
||
/** | ||
* @param string $pattern which should match within the $searchProperties |
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.
@georgehrke the pattern would be a simple text to be searched for - no regex or something
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.
Yep, needs better docs what actually should be passed in and an example maybe
Did you intend to search for all contacts who have a birthday defined or for those within a given time frame? Regarding the general design - from my pov the contacts app should register a 'Birthday Calendar' to be consumed by any app being interested. |
I'm fine with that 👍 |
nice usage scenario - this is where we have to enhance the $options - just like the already defined from and to options (which are basically just an idea for now) |
My intention was to search for all contacts with a birthday defined. |
try the empty string 🙊 - nevertheless: contacts should register the birthday calendar as described above |
Didn't work either iirc (I actually didn't try |
% is added within the implementation - see https://github.com/owncloud/contacts/blob/master/lib/addressbookprovider.php#L124 maybe worth filing an issue against the contacts repo - not really an issue of the api itself |
please review - thx @karlitschek @MorrisJobke @georgehrke @Raydiation @PVince81 @schiesbn @butonic |
public function delete($id, $calendarKey) { | ||
$cal = $this->getCalendar($calendarKey); | ||
if (!$cal) { | ||
return null; |
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.
@return says bool if successful
Can we have some examples how it should be used :)? You can spot usability issues pretty quickly if you see a few lines of code. |
* This function is used to create a new task/event if 'id' is not given or not present. | ||
* Otherwise the task/event will be updated by replacing the entire data set. | ||
* | ||
* @param array $properties this array if key-value-pairs defines a task/event |
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 we have a listing of available keys in the phpdoc?
|
||
namespace OCP { | ||
|
||
interface ICalendar { |
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.
Could we add a findAll method to get all objects? Or are you just supposed to use search with an empty pattern?
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 go for an empty pattern on search - I don't want to clutter the interface with a dozen of methods.
moving to OC8.1 as we are beyond feature freeze |
@DeepDiver1975 I guess this is still valid. Do we want to address this during conf? |
moving since I don't know where this will go |
Any app can use caldav to interact with the new caldav backend starting oc9 |
We need a public api in ownCloud core to access available calendars.
The basic design of the api follows the contacts manager approach which is in place for quite some time now.
There are basically 3 pieces involved:
Please review the api design! THX
@karlitschek @MorrisJobke @georgehrke @Raydiation @PVince81 @schiesbn @butonic THX