Tuesday, 2021-01-19

*** frinring_ is now known as frinring01:29
*** zbenjamin_ is now known as zbenjamin02:05
dcalisteGood evening chriadam. Would you mind if we start a bit earlier today, I've got a video meeting at 8.30 UTC...07:28
chriadamhi dcaliste07:31
chriadamcertainly, no problem07:31
chriadamI hope you had a good week?07:31
dcalisteYes, it was great, a lot of snow in the nearby mountain, went to country sky during the week-end ! I hope yours was fine also.07:32
dcalisteflypig, merged the jolla calendar MR 266. Thanks to him. And you did for kcalendarcore #2, thanks also.07:33
dcalisteAnd pvuorela merged the color / url one in mkcal. Thanks again.07:33
chriadamvery nice07:33
chriadamI hope that the skiing was enjoyable :-)07:34
chriadammy week was good also, thanks07:34
chriadamyep, got some things merged07:34
chriadamdidn't get jolla-calendar PR#267 in yet, though, unfortunately07:34
chriadamI pinged Pekka and Martin about that one again though, so hopefully in the next couple of days07:34
dcalisteSo in mKCal, two remains about adding a possibility to retrieve all the deleted events. And the one trying to circumvent tne recreation issue.07:35
dcalisteThanks for #267.07:35
chriadamthis is mkcal MR#48+51 I believe07:36
dcalisteAbout the latter, you wonder if it could affect the deletedIncidences() call in sync processes. (yes that's MR #48 and #51).07:37
dcalisteI would be nice to have also the opinion of flypig and pvuorela if they have time to, but I think it won't affect it.07:37
dcalisteIn the google sync adaptor, there are two fetchs : insertedIncidences() and deletedIncidences().07:38
dcalisteIf we don't kill the deleted one on recreation, there are creationDate / deletedDate couples that may end up with the incidence being listed in the two calls.07:39
dcalisteWhich would confuse the logic after.07:39
chriadamvery good point07:39
dcalisteThere is the same issue in CalDAV sync.07:39
dcalisteThere, the calls are allIncidences() and deletedIncidences(), but we can apply the same reasoning I think.07:40
chriadamI'm trying to think of some inverse case like:07:40
chriadam1) user has added some event, then triggers sync -> it gets upsynced to server, but then some transient failure makes the rest of the cycle fail07:40
chriadam2) user deletes the event, then re-creates the event, then triggers sync07:40
chriadamI guess in (1) case, now that we have per-event error reporting, we can handle this case gracefully.07:40
dcalisteIf I understand well, the user recreate the event on device ? In that case, it's not really a re-creation since it will end up with a different UID.07:41
chriadamwell, not really sure of the mechanism07:42
dcalisteAh, good point.07:42
dcalisteI think in that case, it's the same issue than a re-creation from sync.07:42
dcalisteThe deleted event should not be in the DB anymore when the event is re-created.07:43
chriadamyes, you're right.07:43
dcalisteTo avoid double listing on next sync.07:43
chriadam@flypig: are you around at the moment?07:44
chriadamhi ;-)07:44
flypigI've not been following though I'm afraid.07:44
chriadamdcaliste and I have just been discussing mkcal PR#48 and #5107:44
flypigOkay, I'm just pulling them up.07:45
dcalisteHello flypig, thanks.07:45
chriadamI'm pretty happy with MR#48 (if you are).  we were just considering whether #51 could possibly cause any issues.  damien has convinced me so far that the "double reporting" of the event (old deleted copy in deletedIncidences() plus recreated copy in addedIncidences()) is always problematic07:46
chriadambut I'm still concerned that there's some case we're not seeing, which "requires" us to know that the old copy was previously deleted and then recreated, in order for sync to work correctly (e.g. maybe some partially completed sync cycle or something..)07:46
flypigI guess there's no problem with MR#48, depending on how deleteIncidences is used. I'm not sure there's any sigutation the change would affect right now.07:47
chriadamyeah 48 looks safe to me, also07:47
chriadamit's 51 which I have questions07:48
flypigSo, all deleted events get purged on every insert.07:51
chriadamall?  should just be the one matching uid with deletedDate!=null07:51
chriadamunless I'm mistaken07:51
flypigAh, yes, sorry, my misreading.07:51
dcalisteyes, matching uid and with a deletedDate.07:52
flypigDoes the deletion still get upsynced?07:53
chriadamit may or may not have been upsynced previously, depending on whether the sync plugin was invoked (and thus called deletedIncidences()) prior to the event being recreated (e.g. due to backup/restore or something, who knows)07:54
dcalisteExact, quicker answer than mine.07:54
flypigIf it wasn't upsynced previously, does it matter that it never gets upsynced?07:54
chriadamif the event was both deleted and then recreated , before the sync plugin was invoked, then it will only be reported as an addedIncidence()07:55
chriadamhrm, well, the only thing that matters is that it will be reported as addedIncidence() rather than merely modified or unmodified07:55
flypigIf it was reported as "deleted + added" or "modified" then all would be correct, yes?07:56
dcalisteThat's the problem in my opinion, it is reported in the insertedIncidences() list and the deletedIncidences() one.07:57
dcalisteThe idea, is to avoid the duplication of treating this specific case in the sync plugins.07:57
dcalisteBut treat it earlier once for all in mKCal.07:57
chriadamwe don't really want to report it in deletedIncidences() in the delete+recreate case.  reporting in either addedIncidences() only (or, preferably, in modifiedIncidences() only) makes the most sense.07:57
chriadamhrm, but we cannot report it in modified incidences (by which I mean: we can't "steal" the created/modified timestamp from the deleted incidence and put it in the recreated incidence, because that previous deletion MAY have been upsynced previously, for example)07:58
chriadamwe're probably overthinking this.  it's DEFINITELY wrong to report it as both deleted+added.  so I think the PR is an improvement.  but there may be an edge case which we need to consider in future if we hit it, specifically related to ... I dunno, an attempt to upsync an event which already exists server-side.07:59
flypigIf the deletion was upsynced previously, it is in theory possible to tell.07:59
chriadamif the deletion was upsynced, I think it's fine as is (because if it got recreated on client side, then we need to upsync that as a "normal addition" during the next sync cycle anyway)08:00
chriadamI think the error case is if the deletion was NOT upsynced, but the recreation is reported as an ADDITION.08:00
dcalisteIndeed, I would be tricky to put it in the modified bucket. Normally, if it is recreated by sync, it won't appear in addedIncidences(), because the insertion in the sync plugin ensure that the creation date is older than the sync date.08:00
chriadamfwiw backup/restore shouldn't do this (as we only backup/restore the local calendar content, which should never get synced anywhere)08:01
chriadamso, it's probably a non-existent problem.08:01
dcalisteIt could become one if after #48 we begin to list all deleted incidences and not only the one after a given date.08:02
dcalisteThe idea of #48 is to allow to retry a failed attempt to delete an upstream resource on next sync.08:02
dcalisteBecause at the moment, the sync date being later than the deleted one, it won't appear on next sync.08:03
dcalisteIt was not an issue before being cool on failure because the sync date was not increased in that case.08:03
chriadamI'm not sure I'm following08:04
flypigWon't this become a problem in general? If an upsync fails, then the modified date will also result in something similar happening.08:04
dcalisteAdditionally, I would like to have #48 available also because I would like to "migrate" CalDAV entries to use new setUrl() and I need to migrate also the deleted incidences...08:05
flypigI.e. a partial upsync leaves things inconsistent if the upsync date is after the modified date of all the partial failed upsyncs.08:05
dcalisteThe issue with deleted (and possibly modified also true):08:05
dcaliste- delete on time T108:05
dcaliste- sync at T2 > T108:05
dcaliste- failed to sync the deleted event, but now the sync date is set to T2, because we allow some failures (imagine that other added events were synced...)08:06
dcaliste- sync again at T3, deletedIncidences() won't report the deleted event because its deleted date is earlier than T208:07
dcalisteSo to handle this case gracefully, we need to be able to list all deleted events and try to repush them.08:08
flypigIf I understand correctly, this is a change in the semantics. Now "deleted+sync" times can no longer be used to determine whether a deletion was upsynced. Instead, purging is being replaced for this purpose.08:08
dcalisteBut if an event is recreated, I don't want the event to be listed in deleted ones.08:08
dcalisteflypig : yes.08:08
flypigI.e. a deleted event is only considered upsynced if it has been purged.08:08
dcalistebecause the sync date is true only for succeeded events...08:09
dcalisteflypig : yes.08:09
dcalisteFor the failed attempt for modified events, we can move by hand the modifiedDate to lure the next call to modifiedIncidences()08:09
dcalisteBut we cannot do that for deletedDate.08:09
flypigThat could be done for deleted events too.08:09
flypigAh, why not08:10
dcalisteOr at least I don't see how.08:10
flypigThe API doesn't support it, but it could do.08:10
chriadamcan we do it for added events also?  after all, additions can fail as easily as modifications08:10
dcalisteAh ok, we can add an API to do it.08:10
dcalisteYes we can set by hand the created date.08:11
chriadamI guess yes, but setting creation date > sync date in that case aso08:11
dcalisteSo we can play the same trick the added events also when they fail to upsync.08:11
chriadamok, so what's the issue with the deletedDate?  there's currently no "setDeletedDate(date, incidenceUid, recId)" api?08:11
dcalisteIn my opinion, it make more sense to allow listing all deleted events and try to upsync them again, than adding an API to set the deleted date.08:12
flypigCorrect, I think there's no way to modify the deletedDate.08:12
flypigI agree, but I'm just wondering if we're essentially moving towards adding an "upsynced" flag for every event.08:12
chriadamdcaliste: ooi why do you think that makes more sense, given that we effectively do it the other way for added/modified events?08:13
flypigForward adjusting a deleted date could be dangerous, I think. For example, it could be shifted being a successful "added" modification that happened after it.08:14
flypigbeing = beyond08:14
chriadamI agree in principle that shifting the date seems bad, and that the "list all deleted events" api seems better.  but wondering why it's NOT deemed so bad in the added/modified case haha.08:15
chriadambut, sure, I have no problem with #4808:15
flypigIs this something that's already done in the added/modified case?08:17
chriadamI don't recall doing so in google-calendar at least08:17
chriadamor exchange case08:17
chriadambut not 100% sure08:17
chriadamI think in general, we just assume that if an addition or modification fails to upsync, then it's a "broken" event which shouldn't be upsynced08:18
chriadamso we just "allow the sync cycle to succeed, and leave that one event behind"08:18
chriadamthen with damien's other change, we can display which events weren't upsynced successfully in the UI08:18
chriadamwhich ... is fine.08:18
flypigYes, sounds fine to me too.08:18
chriadamso, do you have any problem in principle with #48, given all the above?  if not, what about #51 (I added a comment there, but was made before our discussion expanded)08:19
flypigCan I have a bit of time to think on #51 today please? I suspect my conclusion will be that I'm fine with it in practice, but that I fear it's masking a deeper issue related to failed syncs.08:21
chriadamof course08:21
chriadamthank you very much08:21
flypigI'll post a comment to it today.08:22
chriadamdcaliste: I understand your other meeting is very soon08:22
chriadamdcaliste: perhaps you could quickly list any other outstanding PRs which need my attention08:22
dcalisteSorry, I was off to help my wife removing the ice on the car :(08:22
chriadamno problem ;-)08:22
chriadamwinter is a different thing in countries other than Australia haha08:22
chriadamthe only thing I've ever had to remove ice from is my freezer08:23
dcalisteNo problem to take more time to think about these MRs.08:23
dcalisteForgot to put the car in the garage yesterday evening, my bad.08:23
dcalisteBesides, allowing the sync to continue on failures is new, so these failing cases are not handled at all at the moment.08:24
dcalisteNot for deleted, nor added nor modified.08:24
dcalistechriadam, maybe, before I need to connect to my video meeting, just some words about the QMF migration :08:25
flypigIn many cases, it's not even clear to me that there is an indisputably "correct" approach here.08:26
flypigSorry, I'll leave you to discuss migration, but thank you both for your comments on my Google sync PR. Very helpful, and I'm in the process of making some changes based on them.08:26
dcalisteflypig, yes I agree. And thanks for your participation to the discussion.08:27
chriadamtyvm flypig08:27
dcalisteabout QMF, I've added your patch chriadam and added another one for QSslError inclusion.08:28
dcalisteIt now compiles. I didn't yet test it again.08:28
chriadamthanks!  I haven't yet started running unit tests or looking at things in detail, but will this week08:28
chriadamI mean, it's a huge effort just to get it compiling - thank you very much for your work there08:28
dcalisteOk, thanks, don't hesitate to report failures.08:29
chriadamI guess if nothing else to discuss, we can close the meeting.  thanks again for your efforts and contributions, greatly appreciated as always.08:29
chriadamI hope you have a great week!08:29
dcalisteThank you also and have a nice week.08:29
pahi folks09:28
pawhere can i find the-other-half-developer-kit ? page seems gone09:28
pajust in case, has anybody here saved the kit?09:30
pawhy pulling it anyway..09:49
panvm found enough09:54
*** vals_ is now known as tango_13:18
*** albertux1 is now known as albertux16:48
*** Sail0r_ is now known as Sail0r18:56

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