| dcaliste | Hello pvuorela, I hope you're well. Thanks for the reviews yesterday. | 08:04 |
|---|---|---|
| pvuorela | dcaliste: hey | 08:05 |
| pvuorela | mkcal seemed good, but didn't have time to fully finish the review. | 08:06 |
| dcaliste | Ok, thanks. About the "funky" methods, the problem is that they are there in the public API. One day, someone may use them and wonder why they need to load additional stuff for recurring events. The added test is to ensure that this part of the routine will not disappear. Because with this PR, we sort of guarantee that loaded events are always consistent (parent and exception) whatever the call, so the client code don't have to bother about exce | 08:08 |
| dcaliste | ptions by themselves. | 08:08 |
| dcaliste | On the other hand, I'm not against deprecating these routines… But they may have some faint interest, like displaying in the contact card, the related events of this contact in the history, like we're now displaying the recent communication history. | 08:11 |
| dcaliste | That would be based on loadByAttendee() for instance. | 08:11 |
| pvuorela | so was the loadSeries() and load(uid, recid) in the public api :) i wouldn't expect anyone really using them at this point and good to remove before anyone gets ideas. | 08:11 |
| pvuorela | we can also always bring back load methods if seen necessary, perhaps in better shape as they were. | 08:12 |
| pvuorela | in this form on the many i doubt how useful they can be. like the loading of events with coordinate info. | 08:12 |
| pvuorela | loadAttenceeIncidence() sounds like loading so much that it probably is not much different from just loading everything. | 08:15 |
| dcaliste | I agree. By the way, I'm testing at the moment various new API for load methods that make sense to run at DB level to avoid loading all the DB in memory and sort out the result later. loadByAttendee() is of this kind of loading, but I'm doing for a search method. So only events with a given match are loaded. After I'm happy with an API fitting this need, I may deprecate the old "funky" method and we may readd them one by one when needed. | 08:16 |
| pvuorela | loadContactIncidences(), the contact card case one could imagine but does such UI exist anywhere. i doubt such is going to be implemented. | 08:17 |
| pvuorela | alright. one thing could be also that if we get rid of the unused load methods, maybe it's then easier to focus and enhance the ones actually used. | 08:18 |
| dcaliste | Indeed. I'll reply in Github to your question on loadIncidenceInstance(). But maybe we can discuss it a bit here first: | 08:19 |
| dcaliste | This routine was created because one need to pinpoint exact event or exception. For instance, searching for 'foo', if only the exception of a recurring event contains 'foo', you just want to display this exception. At that time, this routine was thus created. Now I agree that we should load all the event (parent and exception) and it's the client that then only display foo. | 08:22 |
| dcaliste | The problem now is that: | 08:22 |
| dcaliste | caldav plugin is logging modification by instance identifiers. So the QML plugin only know this information and is thus calling loadIncidenceInstance(). | 08:23 |
| dcaliste | What I can propose is: | 08:23 |
| dcaliste | We can actually extract the UID from the instance identifier, so loadIncidenceInstance() should internally call load(uid) (with new implementation). But the API need to propose a load method based on instance identifier so the extraction is done once in mKCal and not in every client that know an instance identifier and then need to display such an event. | 08:24 |
| pvuorela | what do you mean logging modification by...? | 08:27 |
| dcaliste | caldav plugin is writing in the Buteo log all modifications at each sync, saying that event foo was imported from server and modifications done locally on bar were sent to server. In the log, foo and bar events are identified by their instance identifiers. | 08:28 |
| dcaliste | Then, with something like https://github.com/dcaliste/harbour-logbook I can display the list of events that have been touched for the latest syncs. | 08:30 |
| pvuorela | just checking some code. so by qml you mean some buteo log thing? | 08:30 |
| dcaliste | No, QML here was for nemo-qml-plugin-calendar, sorry being confusing. | 08:30 |
| pvuorela | but nemo-calendar is not coupled with the buteo logs? | 08:30 |
| dcaliste | Buteo log provides a list of instance identifiers, and then nemo-qml-plugin-calendar/src/calendareventlistmodel.cpp is providing a list model for these events identified by their instance identifiers. | 08:31 |
| pvuorela | right right. | 08:32 |
| dcaliste | I plan to reuse tghe same model to display search results for instance, the new search loading method returning instance identifiers of events matching the search pattern. | 08:32 |
| pvuorela | but ok, guess there some reasoning for keeping that loader method and it's quite small anyways. | 08:33 |
| dcaliste | I'll try to be clearer when replying on Github: | 08:33 |
| dcaliste | - instance identifiers can be used to identify unique occurrences (like for a sync log, or a search result), | 08:34 |
| dcaliste | - the (instance identifier -> UID) is not a trivial function, | 08:34 |
| dcaliste | - thus mKCal should provide a way to load a full event (parent + exception) knowing a given instance identifier. | 08:35 |
| dcaliste | Maybe it's clearer like that ? | 08:36 |
| pvuorela | i think i got it :) | 08:36 |
| dcaliste | Ok, thanks ! | 08:36 |
| dcaliste | About the this and future modifications in jolla-calendar, I wonder if it is used outside SailfishOS. I think I've seen it in Nextcloud web calendar, but I don't have access to a Nextcloud instance so I can't check. Because now that the flag is saved by mKCal, and taken into consideration by KCalendarCore, importing such an exception will break various things without taking the nemo-qml-plugin-calendar PR and part of the UI PR also. | 08:41 |
| dcaliste | The questions you raised about what do display as an end date and what happen when we delete the this and future exception are quite valid. | 08:42 |
| dcaliste | I need to check something else also : what the RFC says about changing the date of a recurring event in the this and future exception. | 08:43 |
| dcaliste | Because, I've tested changing the time of the event and this is playing nicely. But what happen if we change the day… | 08:44 |
| pvuorela | guess importing ics data with such events was broken already in some way | 08:44 |
| dcaliste | Indeed yes, the property was dropped. So nothing appeared. The question is can we support with at a minor cost… | 08:46 |
| dcaliste | Looking at the RFC, changing the date is not an issue, the time shift between the new date and the recurrenceId must then be applied on all future occurrences. | 08:50 |
| dcaliste | Actually, this is playing nicely on device ! Setting up an event every thursday. At some point, I create a this and future exception and choose Friday. Then all later occurrences are displayed on fridays. | 08:51 |
| pvuorela | ok good. but how about then the recurrence end info on the original event? :) | 08:52 |
| dcaliste | So remains what could happen when deleting the this and future exception… It really depends on the user intent : delete this occurrence or cancel the this and future modifications… | 08:52 |
| dcaliste | In my opinion, the end of the recurring event should still be the original end. The this and future exception is supposed to be used as a minor modifier. | 08:53 |
| dcaliste | If the event changes radically, a new one should be created. | 08:53 |
| pvuorela | not sure. google calendar seemed to be showing such info until the date the time changes. and makes sense as it doesn't repeat with the original time after that if there's a later exception | 08:54 |
| dcaliste | I mean, if we have a regular lunch with colleagues on Thursdays, and at one point, it is moved to Fridays, it is still the lunch with colleague event, whenever it happens. | 08:54 |
| pvuorela | then again with the recurrence end info it says "repeats on thursdays until <date>" which would not be true if it's not thursdays after some date in between. | 08:55 |
| dcaliste | Right… | 08:55 |
| pvuorela | was thinking if i should not immediately merge that text PR now. | 08:56 |
| dcaliste | Let's get the end recurrence display PR in, and I'll see in the other one, how I can tweak this properly. | 08:56 |
| dcaliste | Ah, ineed ! | 08:56 |
| pvuorela | dcaliste: for the mkcal, were you planning on doing api migration PRs for related projects? | 09:09 |
| pvuorela | to avoid the deprecation warnings. | 09:09 |
| dcaliste | Yes. I'll start with nemo-qml-plugin-calendar and then move to Buteo plugins (caldav and google). You indeed may want to wait for inclusion in case I notice something nasty there with the new API behaviour. | 09:10 |
| *** Ischwitch is now known as Ingvix | 16:57 | |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!