| *** zbenjamin_ is now known as zbenjamin | 01:58 | |
| *** nyov is now known as Guest39749 | 03:29 | |
| *** JvD_ is now known as JvD | 04:38 | |
| *** frinring_ is now known as frinring | 05:56 | |
| dcaliste | Good afternoon chriadam. | 07:03 |
|---|---|---|
| chriadam | hello dcaliste :-) | 07:03 |
| chriadam | how has your week been? | 07:03 |
| dcaliste | Great thank you, I hope yours was fine also. | 07:04 |
| chriadam | yes, busy but good | 07:04 |
| chriadam | thanks | 07:04 |
| chriadam | if my memory serves, we had the following action items open from last week's meeting: | 07:05 |
| chriadam | 1) the caldav PR72 which I promised to review/test/merge this week | 07:05 |
| chriadam | 2) the QMF patch which I think we should merge (pinged pvuorela about that one on the gitlab) | 07:06 |
| chriadam | 3) the jolla-calendar PRs which flypig is discussing with MartinS and yourself | 07:06 |
| chriadam | then for this week, we have new topics: | 07:06 |
| chriadam | a) use UNTIL instead of COUNT in RRULE with VTIMEZONE kcalcore PR | 07:06 |
| chriadam | is there any other topics you'd like to discuss today also? | 07:07 |
| dcaliste | Maybe the Qt6 transition of QMF, I've updated my changes to follow the solutions used by Edward in QtPIM. | 07:09 |
| dcaliste | For QSet()::toList(), I'm using QSet()::values(), and for QDateTime(QDate()), I'm using QDate()::startOfDay(). | 07:09 |
| dcaliste | That's the two changes done recently. | 07:10 |
| chriadam | cool - 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 |
| dcaliste | Besides, your list is exact. | 07:10 |
| chriadam | ok great, let's start with (1) | 07:11 |
| chriadam | I haven't had a chance to look into it properly yet | 07:11 |
| chriadam | are there any specific parts of the code which I might find tricky to review? | 07:11 |
| dcaliste | The 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 |
| chriadam | makes sense | 07:13 |
| chriadam | from a brief look, it seems there are quite a few changes. I'll try to digest it properly tomorrow. | 07:15 |
| chriadam | can you suggest how I might best test it with my own nextcloud server? | 07:15 |
| dcaliste | The 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 |
| dcaliste | This is because an error now should not cancel pending requests. | 07:17 |
| dcaliste | If these requests are independant from the rror. | 07:17 |
| dcaliste | About 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 |
| dcaliste | To check the new functionnalities, it's very tricky because you need to trigger errors from server side, which is complex… | 07:19 |
| dcaliste | The error client side are also difficult to trigger, because they would correspond to bugs in mKCal… | 07:20 |
| chriadam | indeed, it's the error cases I was wondering about | 07:20 |
| chriadam | but maybe e.g. I could set some particular event as "read-only" on server-side somehow, just prior to the upsync... | 07:20 |
| dcaliste | You can do to the whole calendar in fact. That's a good point. | 07:21 |
| dcaliste | Do local modifications on device, change the calendar to read-only server side and sync. | 07:22 |
| dcaliste | The read-only detection of the CalDAV plugin will trigger, but will inforce in in mKCal for the future modifications. | 07:22 |
| dcaliste | So it should try to push anyway and should fail. | 07:23 |
| dcaliste | Ah, well no… | 07:23 |
| dcaliste | The plugin is now smart enough not to try to push local modifications to read-only calendars… | 07:23 |
| dcaliste | So if you can set somehow a read-only flag per event, it may trigger the error path. | 07:24 |
| dcaliste | In 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 |
| dcaliste | But 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 |
| dcaliste | Thus 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 |
| chriadam | yes | 07:30 |
| *** vilpan is now known as Guest15601 | 07:30 | |
| chriadam | experience 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 issues | 07:31 |
| chriadam | my concern is solely "how can we test the error handling codepaths sufficiently" | 07:31 |
| chriadam | but that's relatively minor - I definitely agree with the direction of the PR. | 07:31 |
| chriadam | and 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 |
| chriadam | within my build* I meant | 07:32 |
| chriadam | cool | 07:32 |
| chriadam | well, hopefully I get a chance to do that in the next couple of days | 07:32 |
| chriadam | was ther esomething else about (1) to discuss, or shall we move to (2) | 07:32 |
| dcaliste | It's ok, we can move to (2). | 07:33 |
| chriadam | ok, 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 |
| dcaliste | Ok, thank you, that's great. | 07:34 |
| dcaliste | Looking at the blame of this line, it was like that since the beginning of git history. | 07:34 |
| chriadam | doesn't surprise me | 07:34 |
| dcaliste | So it was either done on purpose, or forgotten. But at least, it was not removed because breaking something. | 07:35 |
| chriadam | yep. I think it was just an oversight | 07:35 |
| chriadam | for (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 MartinS | 07:35 |
| flypig | Sure. | 07:36 |
| flypig | It's basically there; Martin was happy with it. I think there's just now a question raised by pvuorela concerning the wording. | 07:36 |
| dcaliste | Hello flypig, thanks for the review and pushing this internally. | 07:37 |
| flypig | Thanks for your explanation about the "how to fix" messages. What you wrote makes sense, so I consider that resolved. | 07:37 |
| flypig | dcaliste, you're welcome; I'm sorry I didn't get it all done sooner. | 07:37 |
| dcaliste | About the wording, I agree and proposed another possibility yesterday. Let see what pvuorela is thinking of it. | 07:38 |
| chriadam | I 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 |
| flypig | Yes, 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 |
| flypig | I'm happy to merge it as soon as this last question about the wording is resolved. | 07:40 |
| dcaliste | A 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 |
| flypig | Yes, I saw that but didn't have time to follow it up. Yes, I'll do that, one second. | 07:41 |
| dcaliste | Sure, 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 |
| flypig | ls | 07:42 |
| flypig | yes | 07:42 |
| flypig | Well, it doesn't seem to appear anywhere. | 07:42 |
| *** Guest15601 is now known as vilpan | 07:42 | |
| dcaliste | Ok, so need to find where it can come from… | 07:43 |
| flypig | Let me do a proper check though while you continue with chriadam and I'll let you know what I find. | 07:43 |
| dcaliste | The other place where setIsVisible() is called is in the contactsd plugin, when there is an account change. | 07:43 |
| dcaliste | I 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 |
| flypig | Ah, I was mistaken. It does appear in the eas code. Also here: nemo-qml-plugin-calendar/src/calendarworker.cpp (lines 626, 921). | 07:45 |
| dcaliste | Nothing solid at the moment, but visibility did switched on without prior notice when switching on and off calendars in the setting page. | 07:46 |
| dcaliste | Ok, so maybe two different bugs actually. | 07:46 |
| chriadam | some contactsd things might have changed in devel since the qtcontacts update merge etc | 07:46 |
| dcaliste | The one in calendar worker is valid (at least intentional) because it's the entry point for the calendar UI to switch visibility. | 07:46 |
| chriadam | not sure | 07:46 |
| dcaliste | But the one in EAS may have to be checked. Neither CalDAV pluign nor Google social sync plugin is changing the visibility. | 07:47 |
| dcaliste | Except 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 |
| dcaliste | If 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 |
| dcaliste | But maybe inconvenient ;) | 07:49 |
| dcaliste | And 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 |
| flypig | For EAS one is in createNotebook(), which seems valid, the other in updateVisibilities(..). I should probably look into that second one then? | 07:50 |
| dcaliste | flypig, yes, I think you're right. | 07:50 |
| dcaliste | chriadam, 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 |
| chriadam | no problem :-) | 07:51 |
| chriadam | yes | 07:51 |
| dcaliste | As 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 |
| chriadam | If upstream kcalcore has been using that change for some time, with no issues, then that certainly suggests that most servers support the UNTIL | 07:51 |
| dcaliste | Indeed. | 07:52 |
| chriadam | so, I am happy with that one. I would like pvuorela or flypig to concur before I push the button, however | 07:52 |
| dcaliste | They have a bigger user base than SFOS ;) | 07:52 |
| chriadam | one other thing to mention: will that make the proper kcalcore upgrade more difficult due to mismatching sha1 etc? | 07:52 |
| dcaliste | Sure, it's not in a hurry, I've patched my kcalcore to have my meeting time on proper values. | 07:52 |
| dcaliste | About 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 |
| chriadam | cool | 07:53 |
| chriadam | sounds good to me, then | 07:53 |
| dcaliste | Then, the kcalcore switch will be done with a complete new repo in my opinion. | 07:53 |
| chriadam | makes sense to me. | 07:54 |
| chriadam | we'll have to get the test team to do some very thorough regression testing when that day comes ;-) | 07:54 |
| chriadam | well, I think that about covers that topic. I will poke pvuorela about the QMF PR and about this kcalcore PR. | 07:56 |
| chriadam | any other topics to discuss tonight? | 07:56 |
| dcaliste | Exact. 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 |
| dcaliste | I'm going to rework my WIP MR in mKCal with that respect soon. | 07:57 |
| dcaliste | But 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 |
| dcaliste | I've seen flypig proposing a WIP patch there. | 07:57 |
| flypig | Something 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 |
| chriadam | yes ;-) | 07:59 |
| chriadam | we should fix that | 07:59 |
| flypig | Given that it's being fixed elsewhere, it would be nice I thought. | 07:59 |
| dcaliste | (Use CalDAV ;) ) | 07:59 |
| flypig | :D | 07:59 |
| chriadam | are you volunteering, flypig? :-P | 07:59 |
| chriadam | but, yes you're right | 07:59 |
| flypig | We can add that to the error message ;) | 07:59 |
| dcaliste | Ah ah yes indeed, good quick fix. | 08:00 |
| flypig | Haha. I only have myself to blame chriadam. | 08:00 |
| flypig | Just for info regarding EAS calendar visibility, I created an internal bug about it (JB#51506). Will do the same for Google errors. | 08:01 |
| chriadam | dcaliste: if you have any comments about that google calendar end-date-time for all-day events thing, we're all ears | 08:01 |
| flypig | Yes, very much so. Actually, I have a quick question about that. | 08:02 |
| chriadam | flypig's change LGTM but I haven't tested on device yet, and you know that stuff better than I do, now | 08:02 |
| flypig | Am I misreading, or does this line look odd? | 08:02 |
| flypig | https://git.sailfishos.org/mer-core/buteo-sync-plugins-social/merge_requests/68/diffs#1019a8d724c37777703cf35198f20392ba0b9178_893_895 | 08:02 |
| dcaliste | I 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 |
| dcaliste | I wonder if the same reasoning can be applied here. But I've just read the modified line at the moment. | 08:04 |
| chriadam | flypig: you mean UPDATE_EVENT_PROPERTY_IF_REQUIRED(event, allDay, setAllDay, false, changed) ? | 08:05 |
| flypig | Well, that's the line, but it just seems odd that it's set to false there. | 08:05 |
| dcaliste | So 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 |
| flypig | Yes, 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 |
| chriadam | is 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 |
| dcaliste | chriadam, no I don't think so. | 08:07 |
| dcaliste | The 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)" case | 08:08 |
| chriadam | but my brain isn't working currently, and I don't remember any of the code. | 08:08 |
| dcaliste | flypig, I'll look at it further in the coming days, tomorrow or the day after. | 08:08 |
| chriadam | tyvm dcaliste | 08:08 |
| flypig | +1 thanks dcaliste | 08:08 |
| chriadam | we might need to merge flypig's change as a stopgap / emergency for 3.4.0 | 08:09 |
| chriadam | and do deeper investigation / fixes for the next release | 08:09 |
| chriadam | as I think release team is preparing the latest rc for 3.4.0 today or tomorrow | 08:09 |
| dcaliste | Yes, 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 |
| chriadam | dcaliste: does that sound ok to you, or is flypig's current change likely to cause issues? | 08:10 |
| chriadam | great, thanks. | 08:10 |
| chriadam | ok - anything else to discuss or should we wrap up the meeting? | 08:11 |
| dcaliste | I 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 |
| chriadam | :-) | 08:12 |
| chriadam | or flypig did you have anything further to discuss? | 08:13 |
| flypig | No, that's all great from my side. Thanks as always dcaliste :) | 08:14 |
| dcaliste | Thank you for the meeting today. Let's meet together next week and discuss these issues on MRs. | 08:14 |
| chriadam | thank you very much | 08:14 |
| chriadam | have a great week | 08:14 |
| chriadam | gnight! | 08:14 |
| dcaliste | flypig, 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 |
| dcaliste | As 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 |
| flypig | Yes, I I agree, I'm just looking at that now. But the current case has !event->hasEndDate(), so that's not happening. | 10:04 |
| flypig | So, 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 |
| flypig | ? | 10:06 |
| flypig | I think the allDay() flag issue needs fixing, but it won't solve this particular problem. | 10:07 |
| dcaliste | No, in fact, we don't want to save end date if the event don't have one. | 10:07 |
| flypig | So, what is the correct way to store a single-day all-day event? | 10:07 |
| dcaliste | There are two: | 10:08 |
| dcaliste | (Wait, reading the code again) | 10:08 |
| dcaliste | For 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 |
| dcaliste | This is for iCal data. | 10:09 |
| dcaliste | Now 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 |
| flypig | I think there's a bug on line 1222 | 10:11 |
| dcaliste | With a special treatment to keep backward compatibility, in case an end is given and is start + 12. | 10:12 |
| flypig | The !end.isValid() case is skipped. | 10:12 |
| dcaliste | But yes, there may be a bug there, please go ahead. | 10:12 |
| dcaliste | well, we don't need it, I think. | 10:13 |
| dcaliste | it's an event without given end date. | 10:13 |
| flypig | I see an "if (end.isValid())" (line 1222) inside a "if (!end.isValid())" (line 1219). That seems at least odd to me. | 10:13 |
| dcaliste | the else should be: | 10:13 |
| dcaliste | } else { //keep the end invalid and don't set it. } | 10:14 |
| dcaliste | the 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 |
| flypig | From the kcalcore event? | 10:15 |
| dcaliste | yes, ->foo() I meant them like event->dtStart() and event->dtEnd(). | 10:15 |
| flypig | Yes, agreed. | 10:16 |
| dcaliste | The 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 |
| flypig | That's not the whole story I think. | 10:18 |
| flypig | The 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 |
| flypig | Shouldn't it be startDay + 1? | 10:19 |
| dcaliste | In my opinion, no, not in KCalCore conventions. | 10:20 |
| dcaliste | The dtEnd() method in KCalCore::Event is made to return start date if there is no end date. | 10:21 |
| flypig | Okay, 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 |
| flypig | Because currently such events apparently aren't shown in the calendar at all. | 10:21 |
| flypig | More precisely, the case where (allDay == true) and (endDate == startDate) and (hasEndDate == false) | 10:22 |
| dcaliste | Arg, that's unexpected, indeed. | 10:23 |
| dcaliste | Let's look in nemo-qml-plugin-calendar if there is some day shifting in that particular case… | 10:23 |
| flypig | Yes, good idea. | 10:24 |
| dcaliste | For 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 |
| flypig | Looking around CalendarWorker::createEventStruct(), it looks like it pulls the endTime in as-is. | 10:30 |
| dcaliste | Arf, 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 |
| dcaliste | So I'm going to follow the printf debug route. | 10:35 |
| dcaliste | I'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 |
| dcaliste | and the same last with dtstart +2 and see what is shown. | 10:37 |
| dcaliste | I'll report here when done. It may take a while though… | 10:37 |
| flypig | Is case 1 with dtend as invalid (i.e. 0 in the database)? | 10:37 |
| dcaliste | Yes, 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 |
| flypig | Okay, this would be great, thank you. | 10:39 |
| flypig | The 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 dtEnd | 10:40 |
| flypig | (line 377) | 10:40 |
| flypig | This implies to me that the dtEnd should be set, but that could be an old comment. | 10:40 |
| flypig | That'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 |
| dcaliste | This 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 |
| dcaliste | This bug has been fixed upstream and I backport the fix. Now ->dtEnd() cannot return an invalid KDateTime anymore. | 10:44 |
| dcaliste | Before 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 |
| flypig | Okay, that makes sense, but yes, I can see it's not relevant any more. | 10:45 |
| flypig | The case is now caught in the event->dtEnd() code. | 10:46 |
| dcaliste | Exactly. | 10:46 |
| flypig | Although technically it could still happen if something calls event->setHasEndDate(false) without setting an end date, I suppose. | 10:46 |
| flypig | But anyway, this is a digression. | 10:47 |
| dcaliste | No, that's the purpose of the patch ;) | 10:47 |
| flypig | Ah, okay, that makes sense. | 10:47 |
| dcaliste | Or at least, I remember so ! | 10:47 |
| dcaliste | Let me check… | 10:47 |
| flypig | Upstream no longer has a setHasEndDate(), if I recall correctly. | 10:48 |
| flypig | Yeah, hasEndDate() now just returns mDtEnd.isValid(). | 10:48 |
| dcaliste | Ah, 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 |
| dcaliste | Because in setDtEnd(), it was done setHasEndDate(true) unconditionnally. Now it's guarded by setHasEbndDate(dtEnd.isValid()). | 10:52 |
| dcaliste | But yeh, using setHasEndDate(true) alone will still produce invalid results… | 10:53 |
| dcaliste | Need to check that setHasEndDate() is not used anywhere (particularly since it has been rightfully removed upstream as you pointed out). | 10:54 |
| dcaliste | I need to go for lunch, I'll report the test on various cases in db when coming back. | 11:04 |
| flypig | Of course, bon appétit! | 11:06 |
| dcaliste | flypig, ok, I've found the issue with dtstart alone not showing up in calendar. | 12:44 |
| dcaliste | It's mkcal, when loading events by dates. | 12:44 |
| dcaliste | The SELECT_COMPONENTS_BY_DATE_BOTH request did not take into account enddate not provided (meaning 0). | 12:45 |
| dcaliste | I'm preparing a test and a patch for mKCal, I'll ping you. | 12:46 |
| dcaliste | Then the seven cases detailed above are consistently displayed in calendar. | 12:46 |
| dcaliste | I mean, the dtstart only with allday flag is displayed as all day on the right day. | 12:46 |
| dcaliste | The dtstart == dtend plus flag or not is displayed as full day on the right day only, | 12:47 |
| dcaliste | the dtend == dtstart + 1 case with flag is displayed on two days, and on one day without the flag. | 12:48 |
| dcaliste | the dtend == dtstart + 2 case with flag is displayed on three days, and on two day without the flag. | 12:48 |
| flypig | That sounds great. I've prepared this simple change in anticipation: https://git.sailfishos.org/mer-core/buteo-sync-plugins-social/merge_requests/69 | 13:04 |
| flypig | dcaliste, 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 |
| dcaliste | If you go the social!69 way, you'll need this one also I guess : https://git.sailfishos.org/mer-core/mkcal/merge_requests/45 | 13:07 |
| dcaliste | It's the fix for mKCal. | 13:07 |
| dcaliste | Let'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 45 | 13:10 |
| dcaliste | So I'm afraid there is a bug in your !69 patch in the case of multi day events. | 13:12 |
| dcaliste | Because 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 |
| dcaliste | So 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, or | 13:15 |
| dcaliste | you'll need the mkcal patch plus your 69 plus a .addDays(-1) on the enddate in the else. And some additional testing :/ | 13:15 |
| dcaliste | flypig, what do you think ? | 13:15 |
| flypig | Thanks 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 |
| flypig | And I think this is the "correct" route, but I'm open to persuasion. | 13:31 |
| flypig | Because it feels to me like mKCal!45 is a good patch to include. | 13:31 |
| flypig | It would also fix the calenders of those users who already experienced this issue. | 13:32 |
| dcaliste | Indeed, I would prefer the route using !69 and !45, with the additional modification to !69. | 13:35 |
| dcaliste | As 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 |
| flypig | I'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 |
| dcaliste | Great, thank you flypig. | 13:46 |
| flypig | Okay, it's looking good, but there's still one broken case. | 13:58 |
| flypig | If I create a multi-day event on the SF side, it loses a day after upsyncing. | 13:58 |
| flypig | If I create it on the Google side, it's all fine. | 13:58 |
| dcaliste | Ah, I guess there is a similar change to be done on upsync part in the sync adaptor. Let me have a look… | 13:59 |
| dcaliste | Yes, line 442, there should be a addDays(1) I guess. | 14:00 |
| dcaliste | Because the dtEnd of the event is inclusive at that time, but the JSon should contains exclusive dtEnd… | 14:01 |
| flypig | But only if the start and end date differ by at least 2 | 14:01 |
| flypig | I guess maybe Google is massaging the endDate == startDate case, so probably in all cases. | 14:02 |
| dcaliste | Mmh, 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 |
| dcaliste | Looking 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 |
| dcaliste | So 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 |
| flypig | Google 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 |
| flypig | So I think it's fine. | 14:12 |
| flypig | I 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 |
| flypig | I'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 |
| flypig | dcaliste, could you please add "Contributes to JB#51406" to your commit? | 14:16 |
| dcaliste | Sure, a moment… | 14:16 |
| flypig | And 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 |
| flypig | Thank you! | 14:17 |
| dcaliste | Great, this hopefully will make happy users :)) | 14:19 |
| flypig | Let'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 |
| dcaliste | No 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 |
| flypig | Yes, 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 Sellerie | 14:44 | |
| flypig | Thanks 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/!