Tuesday, 2019-05-21

*** zbenjamin_ is now known as zbenjamin01:18
*** frinring_ is now known as frinring04:53
dcalisteHello chriadam, how are you ?06:48
dcalisteGood morning jpetrell.06:48
chriadamdcaliste: Hi :-)  I'm well thanks, how are you?06:48
dcalisteFine thanks. From last meeting, I've added the mStorage->save() in CalDAV plugin after the first upload part.06:49
chriadamThanks, I saw the PR had been updated but haven't had a chance to test the updated PR yet06:51
dcalisteYes, sorry, did it just yesterday afternoon.06:51
chriadamIf e.g. the device were to spontaneously reboot just after that save() occurs, could that cause any issues for the next sync?  or would everything proceed normally?06:51
dcalisteI've added a comment explaining when the sync happens:06:52
dcalisteIt is just after a notebook sync agent signal finishing upload. At that moment, only the etag values have been updated in memory.06:53
dcalisteSo saving at that time will only update the incidences that have been successfully uploaded.06:53
dcalisteThe writing of the downloaded incidences is done _only_ when _all_ the sync agent have successfully finished, as before.06:53
dcalisteIn the case you describe, the database should be fine, exactly as it was before the sync, with only the etag updated for the incidences that have been uploaded with success.06:54
dcalisteThe sync time is not updated.06:54
chriadamand that doesn't affect the delta (i.e. during the next sync, we would still downsync the appropriate events from the server)?06:55
dcalisteSo next time it will sync, the delta calculation will see the same incidence to be downloaded.06:55
dcalisteBecause the sync time is not updated.06:55
dcalisteFor the notebook, I mean.06:55
chriadamI will test and hopefully merge/tag this week06:57
dcalisteGreat, thanks.06:57
chriadamI saw also that your kcalcore work led you to an issue in qtbase regarding how timezone detection works?06:57
dcalisteFrom last week, I've also finished packaging latest upstream kcalcore: https://git.merproject.org/dcaliste/kf5-calendarcore06:58
dcalisteYes for the bug in Qtbase.06:58
dcalisteThe current code follow only one symlink, while in Sailfish we are using two symlinks to point to the right time zone.06:58
dcalisteBefore, it was not an issue because the TimeZone part of QDateTime was not used anywhere.06:59
dcalisteSince it was delegated to KDateTime.06:59
chriadamah, right06:59
dcalisteBut with modern kcalcore, the time zone of QDateTime is used instead but wrongly detected by Qt.06:59
dcalisteThe fix is simple: allow to follow more than one symlink…06:59
dcalisteAfter that, tests in mkcal are working much better.07:00
dcalisteBut these tests in mkcal lead me to another issue in mkcal itself: the storage of all day event and how this is stored…07:01
dcalisteIn fact it is not stored at all, and it is using an heuristic to decide if one event is all day or not.07:01
dcalisteAnd this heuristic is very fragile in my opinion.07:01
chriadamwhat is the heuristic?  if the timezone is ClockTime and the time is 0,0,0?07:01
chriadamyes mkcal leaves a lot to be desired :/07:02
dcalisteSee https://git.merproject.org/mer-core/mkcal/merge_requests/1707:02
dcalisteYes, that's the heuristic indeed.07:02
dcalisteBut it's written no where and if someone do setDtStart(QDateTime(QDate(2019, 05, 21), Qt::UTC); setAllDay(true);07:02
dcalisteIt will not work.07:03
chriadamdefinitely fragile.  seems like adding a new column (isAllDay) makes more sense.07:03
dcalisteI've found that there are two column to save date time, One for time zone time and one for local. If the time zone is provided, the second is ignored but saved anyway, and vice versa.07:04
dcalisteSo we can use the unused but saved column to read the proper "local" time and use the heuristic.07:04
dcalisteIn https://git.merproject.org/mer-core/mkcal/merge_requests/17 I'm doing this, and it's working great from mkcal tests.07:04
chriadamI'm reading that PR but my brain isn't working at the moment07:05
dcalisteThis bring not changes to the database layout and is backward compatible with existing code and saved events, as far as I know.07:05
chriadamif the timezone is empty, it reads the date tiem and sets its timespec to clocktime07:06
chriadamotherwise, it reads the date time and sets its timespec to the appropriate timezone07:06
dcalisteIn case of empty timezone, it's the old behaviour. No changes here.07:06
dcalisteLet me describe here the two cases:07:06
dcaliste- no time zone is provided when saving, then local time is assumed and saved in column 5 and 6 and no time zone in column 707:07
dcaliste- a time zone is given, then this time is stored in column 5 and time zone is column 7. In addition, the time _without_ timezone is stored in column 6. This is unchanged current behaviour.07:08
dcalisteWhen reading:07:08
dcaliste- no time zone, time is read from column 6 and considered as clock time / local time. Then all day event is decided from time() == Qtime()07:09
dcaliste- a time zone exists, time is read from column 5 and converted in proper time zone,07:09
dcalisteThe PR modify the last line by changing this:07:10
dcaliste- before PR, the time zone time from column 5 is converted into local time (adding thus a shift) and time() == QTime() is checked.07:10
dcaliste- after PR, the local time from column 6 is used instead to do the time() == QTime() test, instead of converting the time from column 5.07:11
dcalisteThis is more consistent with the way it is saved, (see my second dash above in the save part).07:11
dcalisteI understand it is very convoluted and it takes me one week to figure this strategy out to solve the mkcal test issue.07:12
chriadamwhen you say "adding thus a shift" can you explain that a bit?  I get that converting a time from timezone to localtime will cause the converted time to be different/shifted.  but how is that different to the time that woudl read from column 6?07:12
dcalistemy zone is UTC+2 in this example07:13
dcalisteI'm storing QDateTime(today, Qt::UTC)07:14
dcalisteSorry I'm storing QDateTime(origin, QTime(12,00), Qt::UTC), it's simpler then for number in seconds.07:15
dcalisteSo coulmn 5 is 12h, and column 7 is "UTC"07:16
dcalistecolumn 6 is computed from d->mOriginTime.time().secsTo(dt.time()) (see sqliteformat.cpp)07:16
dcalisteSo column 6 contains 12h also.07:17
dcalisteWhen reading:07:17
dcalistewe have a time zone, so we read from column 5 and apply time zone. We get 12h07:17
dcalisteBefore PR, we have dateTime.toLocalZone() to test for full day, so we're getting 12h + 2 (I'm locally UTC+2), and the test is failing and my UTC saved event is not detected as all day.07:19
dcalisteOf course in this example, I've used 12h to have a figure, but you can do the same with 0 and see that changing to local zone will mess up the detection.07:20
dcalisteAfter this PR, we read time from column 6 _only_ for all day detection. Reading will gives here 12h and all day detection will works.07:21
dcalisteI've added tests in mkcal to demonstrate this:07:21
dcaliste- adding event in local tim zone at 00:00 works (old cases).07:21
dcaliste- adding event in UTC at 00:00 works when not being in UTC time zone (new case).07:22
chriadamright.  and happily the detection now doesn't change based on which timezone you (later on) set your device to.  ok.07:22
dcaliste- adding event in time zone A at 00:00 works when being in time zone B (new case).07:22
dcalisteIndeed: as said in the MR message, this was working before, because jolla-calendar has the right taste to save all ull day events in local time only, while normal events are saved in their respective time zone.07:23
dcalisteBut in my opinion, this is a fragile assumption and not very API friendly.07:23
chriadamI agree.  one thing: would it incorrectly detect as all-day an event which was genuinely saved as happening at 00:00 clocktime (but is not all day) e.g. new year celebration fireworks happen at 00:00 on 1st Jan.  I guess that would have an endDt so wouldn't be considered date only?07:24
dcalisteTo be considered date only, the end date time should be date only also.07:25
dcalisteThis code already exists like that and is uncahnged.07:25
chriadamok, sounds good.  thanks for the explanation.  it's nice that you found a solution which doesn't require changes to API or schema.07:25
dcalisteI'm not 100% sure, but from all existing cases (like saving event in UTC but with a shifted time, so local is 00:00) I could figure out, it should have an unchanged behaviour.07:27
dcalisteOf course, saving local time 00:00 was working and continue to work…07:27
chriadamat some point we just have to trust the unit tests and manual testing.  I wonder if pvuorela has any thoughts about whether we should merge it before the upcoming release branches, or not...07:28
dcalisteAh, now there is one change (sorry for remembering it only now…):07:28
dcalisteSaved all day event in UTC before was returning wrongly allDay() == false.07:29
dcalisteAnd the consequence of this is that event of 1 day duration was returned as a 2 day duration.07:29
dcalisteSo the test in mkcal was testing (dateEnd() == startDate.addDays(1))07:30
dcalisteWhich is wrong for a all day event. But it was written like that.07:30
dcalisteCorrecting this behaviour may break any workaround that may happen in users of mkcal…07:31
dcalisteThis is restricted t all day event saved in UTC though07:31
chriadamn-q-p-c should be fixed.  most likely something similar might be in the google calendar plugin also, but we can check that07:31
dcalisteUsual all day events saved in local time are not affected and dateEnd() == startDate() indeed.07:31
dcalisteSo I'm not sure there is a way to detect this wrong behaviour for UTC events and circumvent the bug.07:32
dcalisteI'm more thinking than no one ever saved an all day event in UTC…07:32
chriadamwell, most likely it happened all the time and people just put up with the buggy thing in their calendar.07:33
dcalisteNo really, as said, I've noticed that jolla-calendar is saving all day events in local time always.07:33
dcalisteAnd down synced event are done the same.07:33
chriadamok, well, that's "good" I guess ;-)07:34
dcalisteWith the special FLOATING_DATE flag as time zone in sqliteormat.cpp.07:34
dcalisteEverything is holding because every piece of code is following the assumption that all day event should provide start date and end date QDateTime in LocalTime only.07:34
dcalisteBut nothing in the code is enforcing this…07:35
chriadamdefinitely not robust07:35
dcalisteYes, thus the MR ;) But I'm afraid of unforseen consequences, if any…07:36
chriadamindeed.  thanks for your work and explanations.  I will discuss with pvuorela.07:36
dcalisteThank you.07:36
dcalisteBesides, testing of upstream kcalcore is progressing well.07:37
chriadamOne issue is that - if commercial partners use kcalcore (or n-q-p-c) directly to write events, if they don't follow the "floating date" pattern their code will cause broken events.07:37
chriadamso we're probably best off getting this fix in sooner rather than later07:37
chriadamanyway, will discuss internally :-)  thanks07:37
dcalisteYes, that's my thought also.07:37
chriadamI saw some activity on the jolla-email PR - what's the current status of that?  has jpetrell approved that one?07:38
dcalisteI don't see why I can't do setDtStart(QDateTime(today, Qt::UTC);07:38
dcalistejpetrell has done one founded remark on label that should wrap instead of fading.07:38
dcalisteI've corrected this.07:39
chriadamhopefully jpetrell can approve that one now then - thanks07:40
jpetrellwill approve07:41
chriadamI will merge and tag on Thursday (I am out of office tomorrow)07:41
chriadamthanks jpetrell.  thanks dcaliste!07:41
jpetrellsorry wait will check for screenshots07:41
jpetrellwill approve soon :P07:41
jpetrellah looks good, just approved again07:42
dcalisteI've added the screenshots. I can adjust things if necessary.07:42
jpetrellthe layouts look nice and clean :)07:43
jpetrellexciting to finally have support for email signing07:43
dcalisteGreat :-)07:45
chriadamok, am I forgetting anything?  were there other action points from last meeting which I forgot to do?07:46
dcalisteTo finish with kcalcore transition: mkcal is almost ready (see my kf5 branch). I've got one last bug to correct in recurrence rule saving for all day event (again). After that, I'll update nemo-qml-plugin and caldav plugin.07:46
pvuorelalate here, but yea, that mkcal allday storing has been so bad. as well the special rule for allday event +1 offset etc.07:46
chriadamdcaliste: what are the licensing considerations with the updated kcalcore?  (can't remember if I asked this already)07:48
dcalisteGood point, looking at it…07:48
chriadamwell, LGPLv2 for library linkage I hope07:49
pvuoreladcaliste: not L?07:49
dcalisteSorry, LGPLv2…07:49
dcalistepvuorela: hopefully my MR for mkcal can solve the all day mess for events. I've added tests for all day stored as local, UTC or different time zone. I think it's covering all cases. Tell me if you find a flaw in this.07:51
dcalisteThere is an obvious one, that I keep for later: the todo case.07:51
dcalisteIf you look at the save code (in modifyCompionents() func) and the read code (in selectComponents() func), they are not in accordance at all.07:52
dcalisteBut that's for later in my opinion. There is no tests currently for todos anyway.07:52
dcalisteIn upgrading mkcal for latest kcalcore, it is also dropping uuid dependency (in Qt now) and I've added a commit to replace kDebug() by qCDebug() with proper logging category.07:54
chriadamour support for ToDos is currently extremely limited, not surprised that there isn't much coverage of those codepaths07:55
pvuoreladcaliste: sure, i'll check as soon as i have some capacity and enough bravery for time details :)07:55
dcalistepvuorela: thank you very much. It's not in a hurry.07:55
chriadamany other agenda points?  also, what are the action points for me?  1) merge+tag gpg PRs, 2) test updated caldav PR, 3) review mkcal/kcalore/qtbase PRs?07:57
dcalistechriadam: yes, that's it. Thanks.07:58
dcalistepvuorela: can I ask what do you mean in your latest comment for qtbase MR ?07:58
pvuoreladcaliste: i mean /etc/localtime link to /foo/bar, and /foo/bar link to /etc/localtime08:00
pvuoreladcaliste: would try forever resolving between those two?08:02
dcalisteOk, I see, the symlink loop. Yes, it's required to stop the loop after some following indeed…08:07
dcalisteI'm going to amend the commit.08:07
dcalistechriadam, pvuorela: one last point to look at maybe this week, if you have time is this PR: https://bitbucket.org/jolla/ui-jolla-calendar/pull-requests/22808:19
dcalisteThe one about the synchronisation of modified account only.08:19
chriadamoh, yes.  I had forgotten.  I'm fine with that one, if pvuorela agrees, I'll ask bree if she has time to give it a thorough testing.08:20
dcalistechriadam: thanks a lot.08:20
chriadamthank you :-)08:21
chriadamI have to head off for the night - thanks again for your time and effort, have a great week!08:21
dcalistechriadam: sure, have a good night and a great week ;)08:22
*** axeKirves is now known as kirvesAxe14:04
*** gigetoo_ is now known as gigetoo14:06
*** Almindor_ is now known as Almindor14:23
*** Plnt_ is now known as Plnt15:19
*** leinir_ is now known as leinir18:50
*** BitEvil is now known as SpEEdEVil22:01

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