| *** zbenjamin is now known as Guest72056 | 02:14 | |
| *** zbenjamin_ is now known as zbenjamin | 02:14 | |
| *** frinring_ is now known as frinring | 04:55 | |
| dcaliste | Hello chriadam, how are you ? | 07:54 |
|---|---|---|
| chriadam | hi dcaliste, I'm well thanks | 07:54 |
| chriadam | sorry I wasn't at the meeting last week - it was a public holiday here in Australia | 07:54 |
| chriadam | I sent an email on Monday but it bounced, and I only noticed on Wednesday | 07:54 |
| chriadam | my apologies | 07:54 |
| chriadam | how has your week been? | 07:55 |
| dcaliste | No problem, I've known better weeks than the last two though ! Anyway, let say it globally fine. | 07:55 |
| chriadam | uh oh | 07:55 |
| chriadam | well, hopefully the next weeks are better than the last ones | 07:56 |
| dcaliste | Indeed, let start with the Qt bug : https://git.sailfishos.org/mer-core/qtbase/merge_requests/75 | 07:56 |
| dcaliste | Do you think the fact that I read the latest version to get inspiried is a break of copyright ? | 07:57 |
| chriadam | I don't know | 07:57 |
| chriadam | I am not a lawyer | 07:58 |
| chriadam | I assume not | 07:58 |
| dcaliste | The output is totally different than the latest version since many things and even variable names have changed. | 07:58 |
| chriadam | yeah, I think it's probably fine. I will poke someone internally first, though | 07:59 |
| dcaliste | So the patch is totally different in term of modifications than the equivalent one based on latest. | 07:59 |
| dcaliste | Would be great to fix in our Qt version, because it potentially creates a lot of places with issues. | 07:59 |
| chriadam | yeah, I definitely agree | 07:59 |
| dcaliste | If needed, I can think from scratch to a new way to do the same. The patch is so simple and small that it would be easy. | 08:00 |
| chriadam | thanks. shouldn't be necessary, but will check internally just to be sure | 08:01 |
| dcaliste | Ok, great, thank you. | 08:01 |
| dcaliste | Maybe we can chat a little on https://git.sailfishos.org/mer-core/buteo-syncfw/merge_requests/59 if you don't mind ? | 08:02 |
| dcaliste | There are basically two things done in this MR: | 08:02 |
| dcaliste | - rework the code there handling link between account and profile, there was redundancy in my opinion that could be avoided. | 08:03 |
| dcaliste | - reactivate some old code to ensure that profile are disabled when accounts are and the opposite. | 08:03 |
| dcaliste | The latter avoids to start a Buteo sync process and abort it immediately because the plugin detects that the account is disabled. | 08:04 |
| dcaliste | It's not a big deal, but it fill up the logs, and you don't understand why you have logs for something that is disabled. | 08:05 |
| dcaliste | Additionnally it's an issue for webcal stuff for instance because the Buteo plugin is relying on profile only and don't know that the associated account is disabled. | 08:05 |
| chriadam | yes I definitely agree that it's a useful and worthwhile change | 08:05 |
| chriadam | I had a few comments on that one, by the looks, already - not sure if those are still valid or not | 08:06 |
| dcaliste | The former code restructuration should have no impact because it is not used at the moment. sailfish-component-account is taking care of creating profiles for accounts. | 08:06 |
| dcaliste | Yes, I would like to discuss a bit your comments. | 08:06 |
| chriadam | sounds good | 08:07 |
| dcaliste | One issue I noticed in the restructuration is that we are creating one profile per service, so several profiles for one account. | 08:07 |
| dcaliste | While the public API and DBus one of msyncd is built on the assumption that we create one profile with several sub-profile for one account. | 08:08 |
| dcaliste | Thus it is returning the profile name for the newly created profile. | 08:08 |
| dcaliste | This does not work anymore when creating several profiles for one account. | 08:08 |
| chriadam | ah! | 08:09 |
| dcaliste | To conclude, I cannot change much this and particularly the function name because it's part of the (unused) msyncd DBus API. | 08:09 |
| chriadam | interesting | 08:09 |
| chriadam | I wonder what would be required to transition to the "single profile per account, which contains sub-profiles" (so that it matches expectations of those APIs?) | 08:10 |
| chriadam | well, probably too much effort currently, many places which have various assumptions | 08:13 |
| chriadam | I've added a comment to that PR - maybe a code comment could be added above that line, and we can investigate further in future if we get a chance (e.g. perhaps when we look at improving Buteo in general... one day...) | 08:14 |
| dcaliste | Yes, I'm afraid many things are built on top of one profile per service. | 08:20 |
| dcaliste | But the UI is more oriented with one profile per service since the scheduling is for all service at once. But underlying implementation is different. | 08:21 |
| chriadam | yes, many things currently | 08:21 |
| dcaliste | Anyway, I'll follow the route of adding a comment in the Buteo patch explaining how it is at the moment. | 08:22 |
| dcaliste | Last week, I submitted a MR in CalDAV plugin : https://git.sailfishos.org/mer-core/buteo-sync-plugin-caldav/merge_requests/76 | 08:23 |
| dcaliste | I explained in the MR description the purpose of the commits. | 08:24 |
| chriadam | I worry about the non-transient error case | 08:24 |
| dcaliste | To summarize, some commits are to fix minor errors, like the modification date being changed by setting the error flags on incidence. | 08:24 |
| chriadam | do we "track" whether an event has failed multiple times in a row, and should be "dropped" from the upsync set? or do we always retry? | 08:24 |
| dcaliste | At the moment we always retry. | 08:25 |
| dcaliste | And the failure is always logged and ignored. | 08:25 |
| chriadam | I guess it's not much overhead in terms of data / battery... I wonder how many "broken" events are "normal" | 08:27 |
| chriadam | one question: there is a removal of an "mStorage->save()" which had comment "PUT requests are finished, save the updated etags in storage" or similar | 08:30 |
| chriadam | I guess this is answered in a commit message somewhere | 08:30 |
| chriadam | but just wondering why that one can be removed | 08:30 |
| dcaliste | Yes, it was added when we were not fault tolerant to ensure that etags were written before any error will stop the process. | 08:31 |
| dcaliste | But now, we are sure (besides bugs, but I read the code several time to find a quick return I didn't find) that we will save the storage before leaving. | 08:31 |
| chriadam | ah right | 08:32 |
| dcaliste | So no need for early write that may have enexpected consequences like resetting the m_calendar for one unfinished notebook because of database touch on another. | 08:32 |
| chriadam | I see, because otherwise some network error could occur in a request and we would abort the sync cycle on that | 08:32 |
| chriadam | yep. | 08:32 |
| chriadam | the commit "don't list again incidences for modifications" -- can you briefly explain what that's doing / what the assumptions are? | 08:33 |
| chriadam | a lot of "delta determination" code seemed to be removed, wondering how that was able to be done | 08:34 |
| dcaliste | Yes, before the delta was done like that: | 08:34 |
| dcaliste | - list all databse incidence for the current notebook, | 08:34 |
| dcaliste | - list all modified incidences since last sync, | 08:34 |
| dcaliste | - list all deleted incidences. | 08:35 |
| dcaliste | then from the first list find the local additions, from the second the local modifications and from the last the deleted since last sync. | 08:36 |
| dcaliste | Simple enough. | 08:37 |
| dcaliste | But looking at what is returned by these functions, I found a quicker way to do the same. | 08:38 |
| dcaliste | Indeed, the first one list all incidences. And the modification listing is just a subset of this first listing. | 08:38 |
| chriadam | ah, right, of course | 08:39 |
| dcaliste | And the subset is done with a simple rule : create < sync time && last modified > sync time. | 08:40 |
| dcaliste | This rule we can easily apply it in the delta function itself, no need to ping the database again. | 08:41 |
| chriadam | yes, I agree | 08:41 |
| dcaliste | And this simplify a lot the delta calculation since we don't have to bother if an incidence that was listed in the first list was actually a modification. | 08:41 |
| chriadam | and I guess that locally-deleted incidences aren't returned in that first list | 08:41 |
| dcaliste | We can bucket all incidences in the same logic. | 08:41 |
| chriadam | yeah. I agree, definitely a nice change. | 08:42 |
| dcaliste | Yes locally deleted are not returned. | 08:42 |
| chriadam | thank you for the explanation :-) | 08:42 |
| dcaliste | We need this specific call for deleted. | 08:42 |
| flypig | I also have a query about this PR if I may? | 08:46 |
| dcaliste | The only downpart of this is that I cannot test because now the Buteo plugin is based on latest mkcal that is based on KCalendarCore and I don't want to mess up my device by swaping the mkcal package... | 08:46 |
| dcaliste | @flypig, sure go on. | 08:46 |
| flypig | The final change is to retrigger the failed incidences. I was wondering why not just update the lastModified values for the failures. | 08:47 |
| dcaliste | Because I find it too intrusive to modify the last modification date time and a bit artificial, while we have a flag that can be used for this purpose. | 08:48 |
| dcaliste | My first implementation was to modified the last modification though. | 08:49 |
| flypig | Okay; I only ask because on the Google PR I was wondering about the same thing: whether it would be better to let the setCustomProperty() update the modifiedTime naturally or not. | 08:49 |
| dcaliste | But I found it more concise by testing the flag on the next sync. | 08:49 |
| flypig | Thanks. I don't see a problem with your approach; was just curious if there was a problem I missed with the other approach. | 08:50 |
| chriadam | were there any other PRs we needed to discuss? I know that pvuorela and flypig did a bunch of merging last week - thanks to them both for that | 08:52 |
| chriadam | I still need to look into the upstream QMF patches in more detail | 08:52 |
| chriadam | I got them building but didn't yet try running unit tests etc - I will do so this week, pvuorela has given me the green light to do that | 08:53 |
| dcaliste | Ok, great. | 08:55 |
| dcaliste | I've just started to implement a way to avoid alarms on some notebook. | 08:57 |
| dcaliste | The rational is explained in https://git.sailfishos.org/mer-core/mkcal/merge_requests/54 | 08:58 |
| dcaliste | It's still WIP at the moment, I need to figure out where to add it in the UI, and how. | 08:59 |
| dcaliste | Do you think it's a valuable change or it's too specific ? | 08:59 |
| chriadam | I don't know, honestly. I think "in principle" the idea of having a "non-noisy / non-alarming" property for a notebook makes sense. the two use-cases you gave are (in my opinion) a bit too niche, but perhaps there are other cases which would be very valuable for us (e.g. corporate setting, perhaps some calendar has everyone's meetings in it, so that people can know what is happening in the company - but they don't want to be | 09:02 |
| chriadam | notified of those meetings which they're not explicitly invited to, etc) | 09:02 |
| chriadam | I will add a comment ot the PR, and ask MartinS to take a look | 09:02 |
| dcaliste | Ok thanks. | 09:03 |
| chriadam | done, and poked Martin internally also. | 09:05 |
| dcaliste | Besides, I don't have anything to discuss anymore as far as I remember. Thank you for the comments. | 09:05 |
| chriadam | thank you as always! | 09:05 |
| chriadam | ok quick summary | 09:05 |
| chriadam | qtbase PR - pekka to take that forward once he is satisfied | 09:05 |
| chriadam | buteo-syncfw PR - account / profile disablement etc - I definitely think that's valuable/useful (not waking up the plugin process for a disabled account, will save battery etc). needs review, would like flypig to check that one also. | 09:06 |
| chriadam | caldav PR - this one LGTM from brief check, but need to test on device etc since dcaliste doesn't have 4.x.x device ot test with, with new kcalcore | 09:07 |
| chriadam | mkcal PR - for disabling notebook alarms - I've poked MartinS and pvuorela about this one, can discuss further next week etc. | 09:07 |
| chriadam | QMF PRs - chriadam to start looking into unit tests etc upstream with qt6 | 09:07 |
| chriadam | . | 09:07 |
| chriadam | did I miss anything? | 09:08 |
| dcaliste | No that's it I think. Thanks for the summary. | 09:11 |
| chriadam | great. thank you for your effort and help as always! | 09:13 |
| chriadam | your comments and review on the google calendar sync stuff recently in particular has been extremely helpful also, thanks very much for that | 09:13 |
| dcaliste | Thinking about my testing issue, I may play with the .so number of mKCal to have two versions at the same time on device... | 09:13 |
| chriadam | might work, true. I don't recall which version the kcalcore upgrade is in, so not sure what the ETA for public release of that is... | 09:14 |
| chriadam | hopefully not too much longer.. | 09:14 |
| dcaliste | You're welcome. That's also interesting to see how it's done in another plugin. | 09:14 |
| chriadam | anyway, thanks again, and I hope you have a great week! | 09:14 |
| * chriadam -> home, gnight | 09:14 | |
| dcaliste | Have a good week too. | 09:14 |
| *** JvD_ is now known as JvD | 09:43 | |
| *** vilpan is now known as Guest82838 | 15:41 | |
| *** Guest82838 is now known as vilpan | 15:41 | |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!