Tuesday, 2023-02-21

dcalisteHello pvuorela, I hope you're well. Thanks for the reviews yesterday.08:04
pvuoreladcaliste: hey08:05
pvuorelamkcal seemed good, but didn't have time to fully finish the review.08:06
dcalisteOk, 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 exce08:08
dcalisteptions by themselves.08:08
dcalisteOn 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
dcalisteThat would be based on loadByAttendee() for instance.08:11
pvuorelaso 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
pvuorelawe can also always bring back load methods if seen necessary, perhaps in better shape as they were.08:12
pvuorelain this form on the many i doubt how useful they can be. like the loading of events with coordinate info.08:12
pvuorelaloadAttenceeIncidence() sounds like loading so much that it probably is not much different from just loading everything.08:15
dcalisteI 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
pvuorelaloadContactIncidences(), the contact card case one could imagine but does such UI exist anywhere. i doubt such is going to be implemented.08:17
pvuorelaalright. 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
dcalisteIndeed. I'll reply in Github to your question on loadIncidenceInstance(). But maybe we can discuss it a bit here first:08:19
dcalisteThis 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
dcalisteThe problem now is that:08:22
dcalistecaldav plugin is logging modification by instance identifiers. So the QML plugin only know this information and is thus calling loadIncidenceInstance().08:23
dcalisteWhat I can propose is:08:23
dcalisteWe 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
pvuorelawhat do you mean logging modification by...?08:27
dcalistecaldav 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
dcalisteThen, 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
pvuorelajust checking some code. so by qml you mean some buteo log thing?08:30
dcalisteNo, QML here was for nemo-qml-plugin-calendar, sorry being confusing.08:30
pvuorelabut nemo-calendar is not coupled with the buteo logs?08:30
dcalisteButeo 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
pvuorelaright right.08:32
dcalisteI 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
pvuorelabut ok, guess there some reasoning for keeping that loader method and it's quite small anyways.08:33
dcalisteI'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
dcalisteMaybe it's clearer like that ?08:36
pvuorelai think i got it :)08:36
dcalisteOk, thanks !08:36
dcalisteAbout 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
dcalisteThe 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
dcalisteI 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
dcalisteBecause, I've tested changing the time of the event and this is playing nicely. But what happen if we change the day…08:44
pvuorelaguess importing ics data with such events was broken already in some way08:44
dcalisteIndeed yes, the property was dropped. So nothing appeared. The question is can we support with at a minor cost…08:46
dcalisteLooking 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
dcalisteActually, 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
pvuorelaok good. but how about then the recurrence end info on the original event? :)08:52
dcalisteSo 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
dcalisteIn 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
dcalisteIf the event changes radically, a new one should be created.08:53
pvuorelanot 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 exception08:54
dcalisteI 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
pvuorelathen 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
pvuorelawas thinking if i should not immediately merge that text PR now.08:56
dcalisteLet's get the end recurrence display PR in, and I'll see in the other one, how I can tweak this properly.08:56
dcalisteAh, ineed !08:56
pvuoreladcaliste: for the mkcal, were you planning on doing api migration PRs for related projects?09:09
pvuorelato avoid the deprecation warnings.09:09
dcalisteYes. 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 Ingvix16:57

Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!