| *** zbenjamin_ is now known as zbenjamin | 01:18 | |
| *** frinring_ is now known as frinring | 04:53 | |
| dcaliste | Hello chriadam, how are you ? | 06:48 |
|---|---|---|
| dcaliste | Good morning jpetrell. | 06:48 |
| chriadam | dcaliste: Hi :-) I'm well thanks, how are you? | 06:48 |
| dcaliste | Fine thanks. From last meeting, I've added the mStorage->save() in CalDAV plugin after the first upload part. | 06:49 |
| chriadam | Thanks, I saw the PR had been updated but haven't had a chance to test the updated PR yet | 06:51 |
| dcaliste | Yes, sorry, did it just yesterday afternoon. | 06:51 |
| chriadam | If 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 |
| dcaliste | I've added a comment explaining when the sync happens: | 06:52 |
| dcaliste | It is just after a notebook sync agent signal finishing upload. At that moment, only the etag values have been updated in memory. | 06:53 |
| dcaliste | So saving at that time will only update the incidences that have been successfully uploaded. | 06:53 |
| dcaliste | The writing of the downloaded incidences is done _only_ when _all_ the sync agent have successfully finished, as before. | 06:53 |
| chriadam | great | 06:53 |
| dcaliste | In 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 |
| dcaliste | The sync time is not updated. | 06:54 |
| chriadam | and that doesn't affect the delta (i.e. during the next sync, we would still downsync the appropriate events from the server)? | 06:55 |
| dcaliste | So next time it will sync, the delta calculation will see the same incidence to be downloaded. | 06:55 |
| dcaliste | Because the sync time is not updated. | 06:55 |
| dcaliste | For the notebook, I mean. | 06:55 |
| chriadam | cool | 06:57 |
| chriadam | thanks | 06:57 |
| chriadam | I will test and hopefully merge/tag this week | 06:57 |
| dcaliste | Great, thanks. | 06:57 |
| chriadam | I saw also that your kcalcore work led you to an issue in qtbase regarding how timezone detection works? | 06:57 |
| dcaliste | From last week, I've also finished packaging latest upstream kcalcore: https://git.merproject.org/dcaliste/kf5-calendarcore | 06:58 |
| dcaliste | Yes for the bug in Qtbase. | 06:58 |
| dcaliste | The current code follow only one symlink, while in Sailfish we are using two symlinks to point to the right time zone. | 06:58 |
| dcaliste | Before, it was not an issue because the TimeZone part of QDateTime was not used anywhere. | 06:59 |
| dcaliste | Since it was delegated to KDateTime. | 06:59 |
| chriadam | ah, right | 06:59 |
| dcaliste | But with modern kcalcore, the time zone of QDateTime is used instead but wrongly detected by Qt. | 06:59 |
| dcaliste | The fix is simple: allow to follow more than one symlink… | 06:59 |
| chriadam | indeed | 07:00 |
| dcaliste | After that, tests in mkcal are working much better. | 07:00 |
| dcaliste | But 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 |
| dcaliste | In 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 |
| dcaliste | And this heuristic is very fragile in my opinion. | 07:01 |
| chriadam | what is the heuristic? if the timezone is ClockTime and the time is 0,0,0? | 07:01 |
| chriadam | yes mkcal leaves a lot to be desired :/ | 07:02 |
| dcaliste | See https://git.merproject.org/mer-core/mkcal/merge_requests/17 | 07:02 |
| dcaliste | Yes, that's the heuristic indeed. | 07:02 |
| dcaliste | But it's written no where and if someone do setDtStart(QDateTime(QDate(2019, 05, 21), Qt::UTC); setAllDay(true); | 07:02 |
| dcaliste | It will not work. | 07:03 |
| chriadam | definitely fragile. seems like adding a new column (isAllDay) makes more sense. | 07:03 |
| dcaliste | I'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 |
| dcaliste | So we can use the unused but saved column to read the proper "local" time and use the heuristic. | 07:04 |
| dcaliste | In https://git.merproject.org/mer-core/mkcal/merge_requests/17 I'm doing this, and it's working great from mkcal tests. | 07:04 |
| chriadam | I'm reading that PR but my brain isn't working at the moment | 07:05 |
| dcaliste | This bring not changes to the database layout and is backward compatible with existing code and saved events, as far as I know. | 07:05 |
| chriadam | if the timezone is empty, it reads the date tiem and sets its timespec to clocktime | 07:06 |
| chriadam | otherwise, it reads the date time and sets its timespec to the appropriate timezone | 07:06 |
| dcaliste | In case of empty timezone, it's the old behaviour. No changes here. | 07:06 |
| dcaliste | Let 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 7 | 07: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 |
| dcaliste | When 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 |
| dcaliste | The 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 |
| dcaliste | This is more consistent with the way it is saved, (see my second dash above in the save part). | 07:11 |
| dcaliste | I understand it is very convoluted and it takes me one week to figure this strategy out to solve the mkcal test issue. | 07:12 |
| chriadam | when 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 |
| dcaliste | Ok: | 07:13 |
| dcaliste | my zone is UTC+2 in this example | 07:13 |
| dcaliste | I'm storing QDateTime(today, Qt::UTC) | 07:14 |
| dcaliste | Sorry I'm storing QDateTime(origin, QTime(12,00), Qt::UTC), it's simpler then for number in seconds. | 07:15 |
| dcaliste | So coulmn 5 is 12h, and column 7 is "UTC" | 07:16 |
| chriadam | ok | 07:16 |
| dcaliste | column 6 is computed from d->mOriginTime.time().secsTo(dt.time()) (see sqliteformat.cpp) | 07:16 |
| dcaliste | So column 6 contains 12h also. | 07:17 |
| dcaliste | When reading: | 07:17 |
| chriadam | ok | 07:17 |
| dcaliste | we have a time zone, so we read from column 5 and apply time zone. We get 12h | 07:17 |
| dcaliste | Before 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 |
| dcaliste | Of 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 |
| dcaliste | After 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 |
| dcaliste | I'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 |
| chriadam | right. 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 |
| chriadam | great | 07:22 |
| dcaliste | Indeed: 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 |
| dcaliste | But in my opinion, this is a fragile assumption and not very API friendly. | 07:23 |
| chriadam | I 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 |
| dcaliste | Exact. | 07:25 |
| dcaliste | To be considered date only, the end date time should be date only also. | 07:25 |
| dcaliste | This code already exists like that and is uncahnged. | 07:25 |
| chriadam | ok, 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 |
| dcaliste | I'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 |
| dcaliste | Of course, saving local time 00:00 was working and continue to work… | 07:27 |
| chriadam | at 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 |
| dcaliste | Ah, now there is one change (sorry for remembering it only now…): | 07:28 |
| dcaliste | Saved all day event in UTC before was returning wrongly allDay() == false. | 07:29 |
| dcaliste | And the consequence of this is that event of 1 day duration was returned as a 2 day duration. | 07:29 |
| dcaliste | So the test in mkcal was testing (dateEnd() == startDate.addDays(1)) | 07:30 |
| dcaliste | Which is wrong for a all day event. But it was written like that. | 07:30 |
| dcaliste | Correcting this behaviour may break any workaround that may happen in users of mkcal… | 07:31 |
| chriadam | indeed | 07:31 |
| dcaliste | This is restricted t all day event saved in UTC though | 07:31 |
| chriadam | n-q-p-c should be fixed. most likely something similar might be in the google calendar plugin also, but we can check that | 07:31 |
| dcaliste | Usual all day events saved in local time are not affected and dateEnd() == startDate() indeed. | 07:31 |
| dcaliste | So I'm not sure there is a way to detect this wrong behaviour for UTC events and circumvent the bug. | 07:32 |
| dcaliste | I'm more thinking than no one ever saved an all day event in UTC… | 07:32 |
| chriadam | well, most likely it happened all the time and people just put up with the buggy thing in their calendar. | 07:33 |
| dcaliste | No really, as said, I've noticed that jolla-calendar is saving all day events in local time always. | 07:33 |
| dcaliste | And down synced event are done the same. | 07:33 |
| chriadam | ah! | 07:33 |
| chriadam | ok, well, that's "good" I guess ;-) | 07:34 |
| dcaliste | With the special FLOATING_DATE flag as time zone in sqliteormat.cpp. | 07:34 |
| dcaliste | Everything 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 |
| dcaliste | But nothing in the code is enforcing this… | 07:35 |
| chriadam | definitely not robust | 07:35 |
| dcaliste | Yes, thus the MR ;) But I'm afraid of unforseen consequences, if any… | 07:36 |
| chriadam | indeed. thanks for your work and explanations. I will discuss with pvuorela. | 07:36 |
| dcaliste | Thank you. | 07:36 |
| dcaliste | Besides, testing of upstream kcalcore is progressing well. | 07:37 |
| chriadam | One 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 |
| chriadam | so we're probably best off getting this fix in sooner rather than later | 07:37 |
| chriadam | anyway, will discuss internally :-) thanks | 07:37 |
| dcaliste | Yes, that's my thought also. | 07:37 |
| chriadam | I saw some activity on the jolla-email PR - what's the current status of that? has jpetrell approved that one? | 07:38 |
| dcaliste | I don't see why I can't do setDtStart(QDateTime(today, Qt::UTC); | 07:38 |
| dcaliste | jpetrell has done one founded remark on label that should wrap instead of fading. | 07:38 |
| dcaliste | I've corrected this. | 07:39 |
| chriadam | hopefully jpetrell can approve that one now then - thanks | 07:40 |
| jpetrell | will approve | 07:41 |
| chriadam | \o/ | 07:41 |
| chriadam | I will merge and tag on Thursday (I am out of office tomorrow) | 07:41 |
| chriadam | thanks jpetrell. thanks dcaliste! | 07:41 |
| jpetrell | sorry wait will check for screenshots | 07:41 |
| jpetrell | will approve soon :P | 07:41 |
| jpetrell | ah looks good, just approved again | 07:42 |
| dcaliste | I've added the screenshots. I can adjust things if necessary. | 07:42 |
| jpetrell | the layouts look nice and clean :) | 07:43 |
| jpetrell | exciting to finally have support for email signing | 07:43 |
| chriadam | definitely | 07:44 |
| dcaliste | Great :-) | 07:45 |
| chriadam | ok, am I forgetting anything? were there other action points from last meeting which I forgot to do? | 07:46 |
| dcaliste | To 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 |
| pvuorela | late here, but yea, that mkcal allday storing has been so bad. as well the special rule for allday event +1 offset etc. | 07:46 |
| chriadam | dcaliste: what are the licensing considerations with the updated kcalcore? (can't remember if I asked this already) | 07:48 |
| dcaliste | Good point, looking at it… | 07:48 |
| dcaliste | GPLv2 | 07:48 |
| chriadam | great | 07:49 |
| chriadam | well, LGPLv2 for library linkage I hope | 07:49 |
| pvuorela | dcaliste: not L? | 07:49 |
| dcaliste | Sorry, LGPLv2… | 07:49 |
| chriadam | great | 07:49 |
| dcaliste | pvuorela: 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 |
| dcaliste | There is an obvious one, that I keep for later: the todo case. | 07:51 |
| dcaliste | If 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 |
| dcaliste | But that's for later in my opinion. There is no tests currently for todos anyway. | 07:52 |
| dcaliste | In 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 |
| chriadam | our support for ToDos is currently extremely limited, not surprised that there isn't much coverage of those codepaths | 07:55 |
| pvuorela | dcaliste: sure, i'll check as soon as i have some capacity and enough bravery for time details :) | 07:55 |
| dcaliste | pvuorela: thank you very much. It's not in a hurry. | 07:55 |
| chriadam | any 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 |
| dcaliste | chriadam: yes, that's it. Thanks. | 07:58 |
| dcaliste | pvuorela: can I ask what do you mean in your latest comment for qtbase MR ? | 07:58 |
| pvuorela | dcaliste: i mean /etc/localtime link to /foo/bar, and /foo/bar link to /etc/localtime | 08:00 |
| pvuorela | dcaliste: would try forever resolving between those two? | 08:02 |
| dcaliste | Ok, I see, the symlink loop. Yes, it's required to stop the loop after some following indeed… | 08:07 |
| dcaliste | I'm going to amend the commit. | 08:07 |
| dcaliste | chriadam, 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/228 | 08:19 |
| dcaliste | The one about the synchronisation of modified account only. | 08:19 |
| chriadam | oh, 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 |
| dcaliste | chriadam: thanks a lot. | 08:20 |
| chriadam | thank you :-) | 08:21 |
| chriadam | I have to head off for the night - thanks again for your time and effort, have a great week! | 08:21 |
| dcaliste | chriadam: sure, have a good night and a great week ;) | 08:22 |
| *** axeKirves is now known as kirvesAxe | 14:04 | |
| *** gigetoo_ is now known as gigetoo | 14:06 | |
| *** Almindor_ is now known as Almindor | 14:23 | |
| *** Plnt_ is now known as Plnt | 15:19 | |
| *** leinir_ is now known as leinir | 18:50 | |
| *** BitEvil is now known as SpEEdEVil | 22:01 | |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!