| *** frinring_ is now known as frinring | 01:29 | |
| *** zbenjamin_ is now known as zbenjamin | 02:05 | |
| dcaliste | Good evening chriadam. Would you mind if we start a bit earlier today, I've got a video meeting at 8.30 UTC... | 07:28 |
|---|---|---|
| chriadam | hi dcaliste | 07:31 |
| chriadam | certainly, no problem | 07:31 |
| chriadam | I hope you had a good week? | 07:31 |
| dcaliste | Yes, 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 |
| dcaliste | flypig, merged the jolla calendar MR 266. Thanks to him. And you did for kcalendarcore #2, thanks also. | 07:33 |
| dcaliste | And pvuorela merged the color / url one in mkcal. Thanks again. | 07:33 |
| chriadam | very nice | 07:33 |
| chriadam | I hope that the skiing was enjoyable :-) | 07:34 |
| chriadam | my week was good also, thanks | 07:34 |
| chriadam | yep, got some things merged | 07:34 |
| chriadam | didn't get jolla-calendar PR#267 in yet, though, unfortunately | 07:34 |
| chriadam | I pinged Pekka and Martin about that one again though, so hopefully in the next couple of days | 07:34 |
| dcaliste | So 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 |
| dcaliste | Thanks for #267. | 07:35 |
| chriadam | this is mkcal MR#48+51 I believe | 07:36 |
| dcaliste | About the latter, you wonder if it could affect the deletedIncidences() call in sync processes. (yes that's MR #48 and #51). | 07:37 |
| dcaliste | I 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 |
| dcaliste | In the google sync adaptor, there are two fetchs : insertedIncidences() and deletedIncidences(). | 07:38 |
| dcaliste | If 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 |
| dcaliste | Which would confuse the logic after. | 07:39 |
| chriadam | very good point | 07:39 |
| dcaliste | There is the same issue in CalDAV sync. | 07:39 |
| dcaliste | There, the calls are allIncidences() and deletedIncidences(), but we can apply the same reasoning I think. | 07:40 |
| chriadam | I'm trying to think of some inverse case like: | 07:40 |
| chriadam | 1) user has added some event, then triggers sync -> it gets upsynced to server, but then some transient failure makes the rest of the cycle fail | 07:40 |
| chriadam | 2) user deletes the event, then re-creates the event, then triggers sync | 07:40 |
| chriadam | I guess in (1) case, now that we have per-event error reporting, we can handle this case gracefully. | 07:40 |
| dcaliste | If 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 |
| chriadam | backup/restore | 07:42 |
| chriadam | well, not really sure of the mechanism | 07:42 |
| dcaliste | Ah, good point. | 07:42 |
| dcaliste | I think in that case, it's the same issue than a re-creation from sync. | 07:42 |
| dcaliste | The deleted event should not be in the DB anymore when the event is re-created. | 07:43 |
| chriadam | yes, you're right. | 07:43 |
| dcaliste | To avoid double listing on next sync. | 07:43 |
| chriadam | @flypig: are you around at the moment? | 07:44 |
| flypig | Hi | 07:44 |
| chriadam | excellent | 07:44 |
| chriadam | hi ;-) | 07:44 |
| flypig | I've not been following though I'm afraid. | 07:44 |
| chriadam | dcaliste and I have just been discussing mkcal PR#48 and #51 | 07:44 |
| flypig | Okay, I'm just pulling them up. | 07:45 |
| dcaliste | Hello flypig, thanks. | 07:45 |
| chriadam | I'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 problematic | 07:46 |
| chriadam | but 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 |
| flypig | I 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 |
| chriadam | yeah 48 looks safe to me, also | 07:47 |
| chriadam | it's 51 which I have questions | 07:48 |
| flypig | So, all deleted events get purged on every insert. | 07:51 |
| chriadam | all? should just be the one matching uid with deletedDate!=null | 07:51 |
| chriadam | unless I'm mistaken | 07:51 |
| flypig | Ah, yes, sorry, my misreading. | 07:51 |
| dcaliste | yes, matching uid and with a deletedDate. | 07:52 |
| flypig | Does the deletion still get upsynced? | 07:53 |
| chriadam | it 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 |
| dcaliste | Exact, quicker answer than mine. | 07:54 |
| flypig | If it wasn't upsynced previously, does it matter that it never gets upsynced? | 07:54 |
| chriadam | if 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 |
| chriadam | hrm, well, the only thing that matters is that it will be reported as addedIncidence() rather than merely modified or unmodified | 07:55 |
| flypig | If it was reported as "deleted + added" or "modified" then all would be correct, yes? | 07:56 |
| chriadam | yes | 07:56 |
| dcaliste | That's the problem in my opinion, it is reported in the insertedIncidences() list and the deletedIncidences() one. | 07:57 |
| dcaliste | The idea, is to avoid the duplication of treating this specific case in the sync plugins. | 07:57 |
| dcaliste | But treat it earlier once for all in mKCal. | 07:57 |
| chriadam | we 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 |
| chriadam | hrm, 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 |
| chriadam | we'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 |
| flypig | If the deletion was upsynced previously, it is in theory possible to tell. | 07:59 |
| chriadam | if 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 |
| chriadam | I think the error case is if the deletion was NOT upsynced, but the recreation is reported as an ADDITION. | 08:00 |
| dcaliste | Indeed, 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 |
| chriadam | fwiw backup/restore shouldn't do this (as we only backup/restore the local calendar content, which should never get synced anywhere) | 08:01 |
| chriadam | so, it's probably a non-existent problem. | 08:01 |
| dcaliste | It could become one if after #48 we begin to list all deleted incidences and not only the one after a given date. | 08:02 |
| dcaliste | The idea of #48 is to allow to retry a failed attempt to delete an upstream resource on next sync. | 08:02 |
| dcaliste | Because at the moment, the sync date being later than the deleted one, it won't appear on next sync. | 08:03 |
| dcaliste | It was not an issue before being cool on failure because the sync date was not increased in that case. | 08:03 |
| chriadam | I'm not sure I'm following | 08:04 |
| flypig | Won't this become a problem in general? If an upsync fails, then the modified date will also result in something similar happening. | 08:04 |
| dcaliste | Additionally, 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 |
| flypig | I.e. a partial upsync leaves things inconsistent if the upsync date is after the modified date of all the partial failed upsyncs. | 08:05 |
| dcaliste | The issue with deleted (and possibly modified also true): | 08:05 |
| dcaliste | - delete on time T1 | 08:05 |
| dcaliste | - sync at T2 > T1 | 08: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 T2 | 08:07 |
| dcaliste | So to handle this case gracefully, we need to be able to list all deleted events and try to repush them. | 08:08 |
| flypig | If 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 |
| dcaliste | But if an event is recreated, I don't want the event to be listed in deleted ones. | 08:08 |
| dcaliste | flypig : yes. | 08:08 |
| flypig | I.e. a deleted event is only considered upsynced if it has been purged. | 08:08 |
| dcaliste | because the sync date is true only for succeeded events... | 08:09 |
| dcaliste | flypig : yes. | 08:09 |
| dcaliste | For the failed attempt for modified events, we can move by hand the modifiedDate to lure the next call to modifiedIncidences() | 08:09 |
| dcaliste | But we cannot do that for deletedDate. | 08:09 |
| flypig | That could be done for deleted events too. | 08:09 |
| flypig | Ah, why not | 08:10 |
| flypig | ? | 08:10 |
| dcaliste | Or at least I don't see how. | 08:10 |
| flypig | The API doesn't support it, but it could do. | 08:10 |
| chriadam | can we do it for added events also? after all, additions can fail as easily as modifications | 08:10 |
| dcaliste | Ah ok, we can add an API to do it. | 08:10 |
| dcaliste | Yes we can set by hand the created date. | 08:11 |
| chriadam | I guess yes, but setting creation date > sync date in that case aso | 08:11 |
| dcaliste | So we can play the same trick the added events also when they fail to upsync. | 08:11 |
| chriadam | ok, so what's the issue with the deletedDate? there's currently no "setDeletedDate(date, incidenceUid, recId)" api? | 08:11 |
| dcaliste | In 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 |
| flypig | Correct, I think there's no way to modify the deletedDate. | 08:12 |
| flypig | I agree, but I'm just wondering if we're essentially moving towards adding an "upsynced" flag for every event. | 08:12 |
| chriadam | dcaliste: ooi why do you think that makes more sense, given that we effectively do it the other way for added/modified events? | 08:13 |
| flypig | Forward 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 |
| flypig | being = beyond | 08:14 |
| chriadam | I 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 |
| chriadam | but, sure, I have no problem with #48 | 08:15 |
| flypig | Is this something that's already done in the added/modified case? | 08:17 |
| chriadam | I don't recall doing so in google-calendar at least | 08:17 |
| chriadam | or exchange case | 08:17 |
| chriadam | but not 100% sure | 08:17 |
| chriadam | I 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 upsynced | 08:18 |
| chriadam | so we just "allow the sync cycle to succeed, and leave that one event behind" | 08:18 |
| chriadam | then with damien's other change, we can display which events weren't upsynced successfully in the UI | 08:18 |
| chriadam | which ... is fine. | 08:18 |
| chriadam | IMO. | 08:18 |
| flypig | Yes, sounds fine to me too. | 08:18 |
| chriadam | so, 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 |
| flypig | Can 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 |
| chriadam | of course | 08:21 |
| chriadam | thank you very much | 08:21 |
| flypig | I'll post a comment to it today. | 08:22 |
| chriadam | dcaliste: I understand your other meeting is very soon | 08:22 |
| chriadam | dcaliste: perhaps you could quickly list any other outstanding PRs which need my attention | 08:22 |
| dcaliste | Sorry, I was off to help my wife removing the ice on the car :( | 08:22 |
| chriadam | no problem ;-) | 08:22 |
| chriadam | winter is a different thing in countries other than Australia haha | 08:22 |
| flypig | :D | 08:23 |
| chriadam | the only thing I've ever had to remove ice from is my freezer | 08:23 |
| dcaliste | No problem to take more time to think about these MRs. | 08:23 |
| dcaliste | Forgot to put the car in the garage yesterday evening, my bad. | 08:23 |
| dcaliste | Besides, allowing the sync to continue on failures is new, so these failing cases are not handled at all at the moment. | 08:24 |
| dcaliste | Not for deleted, nor added nor modified. | 08:24 |
| dcaliste | chriadam, maybe, before I need to connect to my video meeting, just some words about the QMF migration : | 08:25 |
| flypig | In many cases, it's not even clear to me that there is an indisputably "correct" approach here. | 08:26 |
| flypig | Sorry, 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 |
| dcaliste | flypig, yes I agree. And thanks for your participation to the discussion. | 08:27 |
| chriadam | tyvm flypig | 08:27 |
| dcaliste | about QMF, I've added your patch chriadam and added another one for QSslError inclusion. | 08:28 |
| dcaliste | It now compiles. I didn't yet test it again. | 08:28 |
| chriadam | thanks! I haven't yet started running unit tests or looking at things in detail, but will this week | 08:28 |
| chriadam | I mean, it's a huge effort just to get it compiling - thank you very much for your work there | 08:28 |
| dcaliste | Ok, thanks, don't hesitate to report failures. | 08:29 |
| chriadam | :-) | 08:29 |
| chriadam | I guess if nothing else to discuss, we can close the meeting. thanks again for your efforts and contributions, greatly appreciated as always. | 08:29 |
| chriadam | I hope you have a great week! | 08:29 |
| dcaliste | Thank you also and have a nice week. | 08:29 |
| pa | hi folks | 09:28 |
| pa | where can i find the-other-half-developer-kit ? page seems gone | 09:28 |
| pa | just in case, has anybody here saved the kit? | 09:30 |
| pa | why pulling it anyway.. | 09:49 |
| pa | nvm found enough | 09:54 |
| *** vals_ is now known as tango_ | 13:18 | |
| *** albertux1 is now known as albertux | 16:48 | |
| *** Sail0r_ is now known as Sail0r | 18:56 | |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!