search

JodliDev / calendar

GNU Affero General Public License v3.0
16 stars 12 forks source link

feat: add preinstalled sources #5

Closed n-peugnet closed 2 years ago

n-peugnet commented 2 years ago

Still a work in progress but this is working quite well already. I am interested in your input.

For instance I didn't use the exact same config as the other forks as it seems the sources configuration is a little bit different from the previous calendar ones. Also if I understood correctly, only the caldav driver is providing this kind of "source" support, thus I decided to remove the driver key.

This is also still missing the example config

Closes #1

JodliDev commented 2 years ago

Thank you very much for your effort :) I have a few concerns with the proposed changes to calendar.php

These changes will most likely crash on other drivers because "new-source" was added for caldav specifically (which is kind of a dirty fix for a structure that is not meant for drivers to have "sources"). So I have two suggestions:

  1. Move these changes to somewhere in the caldav driver, since they are not really usefull for any other driver and would just be confusing otherwise.
  2. Then, instead of using $this->driver->create_calendar(), use $this->driver->create_source()

In general: In order to not make the same mistake as the faster-it fork, (which made it impossible to incorporate any future updates from the roundcube team), I am trying to change as little as possible outside the actual caldav-portion (which enables me to mainly just copy and paste new updates without having to worry about breaking anything). That means I really dont want to touch calendar.php if it can be helped.

n-peugnet commented 2 years ago

I added a commit with your suggestions addressed, I can squash the commits if you want to keep the history clean.

I was also wondering if it would be a good idea to rename the config key from calendar_preinstalled_sources to calendar_preinstalled_caldav_sources as the code is absolutely not meant to support other drivers.