Tuesday, 2021-02-02

*** zbenjamin is now known as Guest7205602:14
*** zbenjamin_ is now known as zbenjamin02:14
*** frinring_ is now known as frinring04:55
dcalisteHello chriadam, how are you ?07:54
chriadamhi dcaliste, I'm well thanks07:54
chriadamsorry I wasn't at the meeting last week - it was a public holiday here in Australia07:54
chriadamI sent an email on Monday but it bounced, and I only noticed on Wednesday07:54
chriadammy apologies07:54
chriadamhow has your week been?07:55
dcalisteNo problem, I've known better weeks than the last two though ! Anyway, let say it globally fine.07:55
chriadamuh oh07:55
chriadamwell, hopefully the next weeks are better than the last ones07:56
dcalisteIndeed, let start with the Qt bug : https://git.sailfishos.org/mer-core/qtbase/merge_requests/7507:56
dcalisteDo you think the fact that I read the latest version to get inspiried is a break of copyright ?07:57
chriadamI don't know07:57
chriadamI am not a lawyer07:58
chriadamI assume not07:58
dcalisteThe output is totally different than the latest version since many things and even variable names have changed.07:58
chriadamyeah, I think it's probably fine.  I will poke someone internally first, though07:59
dcalisteSo the patch is totally different in term of modifications than the equivalent one based on latest.07:59
dcalisteWould be great to fix in our Qt version, because it potentially creates a lot of places with issues.07:59
chriadamyeah, I definitely agree07:59
dcalisteIf 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
chriadamthanks.  shouldn't be necessary, but will check internally just to be sure08:01
dcalisteOk, great, thank you.08:01
dcalisteMaybe we can chat a little on https://git.sailfishos.org/mer-core/buteo-syncfw/merge_requests/59 if you don't mind ?08:02
dcalisteThere 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
dcalisteThe latter avoids to start a Buteo sync process and abort it immediately because the plugin detects that the account is disabled.08:04
dcalisteIt'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
dcalisteAdditionnally 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
chriadamyes I definitely agree that it's a useful and worthwhile change08:05
chriadamI had a few comments on that one, by the looks, already - not sure if those are still valid or not08:06
dcalisteThe 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
dcalisteYes, I would like to discuss a bit your comments.08:06
chriadamsounds good08:07
dcalisteOne issue I noticed in the restructuration is that we are creating one profile per service, so several profiles for one account.08:07
dcalisteWhile 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
dcalisteThus it is returning the profile name for the newly created profile.08:08
dcalisteThis does not work anymore when creating several profiles for one account.08:08
dcalisteTo conclude, I cannot change much this and particularly the function name because it's part of the (unused) msyncd DBus API.08:09
chriadamI 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
chriadamwell, probably too much effort currently, many places which have various assumptions08:13
chriadamI'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
dcalisteYes, I'm afraid many things are built on top of one profile per service.08:20
dcalisteBut 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
chriadamyes, many things currently08:21
dcalisteAnyway, I'll follow the route of adding a comment in the Buteo patch explaining how it is at the moment.08:22
dcalisteLast week, I submitted a MR in CalDAV plugin : https://git.sailfishos.org/mer-core/buteo-sync-plugin-caldav/merge_requests/7608:23
dcalisteI explained in the MR description the purpose of the commits.08:24
chriadamI worry about the non-transient error case08:24
dcalisteTo summarize, some commits are to fix minor errors, like the modification date being changed by setting the error flags on incidence.08:24
chriadamdo 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
dcalisteAt the moment we always retry.08:25
dcalisteAnd the failure is always logged and ignored.08:25
chriadamI guess it's not much overhead in terms of data / battery... I wonder how many "broken" events are "normal"08:27
chriadamone question: there is a removal of an "mStorage->save()" which had comment "PUT requests are finished, save the updated etags in storage" or similar08:30
chriadamI guess this is answered in a commit message somewhere08:30
chriadambut just wondering why that one can be removed08:30
dcalisteYes, it was added when we were not fault tolerant to ensure that etags were written before any error will stop the process.08:31
dcalisteBut 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
chriadamah right08:32
dcalisteSo 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
chriadamI see, because otherwise some network error could occur in a request and we would abort the sync cycle on that08:32
chriadamthe commit "don't list again incidences for modifications" -- can you briefly explain what that's doing / what the assumptions are?08:33
chriadama lot of "delta determination" code seemed to be removed, wondering how that was able to be done08:34
dcalisteYes, 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
dcalistethen 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
dcalisteSimple enough.08:37
dcalisteBut looking at what is returned by these functions, I found a quicker way to do the same.08:38
dcalisteIndeed, the first one list all incidences. And the modification listing is just a subset of this first listing.08:38
chriadamah, right, of course08:39
dcalisteAnd the subset is done with a simple rule : create < sync time && last modified > sync time.08:40
dcalisteThis rule we can easily apply it in the delta function itself, no need to ping the database again.08:41
chriadamyes, I agree08:41
dcalisteAnd 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
chriadamand I guess that locally-deleted incidences aren't returned in that first list08:41
dcalisteWe can bucket all incidences in the same logic.08:41
chriadamyeah.  I agree, definitely a nice change.08:42
dcalisteYes locally deleted are not returned.08:42
chriadamthank you for the explanation :-)08:42
dcalisteWe need this specific call for deleted.08:42
flypigI also have a query about this PR if I may?08:46
dcalisteThe 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
flypigThe final change is to retrigger the failed incidences. I was wondering why not just update the lastModified values for the failures.08:47
dcalisteBecause 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
dcalisteMy first implementation was to modified the last modification though.08:49
flypigOkay; 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
dcalisteBut I found it more concise by testing the flag on the next sync.08:49
flypigThanks. 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
chriadamwere 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 that08:52
chriadamI still need to look into the upstream QMF patches in more detail08:52
chriadamI 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 that08:53
dcalisteOk, great.08:55
dcalisteI've just started to implement a way to avoid alarms on some notebook.08:57
dcalisteThe rational is explained in https://git.sailfishos.org/mer-core/mkcal/merge_requests/5408:58
dcalisteIt's still WIP at the moment, I need to figure out where to add it in the UI, and how.08:59
dcalisteDo you think it's a valuable change or it's too specific ?08:59
chriadamI 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 be09:02
chriadamnotified of those meetings which they're not explicitly invited to, etc)09:02
chriadamI will add a comment ot the PR, and ask MartinS to take a look09:02
dcalisteOk thanks.09:03
chriadamdone, and poked Martin internally also.09:05
dcalisteBesides, I don't have anything to discuss anymore as far as I remember. Thank you for the comments.09:05
chriadamthank you as always!09:05
chriadamok quick summary09:05
chriadamqtbase PR - pekka to take that forward once he is satisfied09:05
chriadambuteo-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
chriadamcaldav 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 kcalcore09:07
chriadammkcal PR - for disabling notebook alarms - I've poked MartinS and pvuorela about this one, can discuss further next week etc.09:07
chriadamQMF PRs - chriadam to start looking into unit tests etc upstream with qt609:07
chriadamdid I miss anything?09:08
dcalisteNo that's it I think. Thanks for the summary.09:11
chriadamgreat.  thank you for your effort and help as always!09:13
chriadamyour comments and review on the google calendar sync stuff recently in particular has been extremely helpful also, thanks very much for that09:13
dcalisteThinking 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
chriadammight 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
chriadamhopefully not too much longer..09:14
dcalisteYou're welcome. That's also interesting to see how it's done in another plugin.09:14
chriadamanyway, thanks again, and I hope you have a great week!09:14
* chriadam -> home, gnight09:14
dcalisteHave a good week too.09:14
*** JvD_ is now known as JvD09:43
*** vilpan is now known as Guest8283815:41
*** Guest82838 is now known as vilpan15:41

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