sailfishos / nemo-qml-plugin-calendar

BSD 3-Clause "New" or "Revised" License
0 stars 6 forks source link

Attachment support on calendar #6

Open pvuorela opened 3 years ago

pvuorela commented 3 years ago

Since Jolla gitlab is now read-only, let's move the discussion on the attachment support here.

For reference we've had:

Now going forward I'd suggest doing this in phases instead of trying to get everything at the same time. Though good to also have some forward plans, of course. The way I'd see this is:

  1. Support showing remote url based attachments on received invitations. For the application these should be read-only first. Testing sending Gmail invitation from Android device it automatically puts the attachment into google drive and the invited person receives the event with ATTACH including google drive url. We should ensure the synchronization plugins set this type of attachments to events they store, have the qml plugin here expose the attachments and finally show the list in the calendar app event view page. For starters it might be good enough if we allow the items to be clicked for generic url handling.
  2. Support remotely set attachments when they are not passed via remote urls. Think ServiceInterface::downloadAttachment() is intended to be used to save the file locally. But don't know yet how the code should be able to know when the downloading is a valid operation, unless we make it always valid by implementing also http fetching in the service plugin(s) etc. Potentially the sync plugins could use their custom url schemes too for their internal details.
  3. Some day finally support editing and creating attachments on events. Most unknowns here. Just for throwing ideas, the app / this module could add attachments with local file urls and Attachment::isLocal() set (to note the method description looks funny), and the sync plugin detects those, moves them forward and updates the attachment status. Or could be considered if the ServiceInterface should have a method for creating attachments, e.g. a method that takes a file and returns a uri that can be added to attachment instance. On editing the current deleteAttachment() might have some use.

And to consider further:

Quite a quick braindump here, but let's keep planning. Don't have github usernames for all the related people, but cc: @chriadam @llewelld @dcaliste

dcaliste commented 3 years ago

Support showing remote url based attachments on received invitations.

This first step makes sense and is more or less straight forward. The initial MR from old gitlab can be reworked a bit to expose URI attachments only in nemo-qml-plugin-calendar (this actual MR need to be resubmitted here soon since the old gitlab will be shut down in the next month). At the moment, there is no attachment support at all in the QML plugin.

On this particular topic, I'm wondering if it would be a smart move not to copy the KCalendarCore Attachment class with slight variations in names and members as done historically for Events and friends, but if we could directly expose and use the KCalendarCore::Attachment class itself in the calendareventdata.h header. Modern KCalendarCore classes are all Q_GADGET with properties, signaling for these properties... So why reinvent the wheel ?

As far as CalDAV synchronisation plugin is concerned, the passing of URI attachments are already in place, both directions.

Think ServiceInterface::downloadAttachment() is intended to be used to save the file locally. But don't know yet how the code should be able to know when the downloading is a valid operation, unless we make it always valid by implementing also http fetching in the service plugin(s) etc.

For iCal serialisation (thus Exchange and CalDAV I guess), the inline attachments are already part of the payload of the iCal data. With the recent addition in mKCal, these binary data are stored in the mkcal database and loaded automatically with incidences. So the process to obtain files from these is just to get the corresponding incidence with inline attachment from the database and save the binary data (from KCalendarCore existing API) as a file somewhere.

While saying that, I'm starting to be concerned about performences of such a scheme. Indeed, fetching incidences from the database will always create a QByteArray of data for every inline attachments, even if it's just to list the incidences in the month view. That's quite suboptimal... Maybe one should think about a delayed loading from the database, adding such a new function as ExtendedStorage::loadIncidenceAttachment(Incidence::Ptr) while all other ExtendedStorage::load*() are always lazy and don't load any binary data. What do you think ?

Or could be considered if the ServiceInterface should have a method for creating attachments, e.g. a method that takes a file and returns a uri that can be added to attachment instance.

Yeh for the third point, creating inline attachments can be trivial : read the file and create an Attachment to the Incidence using the existing KCalendarCore API. Then, the iCal serialisation in the sync plugins will take care of the rest. But as you mention, if one wants to use UIR attachments, this may become much more tricky, with the need of a cloud drive somewhere. Thinking in term of Nextcloud for instance, this would involve to upload the chosen file to the Nextcloud storage, get back the uri and attaching it. Indeed, one need a plugin in mkcal implementing this for every supported cloud storage.

Should the service interface plugins be able to expose more properties for the sync services, what operations they support etc.

In my opinion, there should be a clear separation here :

Making all this work requires a lot of thinking and brainstorming. Thank you @pvuorela having started such discussions in the open here.

pvuorela commented 3 years ago

This first step makes sense and is more or less straight forward

Indeed, that's an easy win compared to everything later :)

but if we could directly expose and use the KCalendarCore::Attachment class itself

Seems doable, yes.

While saying that, I'm starting to be concerned about performences of such a scheme. Indeed, fetching incidences from the database will always create a QByteArray of data for every inline attachments, even if it's just to list the incidences in the month view. That's quite suboptimal... Maybe one should think about a delayed loading from the database, adding such a new function as ExtendedStorage::loadIncidenceAttachment(Incidence::Ptr) while all other ExtendedStorage::load*() are always lazy and don't load any binary data. What do you think ?

Very good point. Overall I'm not sure if saving binary data to mkcal database for the attachments is a good idea. Doesn't matter if the data is small but on bigger items it's bad in both database usage and also on the API side. The separate attachment loading feels a bit special casing and might get the setup more complicated.

One alternatively could be that we avoid putting the binary data to the database, maybe even remove the whole support, and instead save the data elsewhere but support that to be saved elsewhere on the sync specific plugins with the downloadAttachment() function. Might be also an option to support that by mkcal, e.g. method to save binary somewhere and return a URI that can be used to fetch it.

bshmarin commented 2 years ago

I propose for discussion my vision of the concept of attachments in a calendar. Some of this has already been implemented. Please add if i missed something.

Attachment storage

  1. The body of the attachment must be stored as a binary file on disk
  2. Attributes of the attachment must be stored in the database
  3. The current set of attachment attributes is missing for some calendar protocols such as Exchange, so we need the ability to add custom attributes for service plugins.

Sync attachments Some calendar services allow to download the attachment body after synchronization, but others do not. It is a good idea to use the service plugin from mkcal for this, but the attachment must have an downloaded/not downloaded attribute (or use the file name of the attachment body - isEmpty()). The calendar service plugin itself needs to know where it stores attachments - in the cloud, like Google, or in itself, like Exchange or CalDAV. If we want to attach a URL from some other cloud to the event, then it will be attach as URL, not a file, and the service plugin should transmit it as a URL, and not try to upload or download.

QML plugin A QML plugin should provide the following functionality:

  1. List of event attachments with name, index and status loaded/not loaded/URL
  2. Download attachment if it is not downloaded
  3. Download progress (optional)
  4. Save attachment to user location
  5. Attach a file from user location to the event
  6. Attach a URL to the event
  7. Remove attachment
bshmarin commented 2 years ago

Attachment storage is the connecting point between the fontend (QML plugin) and the backend (service plugin), so we need to start with it. At first glance, the way of storing the body of attachments in a file does not raise any questions, a similar scheme is used in QMF. A more difficult question is the storage of non-standard attributes that the backend may need to access attachments on the server. A solution is to add custom properties, as in Event.

dcaliste commented 2 years ago

Thank you for sharing your thoughts and views.

I agree that storing inline data outside the DB itself and storing a file location in the DB, like in QMF, is maybe the best option at hand. This is a mKCal job, to change the current storing of binary into a file location + handling the file read and write. This raises a question about Attachment handling though, let think about the following process :

Up to now, no problem. Later on, the user is opening is calendar and see the new meeting event with the attachment and can tap on the attachment to get it opened in the document viewer. To get this, that's what happening :

This is highly inefficient and memory consuming (as long as the event is in the CalendarManager memory, we're using 25MB of RAM for one event). It would be better if on reading, mKCal is creating an attachment which is not a binary one, but an URI one, with the actual file location as URI. But, if we do this, we need to change the sync service, because on upsync, they will send an attachment that is not a binary anymore, but one with a URI that is a nonsense for the rest of the world.

So to summarize, my preference is for :

The latest point can be sorted out by two ways :

dcaliste commented 2 years ago

A more difficult question is the storage of non-standard attributes that the backend may need to access attachments on the server. A solution is to add custom properties, as in Event.

Indeed, https://datatracker.ietf.org/doc/html/rfc5545#section-3.8.1.1 attachments in RFC5545 can have arbitrary properties. This is not supported at the moment in KCalendarCore API. So the first step would be to allow it at KCalendarCore level.

Then, the DB is not setup for them neither. I'm a bit reluctant to create a new table for x-properties of attachments… Attachments have already ther own table, because there may be several attachment for one incidence. So it would mean that to get one event, we would have to loop inside the attachment table for this event, but in addition loop in the attachment property table for each attachment. It looks a bit overkilling for the purpose… But I've no better idea. One can store a string representation of the properties in the attachment table and serialise and deserialise that string. That's ugly also, but maybe less over engineered.

bshmarin commented 2 years ago

mKCal is viewing an event with an attachment, so it's creating an Attachment object att. It is a binary attachment and it calls att.setData() with the data read from the file location stored in the DB.

The CalendarManager (using mKCal) should not load the attachment into its memory, it should copy the attachment file from mKCal's folder to the user's folder (eg Downloads) and provide the user with the final path to view or open. Like in mail: the first click is to download the attachment (not in the case of CalDAV), the second click is to save the attachment, the third click is to open the attachment.

sync services has to be changed to inline any URI attachments that has a file:/// scheme. This is quite boring because we cannot rely on a one liner ICalFormat::toString(event) anymore…

For synchronization services, we can load file contents from mKCal's folder into KCalendarCore::Incidence for each Attachment and then call ICalFormat::toString (event).

Indeed, https://datatracker.ietf.org/doc/html/rfc5545#section-3.8.1.1 attachments in RFC5545 can have arbitrary properties. This is not supported at the moment in KCalendarCore API. So the first step would be to allow it at KCalendarCore level.

Yes - this is already possible to do.

One can store a string representation of the properties in the attachment table and serialise and deserialise that string. That's ugly also, but maybe less over engineered.

I think that serialization of additional parameters is the most economical and universal solution.

pvuorela commented 2 years ago
  1. The body of the attachment must be stored as a binary file on disk

Generally agree. Maybe one exception: if an .ics file is imported into local calendar it could be ok storing the binary part to the sql database. But also fine if we start by not supporting binary attachments on imported files.

The current set of attachment attributes is missing for some calendar protocols such as Exchange, so we need the ability to add custom attributes for service plugins.

This depends on how the attributes would be used. I'm not yet sure if they would be really needed. More on that below.

Some calendar services allow to download the attachment body after synchronization, but others do not. It is a good idea to use the service plugin from mkcal for this, but the attachment must have an downloaded/not downloaded attribute (or use the file name of the attachment body - isEmpty()).

For one there's the isLocal() property on the attachments. It's not documented too well, but maybe something that could indicate if the content is available on the device or if it needs to be downloaded before opening.

mKCal is viewing an event with an attachment, so it's creating an Attachment object att. It is a binary attachment and it calls att.setData() with the data read from the file location stored in the DB. [etc]

I'm thinking if we could keep the mkcal separate from this. What I had in mind was something like

bshmarin commented 2 years ago

This depends on how the attributes would be used. I'm not yet sure if they would be really needed.

If you use a URI to store additional properties of calendar services, you get the following situation (for example, Exchange):

In case of an incoming event:

  1. Sync plugin receives new event with attachment (attachment is not downloaded) uri = eas://attachment?ClientId=12345&Method=1&FileReference=agenda.pptx&EstimatedDataSize=100500 isLocal = false
  2. Service plugin downloads attachment from server uri = file:///home/user/.local/share/system/privileged/Calendar/mkcal/?ClientId=12345&Method=1&FileReference=agenda.pptx&EstimatedDataSize=100500 isLocal = true

In case of an outgoing event:

  1. User adds attachment to event uri = file:///home/user/.local/share/system/privileged/Calendar/mkcal/ isLocal = true
  2. The service plugin sends an event with an attachment to the server uri = file:///home/user/.local/share/system/privileged/Calendar/mkcal/?ClientId=12345&Method=1&FileReference=agenda.pptx&EstimatedDataSize=100500 isLocal = true
pvuorela commented 2 years ago

If you use a URI to store additional properties of calendar services, you get the following situation (for example, Exchange):

Some comments below, but did you mean there would be some problems with the cases? :)

In case of an incoming event:

  1. Service plugin downloads attachment from server uri = file:///home/user/.local/share/system/privileged/Calendar/mkcal/?ClientId=12345&Method=1&FileReference=agenda.pptx&EstimatedDataSize=100500 isLocal = true

Not sure should we do such replacement for the local copy of the file. Could be still the eas:// url too. Only the implementation behind downloadAttachment() and deleteAttachment() need to know the storage details. Or well, as long as the code in this repository doesn't make any assumptions on such paths.

In case of an outgoing event:

1. User adds attachment to event
   uri = file:///home/user/.local/share/system/privileged/Calendar/mkcal/<attachment_id>
   isLocal = true

That exact path would mean a copy is made and the mkcal internal data path is used outside mkcal. Think this could as well be the attached file the user has chosen.

bshmarin commented 2 years ago

We have KСalendarCore::Attachment and methods for storing it in mKCal, we are now limited to this set of properties, and we have nowhere to store the attachment properties that we need for Exchange. Therefore, we have only two ways: to extend the ways of using the existing set of properties of KCalendarCore::Attachment beyond its design, or to try to add additional properties to KCalendarCore::Attachment.

We need to decide - which way will we go?

pvuorela commented 2 years ago

We have KСalendarCore::Attachment and methods for storing it in mKCal, we are now limited to this set of properties, and we have nowhere to store the attachment properties that we need for Exchange.

Could you clarify what are those attachment properties you'd need and how those would be used?

bshmarin commented 2 years ago

To load an attachment in Exchange, we need to know the FileReference and EstimatedDataSize values for each attachment from the event. These values must be stored somewhere after synchronization and then used by the service plugin to download the attachments.

dcaliste commented 2 years ago

Why not save them in the URI ? Your example is eas://attachment?ClientId=12345&Method=1&FileReference=agenda.pptx&EstimatedDataSize=100500. You can save it in the attachment URI, using KCalendarCore::Attachment::setUri(). It will be properly saved and restored by mKCal.

Then in the service plugin, do a QUrlQuery(attachment.uri()) and then use .queryItems() to get a list of (key, values) pairs.