| *** zbenjamin is now known as Guest12439 | 02:57 | |
| *** zbenjamin_ is now known as zbenjamin | 02:57 | |
| dcaliste | Hello chriadam, how are you ? | 08:09 |
|---|---|---|
| chriadam | hi dcaliste, I'm well thanks! how are you? | 08:10 |
| dcaliste | Winter sickness, but it will pass… | 08:10 |
| chriadam | oh no! I hope you recover swiftly | 08:10 |
| chriadam | I must confess that I haven't had time in the past week to take action on any of the topics we discussed at last meeting, unfortunately | 08:11 |
| dcaliste | Another virus, not the famous one ;) No problem, nothing is in a hurry. | 08:11 |
| chriadam | I will definitely find time to review some PRs this week | 08:11 |
| dcaliste | And most of what we discuss last week was for medium term changes. | 08:12 |
| chriadam | so it might be a short meeting tonight, considering that you've been sick and I've not managed to progress things! | 08:13 |
| chriadam | did you have any topics to discuss? I need to poke Jaymzz again about the blog post... | 08:13 |
| dcaliste | I think, you notice, but there are these two new MRs: | 08:13 |
| Venemo | hey guys | 08:14 |
| dcaliste | - https://git.sailfishos.org/mer-core/nemo-qml-plugin-calendar/merge_requests/53 | 08:14 |
| dcaliste | - and https://bitbucket.org/jolla/ui-jolla-calendar/pull-requests/255 | 08:14 |
| dcaliste | Hello Venemo. I hope you're fine. | 08:15 |
| chriadam | hi Venemo | 08:15 |
| Venemo | totally find, thanks | 08:15 |
| chriadam | I will check | 08:15 |
| Venemo | totally fine, thanks | 08:15 |
| Venemo | I hope you are well too | 08:15 |
| dcaliste | It's about this TJC question : https://together.jolla.com/question/90867/hide-unselected-calendars/ | 08:15 |
| dcaliste | The fact that when creating a new event, non-displayed calendars are proposed in the list, which can be misleading. | 08:16 |
| chriadam | seems indeed good to fix! thank you. | 08:16 |
| chriadam | I hope pvuorela will have time to check the jolla-calendar one | 08:16 |
| chriadam | the n-q-p-c side seems simple enough (just exposing the excluded role) | 08:17 |
| dcaliste | Yes, I'm using the visible property to hide the excluded calendars, whic is not optimal, but which is minimal changes. | 08:17 |
| dcaliste | s/whic/which/ | 08:17 |
| chriadam | seems fine to me, but my brain isn't really working right now (context switched from another project recently, so brain not fully working yet ;-) | 08:19 |
| dcaliste | No problem, besides, I'm going to add MR in buteo-qml-plugin-caldav to discover the read-only property of listed calendars using this https://stackoverflow.com/questions/57116267/determine-access-rights-for-caldav-calendars | 08:21 |
| dcaliste | It may create cobflict with your caldav MR, but I'll rebase if necessary, or build on top of yours if accepted soon enough. | 08:22 |
| dcaliste | s/cobflict/conflicts/ | 08:22 |
| chriadam | ah! that would be very helpful, thanks | 08:22 |
| dcaliste | It may solve various TJC issue of spurious modification that cannot be uploaded nad make the whole thing not syncable anymore, see https://together.jolla.com/question/223428/sync-error-on-read-only-calendar/ | 08:23 |
| chriadam | I'm hoping my MR will go in tomorrow or thursday, gotta fix one more thing, I guess, then it should be fine. | 08:23 |
| chriadam | yep, I have seen such issues in the past | 08:23 |
| chriadam | but wasn't sure how to approach | 08:23 |
| chriadam | the problem | 08:23 |
| chriadam | we tried to do something in google calendar (i.e. support owner and write semantics, and also partially allow read-only calendars) but that confused users sometimes, since we didn't expose the read-onlyness to the UI | 08:24 |
| dcaliste | After Googling a bit yesterday, I discover there is an RFC for this : https://tools.ietf.org/html/rfc5397 | 08:24 |
| chriadam | so they would make changes in an event, and expect it to get upsynced, but we'd drop the update silently if it was for a read only calendar | 08:24 |
| chriadam | I know that mkcal notebook has a readOnly flag, but cannot remember if that worked or not | 08:25 |
| dcaliste | Well, read-only in UI makes event not modifiable ??? | 08:25 |
| chriadam | there was some issue, anyway | 08:25 |
| dcaliste | When you long press on them, it says "not modifiable" | 08:25 |
| chriadam | ah, maybe I'm misremembering | 08:25 |
| dcaliste | And the mkcal flag is working, I'm using it for webcal calendars. | 08:25 |
| dcaliste | So, the only missing bit is the discovery I guess. | 08:26 |
| chriadam | ah great | 08:26 |
| dcaliste | Now that we have propfind in caldav plugin, the road is clear I thnk to have it. | 08:26 |
| chriadam | sounds good :-) | 08:26 |
| chriadam | thanks very much | 08:26 |
| dcaliste | About the attendee list in qml plugin, what do you think ? | 08:27 |
| dcaliste | Should I give a look to major changes on introducing a minimal struct specifically for agenda models ? | 08:27 |
| dcaliste | pvuorela seems to be concern with performence penalties with the full event struct being populated for such models. | 08:28 |
| chriadam | I saw the comment, haven't yet had time to discuss with him | 08:28 |
| chriadam | the code from what I can see already enumerates the attendees and walks over them | 08:28 |
| chriadam | so what we would save is simply caching the data in the EventData struct member (some memory cost to that) | 08:28 |
| dcaliste | Ok, if you think it requires a medium term rework, don't hesitate to ask. | 08:29 |
| chriadam | and some cost to marshalling/demarshalling when sending over the thread boundary in the signal or whatever | 08:29 |
| chriadam | thank you - let's see where the discussion goes, because currently without seeing some hard data, I'd be inclined to keep the PR solution (simple, race-free) | 08:29 |
| chriadam | but of course in some cases memory pressure is very real, so I will try to benchmark some things and see where we get | 08:30 |
| dcaliste | Yeh, I more inclined to keep it as simple as possible, but it requires some hard figures to decide. Sorry for making the solving process a bit longer ! | 08:31 |
| chriadam | no, thank you for your input, greatly appreciated as always | 08:32 |
| dcaliste | This may be also the starting point of a reflexion about signaling from mkcal and see what should be designed (and possible) to improve this storageModified() signal, introduce more fine grained ones… | 08:33 |
| chriadam | very good point... | 08:33 |
| chriadam | amccarthy: oh, hi! long time no chat! How's things? | 08:33 |
| chriadam | I'll have to poke daniel about organising a dead trolls soon, so we can all catch up! | 08:34 |
| chriadam | dcaliste: I wonder what other observers of that signal we have | 08:34 |
| chriadam | dcaliste: technically, I guess it should be used in contactsd to trigger the "sync on change" automatically | 08:34 |
| dcaliste | About this signaling, no specific things in mind at the moment though, just keep this in the back of mind to mature. | 08:35 |
| chriadam | but I don't think I used that one, at the time (the solution we used was fairly hackish unfortunately, my fault) | 08:35 |
| chriadam | yes | 08:35 |
| dcaliste | One other non-related thing : I was wrong about QDateTime not handling ClockTime spec. LocalTime is made for this. | 08:36 |
| dcaliste | I was misleaded by the fact that timeZone() is returning the current one. | 08:36 |
| dcaliste | But when changing zone, this function is returning the new zone. | 08:37 |
| dcaliste | And time doesnot change. | 08:37 |
| dcaliste | So when switching to upstream kcalcore, we can use this LocalTime. Which bringed me to the infamous rawExpandedEvent() function… | 08:38 |
| chriadam | I don't understand... are you saying: QDateTime clockTime = QDateTime(someDate, someTime, LocalTime); clockTime.setTimeZone(helsinki); clockTime.date() == someDate && clockTime.time() == someTime stil? | 08:38 |
| dcaliste | Not exactly. | 08:38 |
| dcaliste | If you set a time zone, it will change spec to TimeZone spec. | 08:39 |
| dcaliste | but clockTime.timeZone() is return systemTimeZone(). | 08:39 |
| dcaliste | Which made me think at the time, that it was equivalent to KDateTime::LocalZone. | 08:39 |
| dcaliste | But no, TZ=bar clockTime = someTime and TZ=foo clockTime = sameSomeTime. | 08:40 |
| chriadam | ok, so it's just that if you check timeSpec, and that's LocalTime, then you can (for all intents and purposes) ignore the timeZone() | 08:41 |
| chriadam | or am I still misunderstanding? | 08:41 |
| dcaliste | Right. | 08:41 |
| chriadam | cool. ok, that's good news | 08:41 |
| dcaliste | Which bringed me to rawExpandedEvents() and its timeSpec (or timeZone) argument… | 08:41 |
| dcaliste | If we want to keep this function as simple as possible, we should remove this argument and assume it is used to always expand in current time zone. | 08:42 |
| dcaliste | Which I guess is how it is called now from nemo-qml-plugin-calendar. | 08:42 |
| chriadam | yes. I wonder what use cases there are for the "specific timezone expansion" parameter in the first place? | 08:43 |
| chriadam | aside from the unit tests ;-) | 08:43 |
| chriadam | which. hrm. is not a terrible reason... | 08:43 |
| dcaliste | Assuming this, we can avoid the complex move of every LocalTime events to the other time zone without changing time as we're doing now. | 08:43 |
| dcaliste | Since QDateTime LocalTime is having a proper zone, all comparison are working for free, assumling that we're dealing with current time zone. | 08:44 |
| dcaliste | And yes, I think the only us case I can forsee is for tests in fact :/ | 08:44 |
| dcaliste | s/us/use/ | 08:44 |
| dcaliste | For not using local zone I mean as a timeSpec argument. | 08:45 |
| dcaliste | The only use case would be to display the calendar in a different time zone than the current. | 08:45 |
| chriadam | I assume the test could simply instead use QLocale or so to set tz to specific tz, perform the test and check expected results for that tz, instead of doing that parametrisation | 08:45 |
| dcaliste | Which is not supported in the UI, and not usefull in my opinion. | 08:45 |
| dcaliste | Indeed, that's correct. The problem with the test is that we want to test that this function is working in different scenarios, | 08:46 |
| chriadam | pvuorela: is that some feature which we might want to support? e.g. "switch calendar to tz" option, in case you want to see what your schedule will be on a business trip or something? | 08:46 |
| chriadam | personally, I think "probably not" | 08:46 |
| chriadam | and the resilience we'd gain from simplifying the code has great value, IMO | 08:47 |
| dcaliste | Me neither, and it simplifies a lot rawExpandedEvents(). | 08:47 |
| dcaliste | But well, it's for later anyway, because we need QDateTime for this to work. | 08:47 |
| chriadam | right | 08:48 |
| dcaliste | What was broken with KDateTime is that ClockTime has no associated time zone and thus comparisons fails depending on scenarios. | 08:48 |
| dcaliste | Understood this this week, so just wanted to let you know about this, in case you need some plannig in advance. | 08:49 |
| chriadam | my brain has turned off unfortunately. this is in the kcalcore upgrade path isn't it? i.e. that's when we get rid of kdatetime and switch to qdatetime? | 08:51 |
| dcaliste | Exact. | 08:51 |
| dcaliste | I was trying to rebase mkcal upgrade MR and found this in the process. | 08:52 |
| chriadam | right. in any case, I think that simplifying the rawExpandedEvents() makes sense, in order to avoid parametrisation. updating the unit tests to verify behaviour properly will be an important step there I guess. | 08:52 |
| dcaliste | Yes, what I don't like is that it requires to change code and tests at the same time, when switching to QDateTime… | 08:53 |
| chriadam | aside from that, perhaps I'm not following (in regards to the KDateTime comparisons failing - does that affect us now, currently? or is that something we handled via the manually settings things to ClockTime etc without the conversion through local-time?) | 08:53 |
| chriadam | true, but so long as we're confident that the changes+tests are valid, then I'm fine with that. after all, we've added a bunch of tests and fixed issues already, so to me there's not too much difference ;-) | 08:54 |
| chriadam | but I agree that the more moving parts, the more chance for accidental regression. but, sometimes such risks are necessary to move to a better place. | 08:54 |
| dcaliste | All the patch you did last year in rawExpandedEvents() was to handle these failing comparisons micely with KDateTime. Now, it's fine, but complicated. | 08:54 |
| dcaliste | lol s/micely/nicely/ | 08:55 |
| chriadam | right, I was hoping there weren't some we missed back then! | 08:56 |
| chriadam | and, yes, complicated. will be good to simplify, for sure. | 08:56 |
| chriadam | thanks again for looking into this, and pushing the kcalcore upgrade forward | 08:56 |
| dcaliste | Still need to work on the rebase though with my new understanding of the LocalTime spec, because at the moment, it's a mess in mkcal MR ;) | 08:57 |
| chriadam | :-) | 08:58 |
| dcaliste | So, let's work on opened MR and I'm going to add the access right discovery for CalDAV. | 08:58 |
| chriadam | sounds good, thank you | 08:58 |
| chriadam | I will definitely check the nqpc/jolla-calendar MRs, and follow up on the PRs we discussed last week aslo | 08:59 |
| chriadam | thanks again for your time and effort, and I hope that you have a good week (and feel better soon!) | 08:59 |
| dcaliste | Thanks, got medicine now, it should improve more quickly ;) | 09:00 |
| dcaliste | Have a nice week chriadam. See you. | 09:00 |
| chriadam | thanks! gnight! | 09:00 |
| *** ecloud is now known as ecloud_wfh | 10:01 | |
| *** frinring_ is now known as frinring | 13:15 | |
| *** vilpan is now known as Guest14675 | 17:11 | |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!