Tuesday, 2020-10-06

*** zbenjamin_ is now known as zbenjamin01:58
*** nyov is now known as Guest3974903:29
*** JvD_ is now known as JvD04:38
*** frinring_ is now known as frinring05:56
dcalisteGood afternoon chriadam.07:03
chriadamhello dcaliste :-)07:03
chriadamhow has your week been?07:03
dcalisteGreat thank you, I hope yours was fine also.07:04
chriadamyes, busy but good07:04
chriadamif my memory serves, we had the following action items open from last week's meeting:07:05
chriadam1) the caldav PR72 which I promised to review/test/merge this week07:05
chriadam2) the QMF patch which I think we should merge (pinged pvuorela about that one on the gitlab)07:06
chriadam3) the jolla-calendar PRs which flypig is discussing with MartinS and yourself07:06
chriadamthen for this week, we have new topics:07:06
chriadama) use UNTIL instead of COUNT in RRULE with VTIMEZONE kcalcore PR07:06
chriadamis there any other topics you'd like to discuss today also?07:07
dcalisteMaybe the Qt6 transition of QMF, I've updated my changes to follow the solutions used by Edward in QtPIM.07:09
dcalisteFor QSet()::toList(), I'm using QSet()::values(), and for QDateTime(QDate()), I'm using QDate()::startOfDay().07:09
dcalisteThat's the two changes done recently.07:10
chriadamcool - I've also made some progress on QtPIM/Qt6 builds.  so far I have all of the libraries building, but unit tests are still a work in progress.  once I've finished that, I need to make sure the tests still pass, of course ;-)07:10
dcalisteBesides, your list is exact.07:10
chriadamok great, let's start with (1)07:11
chriadamI haven't had a chance to look into it properly yet07:11
chriadamare there any specific parts of the code which I might find tricky to review?07:11
dcalisteThe code base changes are important. Maybe you should start by looking to the separate commits, as an overview, to understand the logical changes, then you may look to the diffs.07:13
chriadammakes sense07:13
chriadamfrom a brief look, it seems there are quite a few changes.  I'll try to digest it properly tomorrow.07:15
chriadamcan you suggest how I might best test it with my own nextcloud server?07:15
dcalisteThe biggest change in term of logical operation of the code is that now, instead of leaving processing slot to emit finish signal in notebooksyncagent, there is a requestFinished() function that _must_ be called by every slot that will emit the finish signal or not depending if some requests are still pending.07:16
dcalisteThis is because an error now should not cancel pending requests.07:17
dcalisteIf these requests are independant from the rror.07:17
dcalisteAbout testing, to check that nothing breaks, I'm afraid you'll have to go all around adding, modifying deleteing locally and remotely events, all day and recurring ones…07:18
dcalisteTo check the new functionnalities, it's very tricky because you need to trigger errors from server side, which is complex…07:19
dcalisteThe error client side are also difficult to trigger, because they would correspond to bugs in mKCal…07:20
chriadamindeed, it's the error cases I was wondering about07:20
chriadambut maybe e.g. I could set some particular event as "read-only" on server-side somehow, just prior to the upsync...07:20
dcalisteYou can do to the whole calendar in fact. That's a good point.07:21
dcalisteDo local modifications on device, change the calendar to read-only server side and sync.07:22
dcalisteThe read-only detection of the CalDAV plugin will trigger, but will inforce in in mKCal for the future modifications.07:22
dcalisteSo it should try to push anyway and should fail.07:23
dcalisteAh, well no…07:23
dcalisteThe plugin is now smart enough not to try to push local modifications to read-only calendars…07:23
dcalisteSo if you can set somehow a read-only flag per event, it may trigger the error path.07:24
dcalisteIn fact, some time ago, I thought it was not necessary to handle error cases, and I was fine with the stop-on-error behaviour because I thought it was related to network issues mainly that will solve by themselves at next sync with a better network.07:26
dcalisteBut constant report on TJC and forum make me think differently. There are bugs, on our side and on server side, that can make this stop-on-error behaviour a complete pain because the sync will not recover by itself at next sync, but at next SFOS update in the better case…07:27
dcalisteThus this MR and a better error handling, but it's difficult to trigger this code path, because it's mainly related to bug caes.07:27
*** vilpan is now known as Guest1560107:30
chriadamexperience has definitely taught us that transient errors and server side errors should be worked around as much as possible, as they are common enough to cause ongoing issues07:31
chriadammy concern is solely "how can we test the error handling codepaths sufficiently"07:31
chriadambut that's relatively minor - I definitely agree with the direction of the PR.07:31
chriadamand I will figure out a way to test it (even if I have to manually add code to simulate error conditions, within by build)07:32
chriadamwithin my build* I meant07:32
chriadamwell, hopefully I get a chance to do that in the next couple of days07:32
chriadamwas ther esomething else about (1) to discuss, or shall we move to (2)07:32
dcalisteIt's ok, we can move to (2).07:33
chriadamok, the QMF PR - I think we should just merge it as-is.  I pinged Pekka.  not sure if this one needs more discussion, let's see what Pekka says, but I assume we'll merge and tag that one this week.07:34
dcalisteOk, thank you, that's great.07:34
dcalisteLooking at the blame of this line, it was like that since the beginning of git history.07:34
chriadamdoesn't surprise me07:34
dcalisteSo it was either done on purpose, or forgotten. But at least, it was not removed because breaking something.07:35
chriadamyep.  I think it was just an oversight07:35
chriadamfor (3) I wonder if flypig would like to comment on the current status of the jolla-calendar things - I know he had several discussions with MartinS07:35
flypigIt's basically there; Martin was happy with it. I think there's just now a question raised by pvuorela concerning the wording.07:36
dcalisteHello flypig, thanks for the review and pushing this internally.07:37
flypigThanks for your explanation about the "how to fix" messages. What you wrote makes sense, so I consider that resolved.07:37
flypigdcaliste, you're welcome; I'm sorry I didn't get it all done sooner.07:37
dcalisteAbout the wording, I agree and proposed another possibility yesterday. Let see what pvuorela is thinking of it.07:38
chriadamI guess not much else to discuss on this one, then?  I wonder if you think it might get merged/tagged this week, flypig?07:39
flypigYes, it's tricky. I'm afraid I think your original wording was a bit more elegant, but I see the issue about "Web" being potentially confusing.07:39
flypigI'm happy to merge it as soon as this last question about the wording is resolved.07:40
dcalisteA bit out of topic flypig, but related to https://forum.sailfishos.org/t/calendar-bug-for-disabled-calendars-in-3-4-ea/2563/7, could you try to grep for setIsVisible() in the buteo sync plugin for exchange ?07:40
flypigYes, I saw that but didn't have time to follow it up. Yes, I'll do that, one second.07:41
dcalisteSure, dealing with it will take time, and I cannot help much there :( So of course, going further than a simple grep can happen only when you have time.07:42
flypigWell, it doesn't seem to appear anywhere.07:42
*** Guest15601 is now known as vilpan07:42
dcalisteOk, so need to find where it can come from…07:43
flypigLet me do a proper check though while you continue with chriadam and I'll let you know what I find.07:43
dcalisteThe other place where setIsVisible() is called is in the contactsd plugin, when there is an account change.07:43
dcalisteI notice an issue that I need to investigate with that respect also while trying to reproduce the behaviour with caldav and playing with notebook check and uncheck in account setting page.07:44
flypigAh, I was mistaken. It does appear in the eas code. Also here: nemo-qml-plugin-calendar/src/calendarworker.cpp (lines 626, 921).07:45
dcalisteNothing solid at the moment, but visibility did switched on without prior notice when switching on and off calendars in the setting page.07:46
dcalisteOk, so maybe two different bugs actually.07:46
chriadamsome contactsd things might have changed in devel since the qtcontacts update merge etc07:46
dcalisteThe one in calendar worker is valid (at least intentional) because it's the entry point for the calendar UI to switch visibility.07:46
chriadamnot sure07:46
dcalisteBut the one in EAS may have to be checked. Neither CalDAV pluign nor Google social sync plugin is changing the visibility.07:47
dcalisteExcept if exchange is exporting calendar visibility from server (which CalDAV is not doing for instance as far as I know), then the forum report is not a bug but a functionality ;)07:48
dcalisteIf his calendars are visible on server and this is exported by sync, then, it's normal that calendar switches on on device also.07:48
dcalisteBut maybe inconvenient ;)07:49
dcalisteAnd it would be a regression, because mKCal visibility (set by EAS) and UI visibility were two different things before, while now the UI visibility is not a QSetting anymore and is following mKCal visibility.07:49
flypigFor EAS one is in createNotebook(), which seems valid, the other in updateVisibilities(..). I should probably look into that second one then?07:50
dcalisteflypig, yes, I think you're right.07:50
dcalistechriadam, sorry, I diverged a bit. For point (3), I think it's fine. So it remains the additional new point for today, the COUNT server issue.07:51
chriadamno problem :-)07:51
dcalisteAs you mentioned in your comment on gitlab, I agree that it's fragile for server that doesnot support properly UNTIL, but since upstream is using this trick, I think we're safe here.07:51
chriadamIf upstream kcalcore has been using that change for some time, with no issues, then that certainly suggests that most servers support the UNTIL07:51
chriadamso, I am happy with that one.  I would like pvuorela or flypig to concur before I push the button, however07:52
dcalisteThey have a bigger user base than SFOS ;)07:52
chriadamone other thing to mention: will that make the proper kcalcore upgrade more difficult due to mismatching sha1 etc?07:52
dcalisteSure, it's not in a hurry, I've patched my kcalcore to have my meeting time on proper values.07:52
dcalisteAbout the migration, no problem, we're already on a fork in fact, and I'm taking care that any patch done in SFOS are propagated to upstream.07:53
chriadamsounds good to me, then07:53
dcalisteThen, the kcalcore switch will be done with a complete new repo in my opinion.07:53
chriadammakes sense to me.07:54
chriadamwe'll have to get the test team to do some very thorough regression testing when that day comes ;-)07:54
chriadamwell, I think that about covers that topic.  I will poke pvuorela about the QMF PR and about this kcalcore PR.07:56
chriadamany other topics to discuss tonight?07:56
dcalisteExact. The main impact being this QDateTime transition from somehow slightly different KDateTime API. But now, I'm feeling more confident that I understand the emaning of the new API and the various implication in the open source stack.07:56
dcalisteI'm going to rework my WIP MR in mKCal with that respect soon.07:57
dcalisteBut for today, no it's fine. I'm sorry the end time transition in kcalcore causes a regression in google adaptor. Do you need some addition eye for review ?07:57
dcalisteI've seen flypig proposing a WIP patch there.07:57
flypigSomething I noticed while looking at the Google stuff is that a single error is bringing down the entire calendar. Does that sound plausible?07:58
chriadamyes ;-)07:59
chriadamwe should fix that07:59
flypigGiven that it's being fixed elsewhere, it would be nice I thought.07:59
dcaliste(Use CalDAV ;) )07:59
chriadamare you volunteering, flypig? :-P07:59
chriadambut, yes you're right07:59
flypigWe can add that to the error message ;)07:59
dcalisteAh ah yes indeed, good quick fix.08:00
flypigHaha. I only have myself to blame chriadam.08:00
flypigJust for info regarding EAS calendar visibility, I created an internal bug about it (JB#51506). Will do the same for Google errors.08:01
chriadamdcaliste: if you have any comments about that google calendar end-date-time for all-day events thing, we're all ears08:01
flypigYes, very much so. Actually, I have a quick question about that.08:02
chriadamflypig's change LGTM but I haven't tested on device yet, and you know that stuff better than I do, now08:02
flypigAm I misreading, or does this line look odd?08:02
dcalisteI need to read the code with more attention, because ideally, setting the end date should not be necessary. In iCal parser in KCalCore, they are not saving any end date if not present, just putting a allDay flag on on event. If present, the endDate is set to read value -1 because it is inclusive in kcalcore but exclusive in iCal.08:04
dcalisteI wonder if the same reasoning can be applied here. But I've just read the modified line at the moment.08:04
chriadamflypig: you mean UPDATE_EVENT_PROPERTY_IF_REQUIRED(event, allDay, setAllDay, false, changed) ?08:05
flypigWell, that's the line, but it just seems odd that it's set to false there.08:05
dcalisteSo I need to have a better full picture to understand why end must be provided and which scheme is awaited, exclusive or inclusive.08:05
flypigYes, there's a bunch of stuff to dig into. I need to look into it more too. But your thoughts, in time, would be greatly appreciated.08:06
chriadamis it possible that some of the kcalcore changes related to this, were pushed after 3.4.0 branched?08:07
chriadam(I assume not)08:07
dcalistechriadam, no I don't think so.08:07
dcalisteThe end time transition were done much earlier than the 3.4 branching as far as I rememeber.08:07
chriadam@flypig: yes, it is odd, I would naively assume the flag should be set to true in the "if (isAllDay)" case08:08
chriadambut my brain isn't working currently, and I don't remember any of the code.08:08
dcalisteflypig, I'll look at it further in the coming days, tomorrow or the day after.08:08
chriadamtyvm dcaliste08:08
flypig+1 thanks dcaliste08:08
chriadamwe might need to merge flypig's change as a stopgap / emergency for 3.4.008:09
chriadamand do deeper investigation / fixes for the next release08:09
chriadamas I think release team is preparing the latest rc for 3.4.0 today or tomorrow08:09
dcalisteYes, I understand, sure, if it's fixing the issue, it's great already. We can work on a cleaner fix and understand the root issue better in a second period.08:10
chriadamdcaliste: does that sound ok to you, or is flypig's current change likely to cause issues?08:10
chriadamgreat, thanks.08:10
chriadamok - anything else to discuss or should we wrap up the meeting?08:11
dcalisteI don't know this part of the code. At the moment, I don't understand why not setting an end date and putting the all-day flag should not suffice as done before the patch. But it's because I don't know this part. I'm going to read further than the sole diff, but don't wait for me if it's fixing the issue already.08:12
chriadamor flypig did you have anything further to discuss?08:13
flypigNo, that's all great from my side. Thanks as always dcaliste :)08:14
dcalisteThank you for the meeting today. Let's meet together next week and discuss these issues on MRs.08:14
chriadamthank you very much08:14
chriadamhave a great week08:14
dcalisteflypig, for your fix, I think it's working because of this in mKCal/src/sqliteformat.cpp#342:10:03
dcaliste                    if (incidence->allDay()) {10:04
dcaliste                        effectiveDtEnd = event->dtEnd().addDays(1);10:04
dcaliste                    } else {10:04
dcaliste                        effectiveDtEnd = event->dtEnd();10:04
dcaliste                    }10:04
dcalisteAs you noticed, the Google sync adaptor is not setting the allDay flag on event (I don't know why), thus we go to the second part of the if here.10:04
flypigYes, I I agree, I'm just looking at that now. But the current case has !event->hasEndDate(), so that's not happening.10:04
flypigSo, should that be followed by:10:06
flypig                if (!event->hasEndDate()) {10:06
flypig                    if ((incident->hasStartDate()) && (incidence->allDay())) {10:06
flypig                        effectiveDtEnd = event->dtStart().addDays(1);10:06
flypig                    }10:06
flypig                }10:06
flypigI think the allDay() flag issue needs fixing, but it won't solve this particular problem.10:07
dcalisteNo, in fact, we don't want to save end date if the event don't have one.10:07
flypigSo, what is the correct way to store a single-day all-day event?10:07
dcalisteThere are two:10:08
dcaliste(Wait, reading the code again)10:08
dcalisteFor iCal data there are two:10:09
dcaliste- no end date and date only start time;10:09
dcaliste- date only start time and end date set to start +1 day.10:09
dcalisteThis is for iCal data.10:09
dcalisteNow for mKCal, looking now at selectComponents() method:10:10
dcaliste- start is date and no end, like iCal;10:11
dcaliste- start is date and end is date.10:11
flypigI think there's a bug on line 122210:11
dcalisteWith a special treatment to keep backward compatibility, in case an end is given and is start + 12.10:12
flypigThe !end.isValid() case is skipped.10:12
dcalisteBut yes, there may be a bug there, please go ahead.10:12
dcalistewell, we don't need it, I think.10:13
dcalisteit's an event without given end date.10:13
flypigI see an "if (end.isValid())" (line 1222) inside a "if (!end.isValid())" (line 1219). That seems at least odd to me.10:13
dcalistethe else should be:10:13
dcaliste} else { //keep the end invalid and don't set it. }10:14
dcalistethe event is flagged allDay, ->dtStart() returns the start date and ->dtEnd() returns also the same start date because there is no end date set.10:15
flypigFrom the kcalcore event?10:15
dcalisteyes, ->foo() I meant them like event->dtStart() and event->dtEnd().10:15
flypigYes, agreed.10:16
dcalisteThe problem I think is coming from the fact that google sync adaptor was _not_ setting an end date and _not_ setting the all day flag properly at the same time.10:17
flypigThat's not the whole story I think.10:18
flypigThe endDate is set as invalid in the db. When it comes back out (through selectComponents) the endDate will be set as invalid again. So the endDate in future will return the same as the startDate.10:19
flypigShouldn't it be startDay + 1?10:19
dcalisteIn my opinion, no, not in KCalCore conventions.10:20
dcalisteThe dtEnd() method in KCalCore::Event is made to return start date if there is no end date.10:21
flypigOkay, but then the question in my mind is: if KcalCore returns an event with (allDay == true) and (endDate == startDate), how should that be handled?10:21
flypigBecause currently such events apparently aren't shown in the calendar at all.10:21
flypigMore precisely, the case where (allDay == true) and (endDate == startDate) and (hasEndDate == false)10:22
dcalisteArg, that's unexpected, indeed.10:23
dcalisteLet's look in nemo-qml-plugin-calendar if there is some day shifting in that particular case…10:23
flypigYes, good idea.10:24
dcalisteFor reccord, grep addDays -r ../ui-jolla-calendar/, is returning only false positive in my opinion. So I'm looking closer to the nemo QML ones.10:25
flypigLooking around CalendarWorker::createEventStruct(), it looks like it pulls the endTime in as-is.10:30
dcalisteArf, I cannot spot anything looking at the code iteself. I'm wondering if rawExpandedEvent() from mKCal::ExtendedCalendar, could fails also to list these particular events.10:34
dcalisteSo I'm going to follow the printf debug route.10:35
dcalisteI'm adding in a calendar such events:10:35
dcaliste- a dtstart-as-date only allDay event,10:35
dcaliste- a dtstart-as-date + dtend-as-date to the same date with all day flag,10:36
dcaliste- a dtstart-as-date + dtend-as-date to the same date without all day flag,10:36
dcaliste- a dtstart-as-date + dtend-as-date to the same date + 1 with all day flag,10:36
dcaliste- a dtstart-as-date + dtend-as-date to the same date + 1 without all day flag,10:36
dcalisteand the same last with dtstart +2 and see what is shown.10:37
dcalisteI'll report here when done. It may take a while though…10:37
flypigIs case 1 with dtend as invalid (i.e. 0 in the database)?10:37
dcalisteYes, that's it I think. I mean, I'm going to create these from C++ code and save them in a custom db. Then look at this db with a local instance of jolla calendar.10:39
flypigOkay, this would be great, thank you.10:39
flypigThe only possibly relevant thing I notice is this comment in nqpc:tools:main.cpp:prepareImportedIncidence() where it says:10:40
flypig/ calendar processing requires all-day events to have a dtEnd10:40
flypig(line 377)10:40
flypigThis implies to me that the dtEnd should be set, but that could be an old comment.10:40
flypigThat's nemo-qml-plugin-calendar/tools/icalconverter/main.cpp line 377 (my attempted simplification above was poor). In this case, it adds in the endDate as the startDate.10:42
dcalisteThis is an old comment, that I forgot to remove. It was due to a bug in KCalCore, where ->dtEnd() could return an invalid KDateTime() in certain circumstances making later code path to fail because of this invalidity.10:43
dcalisteThis bug has been fixed upstream and I backport the fix. Now ->dtEnd() cannot return an invalid KDateTime anymore.10:44
dcalisteBefore this kcalcore patch all using code were obliged to set an end date time, that's the origin of the comment I think.10:45
flypigOkay, that makes sense, but yes, I can see it's not relevant any more.10:45
flypigThe case is now caught in the event->dtEnd() code.10:46
flypigAlthough technically it could still happen if something calls event->setHasEndDate(false) without setting an end date, I suppose.10:46
flypigBut anyway, this is a digression.10:47
dcalisteNo, that's the purpose of the patch ;)10:47
flypigAh, okay, that makes sense.10:47
dcalisteOr at least, I remember so !10:47
dcalisteLet me check…10:47
flypigUpstream no longer has a setHasEndDate(), if I recall correctly.10:48
flypigYeah, hasEndDate() now just returns mDtEnd.isValid().10:48
dcalisteAh, yes, ok, if you setHasEndDate(true), but don't set an end date, ->dtEnd() will return an invalid date… The code path that was patched was setDtEnd(KDateTime()), which was then returning an invalid date in dtEnd() call.10:51
dcalisteBecause in setDtEnd(), it was done setHasEndDate(true) unconditionnally. Now it's guarded by setHasEbndDate(dtEnd.isValid()).10:52
dcalisteBut yeh, using setHasEndDate(true) alone will still produce invalid results…10:53
dcalisteNeed to check that setHasEndDate() is not used anywhere (particularly since it has been rightfully removed upstream as you pointed out).10:54
dcalisteI need to go for lunch, I'll report the test on various cases in db when coming back.11:04
flypigOf course, bon appétit!11:06
dcalisteflypig, ok, I've found the issue with dtstart alone not showing up in calendar.12:44
dcalisteIt's mkcal, when loading events by dates.12:44
dcalisteThe SELECT_COMPONENTS_BY_DATE_BOTH request did not take into account enddate not provided (meaning 0).12:45
dcalisteI'm preparing a test and a patch for mKCal, I'll ping you.12:46
dcalisteThen the seven cases detailed above are consistently displayed in calendar.12:46
dcalisteI mean, the dtstart only with allday flag is displayed as all day on the right day.12:46
dcalisteThe dtstart == dtend plus flag or not is displayed as full day on the right day only,12:47
dcalistethe dtend == dtstart + 1 case with flag is displayed on two days, and on one day without the flag.12:48
dcalistethe dtend == dtstart + 2 case with flag is displayed on three days, and on two day without the flag.12:48
flypigThat sounds great. I've prepared this simple change in anticipation: https://git.sailfishos.org/mer-core/buteo-sync-plugins-social/merge_requests/6913:04
flypigdcaliste, we have a deadline I'm afraid of 30 mins I'm afraid. Do you think it makes sense to try to get this merged in the next 30 mins? If not, I'll merge in the buteo workaround instead.13:05
dcalisteIf you go the social!69 way, you'll need this one also I guess : https://git.sailfishos.org/mer-core/mkcal/merge_requests/4513:07
dcalisteIt's the fix for mKCal.13:07
dcalisteLet's wrap what we discovered so far:13:08
dcaliste- without all day flag, to get proper duration, you need to set a dtend with an end date +1 on the date you want to actually stop (dtstart + 1 for a single day, dtstart + 2 for a two days event…)13:09
dcaliste- with alld day flag, you have to use inclusive dtend, so dtstart for a single day event, dtstart + 1 for a two day event…13:10
dcaliste- you can create a single day event by not providing an end date and puting the all day flag. But this requires the mKCal patch 4513:10
dcalisteSo I'm afraid there is a bug in your !69 patch in the case of multi day events.13:12
dcalisteBecause in syncadaptor.cpp line 602, (the else case), the allday flag is passed *with* the dtend being +1 to be exclusive like in iCal.13:13
dcalisteSo either I would keep your !68 since it will do the job and conform to the above conclusions on allday flag and dtend being +1 or not, or13:15
dcalisteyou'll need the mkcal patch plus your 69 plus a .addDays(-1) on the enddate in the else. And some additional testing :/13:15
dcalisteflypig, what do you think ?13:15
flypigThanks for the very clear explanation. I think it would be straightforward to address the shortcomings in social!69, so it could then be merged with mKCal!45.13:30
flypigAnd I think this is the "correct" route, but I'm open to persuasion.13:31
flypigBecause it feels to me like mKCal!45 is a good patch to include.13:31
flypigIt would also fix the calenders of those users who already experienced this issue.13:32
dcalisteIndeed, I would prefer the route using !69 and !45, with the additional modification to !69.13:35
dcalisteAs you said, it is cleaner and would solve the issue of people not seeing their single day events, hopefully without creating new ones.13:37
flypigI've updated social!69. I'll just now test it with mkcal!45. If they work together as expected, then I think that should be enough.13:41
dcalisteGreat, thank you flypig.13:46
flypigOkay, it's looking good, but there's still one broken case.13:58
flypigIf I create a multi-day event on the SF side, it loses a day after upsyncing.13:58
flypigIf I create it on the Google side, it's all fine.13:58
dcalisteAh, I guess there is a similar change to be done on upsync part in the sync adaptor. Let me have a look…13:59
dcalisteYes, line 442, there should be a addDays(1) I guess.14:00
dcalisteBecause the dtEnd of the event is inclusive at that time, but the JSon should contains exclusive dtEnd…14:01
flypigBut only if the start and end date differ by at least 214:01
flypigI guess maybe Google is massaging the endDate == startDate case, so probably in all cases.14:02
dcalisteMmh, good question… I think in all cases. To be completely symmetric, this if (event->dtEnd().isDateOnly()) should be in a complete if (event->hasDtEnd()) and don't send anything if there is no end date.14:03
dcalisteLooking at the parsing code further, I'm wondering if there should not be an else in the if (*endExists), otherwise the all day flag is not set when there is no end date, and the start date time is a date only case. Maybe Google doesnot send single day event like that and it's fine after all.14:10
dcalisteSo I would leave it like that for the moment in the parsing case, and modify the json serialisation case to always add one day to dtEnd in case of date only and don't bother with being symmetric.14:11
flypigGoogle seems to send single day events like this:14:12
flypig\"start\": {14:12
flypig \"date\": \"2020-10-06\"14:12
flypig },14:12
flypig\"end\": {14:12
flypig \"date\": \"2020-10-07\"14:12
flypigSo I think it's fine.14:12
flypigI agree: I'd want to check the spec about whether it's acceptable to not send an end date. So the simpler solution seems fine to me.14:13
flypigI've updated the PR. It seems to work okay (I created single, multi day events on both sides and they seem to sync fine).14:13
flypigdcaliste, could you please add "Contributes to JB#51406" to your commit?14:16
dcalisteSure, a moment…14:16
flypigAnd I am then happy that they're both ready to be merged, if you think the same.14:16
dcaliste… ok, JB bug added.14:17
flypigThank you!14:17
dcalisteGreat, this hopefully will make happy users :))14:19
flypigLet's hope so! Thanks for your fix and sorry for the rush. pvuorela is giving it a review and depending on his decision I can then hopefully merge it..14:22
dcalisteNo problem, I prefer to have this fixed indeed. I feel a bit responsible after all since I initiated the transition to use inclusive dtend for all day events. This moved as been done upstream a long time ago and we would have to face it sooner or later when transitioning to upstream.14:25
flypigYes, it's a good change, but there is always the potential for regression. If this is the only case that was missed, then that's pretty good going really.14:27
*** Sellerie0 is now known as Sellerie14:44
flypigThanks again dcaliste; both PRs have now been merged. I'll take a look at the EAS setIsVisible() issue you flagged up in the morning.15:50

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