| dcaliste | Hello chriadam, how are you ? | 07:01 |
|---|---|---|
| chriadam | hi dcaliste | 07:01 |
| chriadam | I'm well, thanks. How are you? | 07:02 |
| dcaliste | I'm fine thank you. | 07:03 |
| dcaliste | I've migrated most of my pending MRs to github. | 07:03 |
| chriadam | could you paste links to them, just so I have a list of them all? | 07:04 |
| dcaliste | Except the QMF one, that I wanted to address your comments first before resubmitting it. | 07:04 |
| dcaliste | But I haven't had the time yet... | 07:04 |
| dcaliste | https://github.com/pulls?q=is%3Aopen+is%3Apr+author%3Adcaliste+archived%3Afalse+user%3Asailfishos | 07:04 |
| chriadam | excellent, thanks | 07:05 |
| chriadam | shall we go from bottom to top, starting with the mkcal one | 07:06 |
| dcaliste | Yes, let's do like that. | 07:06 |
| chriadam | the mkcal remove organizer from attendee list PR | 07:06 |
| chriadam | so, 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 dbs | 07:07 |
| chriadam | but aside from that note, there were no open discussion points, I think? | 07:08 |
| dcaliste | Not exactly, with the organizer removal MR, the downstream code was only in upload parts. | 07:09 |
| dcaliste | And it can then be removed. | 07:09 |
| dcaliste | What 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 |
| dcaliste | Before 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 |
| dcaliste | So the old bug fix was to remove the organizer from attendee list before upload. | 07:11 |
| dcaliste | But this MR propose to do it earlier, by adopting the other convention in mKCal when reading an event. | 07:12 |
| dcaliste | So 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 |
| dcaliste | As a consequence, the code removing the organizer from the attendee list before upload can be removed in every sync plugin. | 07:13 |
| chriadam | that makes sense | 07:14 |
| chriadam | but | 07:14 |
| chriadam | wasn'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 |
| dcaliste | No 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 |
| chriadam | ok | 07:17 |
| dcaliste | The symmetric code is for the EXDATE property. The other MR. | 07:17 |
| chriadam | ah! | 07:17 |
| chriadam | yes | 07:17 |
| chriadam | now I remember | 07:17 |
| chriadam | ok, so this one (organizer) is quite simple: we merge it, and then we can remove some code from sync plugins in upsync codepath | 07:17 |
| chriadam | (that's right, there's some edge case which we now wouldn't support, but we don't care about) | 07:18 |
| dcaliste | Exact, 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 |
| dcaliste | I 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 |
| chriadam | sounds good. I've poked flypig and pvuorela on that MR, but otherwise I will merge it this week. | 07:20 |
| chriadam | ok, next one: the exdate one | 07:20 |
| chriadam | where did we get up to, with that one? I think flypig had a couple of comments but those were addressed already? | 07:22 |
| dcaliste | Yes, I addressed the mistakes that he spotted. It remains the possibility that will exist now to dissociate twice on the same exdatetime... | 07:23 |
| dcaliste | It's this comment : https://git.sailfishos.org/mer-core/mkcal/merge_requests/66#note_91860 | 07:24 |
| flypig | I 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 |
| dcaliste | The 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 |
| dcaliste | My 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 |
| chriadam | how 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 |
| chriadam | or am I mistaken, and this PR creates a new "failure" case here? | 07:27 |
| dcaliste | Previously, it would have failed in the dissociation code, because the parent knows when are the exception. | 07:27 |
| dcaliste | With the MR, the parent does not know anymore about the exception. | 07:28 |
| dcaliste | Only the calendar knows about. | 07:28 |
| dcaliste | So the failure is translated now in the save() function of mKCal, when the uniqueness constraint will fail. | 07:28 |
| chriadam | right | 07:28 |
| chriadam | seems fine to me. | 07:29 |
| dcaliste | Technically, from the MR, one can have several identical UID/RecID in a calendar as long as the SEQUENCE priperty is different. | 07:29 |
| flypig | Is there any place this can happen in practice currently? I'd guess maybe with data coming from a remote server? | 07:29 |
| dcaliste | So, it would be a RFC violation to add a constraint in the dissociation restricting to different dates. | 07:30 |
| dcaliste | flypig, 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 |
| flypig | Would fail before and will also fail after, but maybe with slightly different consequences? | 07:31 |
| dcaliste | It 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 |
| dcaliste | The difference after this MR, is that it will fail in mKCal save for exceptions while before it would have failed at dissociation. | 07:32 |
| dcaliste | So technically consequences could be different, depending on how these two failures are handled. | 07:32 |
| chriadam | we don't even support "same event in different notebooks" do we? I thought there was some issue with that, or maybe you fixed it already | 07:32 |
| chriadam | but ISTR having some code somewhere to prefix the UID with the notebook UID, for that case.. | 07:33 |
| flypig | I'm just wondering if it would potentially convert an error flagged onto a single item onto an error for the entire sync? | 07:33 |
| dcaliste | chriadam : exact, but there is (ugly) code in every sync plugin to scramble and descramble received UIDs to ensure uniaueness between notebooks. | 07:33 |
| chriadam | flypig: ... interesting point.. | 07:34 |
| flypig | I must admit I'm sceptical the situation ever arises in practice. | 07:35 |
| dcaliste | flypig: yes, that's a potential bigger issue, but at the moment, I've never seen any server sending events with different sequences. | 07:36 |
| flypig | Yes, it does seem like such a small risk. | 07:36 |
| dcaliste | That 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 |
| chriadam | and also hit the db again, which I'd prefer to avoid | 07:38 |
| dcaliste | Well, technically, instances() is only handling the QMap from the memory calendar. | 07:39 |
| chriadam | will it already have been loaded / populated? | 07:39 |
| chriadam | if so, then perfect | 07:39 |
| dcaliste | Of course, it should be populated already... | 07:39 |
| dcaliste | It's in the responsability of the caller in that case, I think. | 07:39 |
| dcaliste | Adding a load from the db inside the dissociation code is technically possible, but is strange in my opinion. | 07:40 |
| chriadam | yeah, wouldn't want to hit the db itself within the dissociation code. | 07:41 |
| chriadam | but if it's already populated, then I have no concerns about calling instances() | 07:41 |
| dcaliste | Ok, then, I'll add it to the MR. | 07:41 |
| chriadam | thank you | 07:42 |
| chriadam | flypig: ^ do you agree? | 07:42 |
| flypig | Honestly, 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 |
| flypig | So I would go with whichever you decide, and if you're happy with this then I definitely agree. | 07:44 |
| chriadam | cool | 07:44 |
| chriadam | thanks. | 07:44 |
| chriadam | next one: caldav alarm duration during export | 07:45 |
| chriadam | I think we agreed I'd just merge this one. anyone object? | 07:45 |
| dcaliste | Thank 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 |
| flypig | Perfect, thanks both for the useful discussion. | 07:46 |
| flypig | chriadam: no objections from me (I don't think I was involved with the PR). | 07:47 |
| dcaliste | chriadam, 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 |
| dcaliste | s/if/yes/ | 07:47 |
| chriadam | kf5 calendarcore one: update to latest upstream + pkgconfig support... I haven't looked at that one in-depth, but I have no concerns with it | 07:48 |
| chriadam | hmm, we have no bug number associated with that one | 07:49 |
| chriadam | I wonder which bug pvuorela uses for such "update to upstream" cases | 07:49 |
| chriadam | shall we move on the next one: nqpc list mode | 07:51 |
| flypig | There was JB#47814? but that's closed now. | 07:51 |
| chriadam | was this one related to the buteo sync log bug? | 07:52 |
| dcaliste | If you have a new JB id, I can add it, no problem. | 07:54 |
| chriadam | added comments to the MRs, we have JB#49486 for the buteo sync log UI thing | 07:55 |
| chriadam | for the kf5 calendarcore one, I will ask pvuorela | 07:55 |
| dcaliste | chriadam, yes the nemo-qml-plugin one is related to exposing the log results to UI. | 07:55 |
| chriadam | poked people in both of those MRs | 07:59 |
| chriadam | I won't promise to merge those two this week | 07:59 |
| dcaliste | Thank you chriadam. | 07:59 |
| chriadam | in case Pekka wants some more time to consider | 07:59 |
| chriadam | I will merge the caldav one, plus the two mkcal ones, unless someone objects tomorrow / thursday | 08:00 |
| dcaliste | Of course, they are still under discussion, beside their technical characteristics, about the need to actually do it or not. | 08:00 |
| chriadam | the kf5calendarcore one I will leave to pvuorela I think | 08:00 |
| chriadam | dcaliste: yeah. I think jpetrell took a look a while back, but he's been somewhat sidetracked | 08:00 |
| chriadam | in 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 future | 08:01 |
| dcaliste | chriadam, I agree with this. Particularly, it would allow to create a UI package outside the OS for demonstration purposes. | 08:02 |
| chriadam | yep, I agree. | 08:02 |
| chriadam | let's discuss again next week, and push the buttons then, in any case | 08:02 |
| dcaliste | Ok, great. | 08:02 |
| chriadam | was there anything else to discuss this week? | 08:02 |
| chriadam | flypig: did you have any topics? | 08:03 |
| flypig | I have a non-technical question after the technical ones have been covered. | 08:03 |
| chriadam | with some PR in particular, you mean? or? | 08:03 |
| flypig | Just 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 |
| flypig | If you prefer I can do this myself, but with less competence :) | 08:06 |
| dcaliste | Yes, 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 |
| flypig | I'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 |
| flypig | I don't want this to be a distraction though. | 08:08 |
| dcaliste | Ok, let's do like that. I can prepare one later this week, and the second for early next week I think. | 08:08 |
| flypig | That 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 |
| chriadam | also, thanks very much for moving all the MRs over the github already | 08:09 |
| dcaliste | Right, 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 |
| chriadam | haha, sounds good! hopefully you have some nice weather for your beach vacation! | 08:10 |
| flypig | Yes, sounds great :D | 08:10 |
| chriadam | ok, 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 | |
| flypig | Thank you dcaliste, chriadam. | 08:10 |
| dcaliste | Thank you both, have a nice week. | 08:12 |
| *** coulomb.oftc.net sets mode: +o cybette | 12: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 burnson | 14:03 | |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!