Tuesday, 2021-06-22

dcalisteHello chriadam, how are you ?07:01
chriadamhi dcaliste07:01
chriadamI'm well, thanks.  How are you?07:02
dcalisteI'm fine thank you.07:03
dcalisteI've migrated most of my pending MRs to github.07:03
chriadamcould you paste links to them, just so I have a list of them all?07:04
dcalisteExcept the QMF one, that I wanted to address your comments first before resubmitting it.07:04
dcalisteBut I haven't had the time yet...07:04
chriadamexcellent, thanks07:05
chriadamshall we go from bottom to top, starting with the mkcal one07:06
dcalisteYes, let's do like that.07:06
chriadamthe mkcal remove organizer from attendee list PR07:06
chriadamso, for this one, my understanding is that: once we merge this, we can remove some code from downsync codepath in plugins, but NOT from upsync codepath as we haven't migrated existing events in dbs07:07
chriadambut aside from that note, there were no open discussion points, I think?07:08
dcalisteNot exactly, with the organizer removal MR, the downstream code was only in upload parts.07:09
dcalisteAnd it can then be removed.07:09
dcalisteWhat happen is this:07:09
dcaliste- on download, the organizer can or cannot be in the attendee list,07:09
dcaliste- then on save in mkcal, this possibility is lost because of schema layout,07:10
dcaliste- on read from DB, one can choose the convention to always add organizer as attendee or never do it.07:10
dcalisteBefore the first convention was in use, which makes the organizer always in the attendee list when reading the DB, which in turn makes the servers after upload send a mail to the aroganizer asking for participation...07:11
dcalisteSo the old bug fix was to remove the organizer from attendee list before upload.07:11
dcalisteBut this MR propose to do it earlier, by adopting the other convention in mKCal when reading an event.07:12
dcalisteSo the organizer is never in the attendee list (but it is adding in the UI, thanks to code already existing in nemo-qml-plugin-calendar).07:12
dcalisteAs a consequence, the code removing the organizer from the attendee list before upload can be removed in every sync plugin.07:13
chriadamthat makes sense07:14
chriadamwasn't there some symmetric code (which I guess must be on download side, in the sync plugin) which we cannot remove, unless we first do a migration of existing db data?  or am I confusing two different MRs?07:15
dcalisteNo there is no code on download, because KCalendarCore support the separation between organizer and attendee. It's mKCal that fails to do the separation.07:16
dcalisteThe symmetric code is for the EXDATE property. The other MR.07:17
chriadamnow I remember07:17
chriadamok, so this one (organizer) is quite simple: we merge it, and then we can remove some code from sync plugins in upsync codepath07:17
chriadam(that's right, there's some edge case which we now wouldn't support, but we don't care about)07:18
dcalisteExact, and it is not mandatory to do a synced merge, because the code from sync plugin will continue to work, it will just do nothing.07:18
dcalisteI will just bump the Requires in the spec, to be sure that sync plugins compile with a version of mKCal that has the reversed convention.07:19
chriadamsounds good.  I've poked flypig and pvuorela on that MR, but otherwise I will merge it this week.07:20
chriadamok, next one: the exdate one07:20
chriadamwhere did we get up to, with that one?  I think flypig had a couple of comments but those were addressed already?07:22
dcalisteYes, I addressed the mistakes that he spotted. It remains the possibility that will exist now to dissociate twice on the same exdatetime...07:23
dcalisteIt's this comment : https://git.sailfishos.org/mer-core/mkcal/merge_requests/66#note_9186007:24
flypigI might have said I'd do some testing with EAS and Google, but I'm afraid I didn't get around to it yet.07:24
dcalisteThe summery of it is that this problem exists upstream and is not necessary a problem according to the RFC, it depends on the sequence property.07:25
dcalisteMy own conclusion is that since mKCal has already troubles with uniqueness between UID/sequence pairs, creating potentially the same issue for UID/RecID/sequence triplets is a minor inconvenience.07:26
chriadamhow does the MR affect this case?  I mean, if we tried to create multiple exceptions on the same date, previously, I assumed it would have failed at storage save time, also?07:27
chriadamor am I mistaken, and this PR creates a new "failure" case here?07:27
dcalistePreviously, it would have failed in the dissociation code, because the parent knows when are the exception.07:27
dcalisteWith the MR, the parent does not know anymore about the exception.07:28
dcalisteOnly the calendar knows about.07:28
dcalisteSo the failure is translated now in the save() function of mKCal, when the uniqueness constraint will fail.07:28
chriadamseems fine to me.07:29
dcalisteTechnically, from the MR, one can have several identical UID/RecID in a calendar as long as the SEQUENCE priperty is different.07:29
flypigIs there any place this can happen in practice currently? I'd guess maybe with data coming from a remote server?07:29
dcalisteSo, it would be a RFC violation to add a constraint in the dissociation restricting to different dates.07:30
dcalisteflypig, yes, sync will fail now if we receive a valid calendar with let say two events with the same UID, but with a different SEQUENCE.07:31
flypigWould fail before and will also fail after, but maybe with slightly different consequences?07:31
dcalisteIt will fail with the current code I think, except if we discard the lower sequence value events, but I think there is no code actually doing this.07:31
dcalisteThe difference after this MR, is that it will fail in mKCal save for exceptions while before it would have failed at dissociation.07:32
dcalisteSo technically consequences could be different, depending on how these two failures are handled.07:32
chriadamwe don't even support "same event in different notebooks" do we?  I thought there was some issue with that, or maybe you fixed it already07:32
chriadambut ISTR having some code somewhere to prefix the UID with the notebook UID, for that case..07:33
flypigI'm just wondering if it would potentially convert an error flagged onto a single item onto an error for the entire sync?07:33
dcalistechriadam : exact, but there is (ugly) code in every sync plugin to scramble and descramble received UIDs to ensure uniaueness between notebooks.07:33
chriadamflypig: ... interesting point..07:34
flypigI must admit I'm sceptical the situation ever arises in practice.07:35
dcalisteflypig: yes, that's a potential bigger issue, but at the moment, I've never seen any server sending events with different sequences.07:36
flypigYes, it does seem like such a small risk.07:36
dcalisteThat being said, one can add a call to instances() inside the dissociation routine, fetching the exceptions from the calendar and checking that none already exist. We would recover the exact old behaviour.07:37
chriadamand also hit the db again, which I'd prefer to avoid07:38
dcalisteWell, technically, instances() is only handling the QMap from the memory calendar.07:39
chriadamwill it already have been loaded / populated?07:39
chriadamif so, then perfect07:39
dcalisteOf course, it should be populated already...07:39
dcalisteIt's in the responsability of the caller in that case, I think.07:39
dcalisteAdding a load from the db inside the dissociation code is technically possible, but is strange in my opinion.07:40
chriadamyeah, wouldn't want to hit the db itself within the dissociation code.07:41
chriadambut if it's already populated, then I have no concerns about calling instances()07:41
dcalisteOk, then, I'll add it to the MR.07:41
chriadamthank you07:42
chriadamflypig: ^ do you agree?07:42
flypigHonestly, I'm convinced from the discussion that this is very unlikely to cause problems as it is, but preserving the existing behaviour will certainly minimise issues if it's possible and straightforward.07:44
flypigSo I would go with whichever you decide, and if you're happy with this then I definitely agree.07:44
chriadamnext one: caldav alarm duration during export07:45
chriadamI think we agreed I'd just merge this one.  anyone object?07:45
dcalisteThank you flypigCalling, just instances() is a minimal cost and will recover the exact previous behaviour as long as the memory calendar has been populated already.07:45
flypigPerfect, thanks both for the useful discussion.07:46
flypigchriadam: no objections from me (I don't think I was involved with the PR).07:47
dcalistechriadam, if as far as I can tell this code in caldav has no effect on deviceand normally it should have no effect on servers neither where the values are sent, because this code was just rewriting the same info in a different maner.07:47
chriadamkf5 calendarcore one: update to latest upstream + pkgconfig support... I haven't looked at that one in-depth, but I have no concerns with it07:48
chriadamhmm, we have no bug number associated with that one07:49
chriadamI wonder which bug pvuorela uses for such "update to upstream" cases07:49
chriadamshall we move on the next one: nqpc list mode07:51
flypigThere was JB#47814? but that's closed now.07:51
chriadamwas this one related to the buteo sync log bug?07:52
dcalisteIf you have a new JB id, I can add it, no problem.07:54
chriadamadded comments to the MRs, we have JB#49486 for the buteo sync log UI thing07:55
chriadamfor the kf5 calendarcore one, I will ask pvuorela07:55
dcalistechriadam, yes the nemo-qml-plugin one is related to exposing the log results to UI.07:55
chriadampoked people in both of those MRs07:59
chriadamI won't promise to merge those two this week07:59
dcalisteThank you chriadam.07:59
chriadamin case Pekka wants some more time to consider07:59
chriadamI will merge the caldav one, plus the two mkcal ones, unless someone objects tomorrow / thursday08:00
dcalisteOf course, they are still under discussion, beside their technical characteristics, about the need to actually do it or not.08:00
chriadamthe kf5calendarcore one I will leave to pvuorela I think08:00
chriadamdcaliste: yeah.  I think jpetrell took a look a while back, but he's been somewhat sidetracked08:00
chriadamin general, I tend to think: if the enablers are merged (and don't affect anything else) then that can only reduce the friction to getting the useful feature implemented in future08:01
dcalistechriadam, I agree with this. Particularly, it would allow to create a UI package outside the OS for demonstration purposes.08:02
chriadamyep, I agree.08:02
chriadamlet's discuss again next week, and push the buttons then, in any case08:02
dcalisteOk, great.08:02
chriadamwas there anything else to discuss this week?08:02
chriadamflypig: did you have any topics?08:03
flypigI have a non-technical question after the technical ones have been covered.08:03
chriadamwith some PR in particular, you mean?  or?08:03
flypigJust related to the newsletter: would you be up for creating a one-paragraph summary of recent changes dcaliste? It doesn't have to be this week, but whenever you think it's a good time to mention it.08:04
flypigIf you prefer I can do this myself, but with less competence :)08:06
dcalisteYes, I can write something about cleaning mKCal, upstreaming as much as possible, and the interaction with kcalendarcore KDE project. There can be another topic about internals of sync plugins, how they work and so one. Or both as you prefer.08:06
flypigI'd like to include a brief summary as often as possible, so if you'd be willing to do both, as separate items for separate newsletters, then that would be particularly good from my point of view.08:07
flypigI don't want this to be a distraction though.08:08
dcalisteOk, let's do like that. I can prepare one later this week, and the second for early next week I think.08:08
flypigThat would be fantastic. If you're able to get me something this week, then you have a two-week window after that. But there is no rush with either.08:08
chriadamalso, thanks very much for moving all the MRs over the github already08:09
dcalisteRight, but I'll be on the beach early in July, so I think, I will try to create the second one before that ;)08:09
chriadamhaha, sounds good!  hopefully you have some nice weather for your beach vacation!08:10
flypigYes, sounds great :D08:10
chriadamok, if nothing else to discuss, let's end the meeting.  again, thanks very much for all your efforts, and see you next week!08:10
* chriadam -> away, gnight!08:10
flypigThank you dcaliste, chriadam.08:10
dcalisteThank you both, have a nice week.08:12
*** coulomb.oftc.net sets mode: +o cybette12:19
*** ChanServ changes topic to "https://www.sailfishos.org | Developers: https://sailfishos.org/wiki | Community: https://sailfishos.org/community | Logs: https://irclogs.sailfishos.org/logs/%23sailfishos"12:29
*** bernd is now known as burnson14:03

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