| dcaliste | Good morning pvuorela. Thank you for the reviews and suggestions on mKCal PRs. | 08:02 |
|---|---|---|
| pvuorela | dcaliste: hey. been nicely getting rid of unnecessary cruft there. | 08:03 |
| dcaliste | Yes, what remains get much clearer. | 08:05 |
| dcaliste | I've also tried finally to get rid of timezone storage, maybe you've seen this PR : https://github.com/sailfishos/mkcal/pull/52 | 08:06 |
| dcaliste | It was not as scary as I feared. | 08:06 |
| pvuorela | yes, noticed that. on the surface seemed good but prioritized the other PR first. | 08:09 |
| pvuorela | maybe we'd finally be able to survive a timezone change while calendar app is running :D | 08:09 |
| dcaliste | Yes, I would like to give a look to what happen at the moment. My guess it that it's only missing a listener on timed in calendarworker to setTimezone() on the calendar and rerun range occurrence calculations. | 08:13 |
| pvuorela | dcaliste: i tried to investigate that years ago, but back then the problem seemed to go deep. but now that we don't have anymore kdatetime and potentially this whole timezone thing in the storage backend class, it could be quite a different thing. | 08:16 |
| dcaliste | I've a good feeling for it ;) I'll test it this week. Few years back then, I also corrected a bug upstream that the cache per date was not updated in the calendar when calling setTimezone(). All in all, as you said, it should be smoother now. | 08:19 |
| dcaliste | About the timezone PR, no problem to iron out the API removal one before. | 08:19 |
| dcaliste | What do you think about the idea of moving (with deprecation first not to break all code) notebook routines from storage to calendar ? | 08:23 |
| pvuorela | which ones? | 08:31 |
| pvuorela | dcaliste: a small remark on the mkcal api removal pr. overall seems quite good and thinking of merging soon. | 08:38 |
| dcaliste | Thanks, I'm fixing the PR according to your remarks. | 08:41 |
| dcaliste | About moving the notebook API from storage to calendar, I'm referring to this comment https://github.com/sailfishos/mkcal/pull/53#issuecomment-1455859669, the second part. I plan to deprecate all notebook related methods, so storage is just a sophisticated serializer for a calendar. The calendar would become the holder of the notebooks and all the related API. | 08:42 |
| pvuorela | dcaliste: hm, could be that they would fit the calendar, but that also sounds like a bit of code churn if all notebook related code needs to be adjusted. i'm not sure how much there's benefit of such move. | 08:52 |
| dcaliste | It's cleaner, that's the main benefit ;) There is also another reason (but admettedly a bit far fetch): | 08:59 |
| dcaliste | One day, KCalendarCore may remove the (limited) notebook support they have in Calendar class. It's conflicting with the fact that Calendar API is going in the direction of 1:1 with an ical calendar. | 09:00 |
| dcaliste | To circumvent this, the idea is to modify ExtendedCalendar to become a dictionnary of MemoryCalendar, one for every notebook. | 09:01 |
| dcaliste | Instead of inheriting from a single memory calendar. | 09:01 |
| dcaliste | It would not change anything from outside, and there will still only one DB as we know it at the moment, but internally the incidences would be stored a bit differently. | 09:02 |
| dcaliste | mKCal::Notebook would just be additional metadata to existing Calendar metadata. | 09:03 |
| dcaliste | For this to work, client code need to get these metadata from the calendar class though. | 09:04 |
| dcaliste | Which in my opinion makes more sense than reading them directly from the DB. | 09:05 |
| pvuorela | dcaliste: we should do something for more separate notebooks (i.e. events not in single pool, one uid only in one notebook), but i'm not necessarily immediately thrilled on adjusting this. sure can consider better with a PR, but anyhow. | 09:06 |
| dcaliste | Yeh, the problam is that client code would have to search and replace everywhere mStorage->fooNotebook() with mCalendar->fooNotebook(). It's a bit painful, but quite straight forward. | 09:08 |
| dcaliste | pvuorela, tell me if there are some problem arising on the test farm with the buteo caldav plugin. Tests are passing on my device, but the test farm maybe a bit different for whatever reason. | 09:11 |
| pvuorela | dcaliste: sure. now it was all failure due to not having the blts-tool available. | 09:11 |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!