| *** marja is now known as Guest2772 | 00:49 | |
| dcaliste | Good morning pvuorela, sorry for the delay. | 08:11 |
|---|---|---|
| pvuorela | dcaliste: good morning, np. | 08:12 |
| dcaliste | Thank you for the various merges of last week. | 08:13 |
| dcaliste | Upstream approved yesterday night the PR adding notebook support in ICalFormat loaders, see https://invent.kde.org/frameworks/kcalendarcore/-/merge_requests/98 | 08:14 |
| pvuorela | that was again quite a bunch of things. thank you yourself! | 08:15 |
| dcaliste | I think I'll merge it later today and create a PR in SailfishOS to sync with upstream. | 08:15 |
| pvuorela | nice | 08:15 |
| dcaliste | About the recent PRs, I'm quite happy with this local restructuring. Even if the switch to KCalendarCore is not yet possible in the bindings it has bring some interesting side effects. | 08:16 |
| pvuorela | indeed. | 08:17 |
| dcaliste | Regarding the PR on decreasing the ExtendedStorage dependency from ExtendedCalendar to Calendar, the upstream MR will help on one side, but there is still this different behaviour between upstream and ExtendedCalendar about adding incidence and setting its notebook. | 08:20 |
| dcaliste | I thought about it all the week-end, and finally discard the initial implementation proposed last Friday using the default notebook in calendar. | 08:20 |
| dcaliste | This will not work with upstream calendars because they use a different way to set the notebook and I don't think they would change it… | 08:21 |
| dcaliste | So, the only other possibility is to change the way we do it in ExtendedCalendar. | 08:21 |
| dcaliste | That's the latest PR : https://github.com/sailfishos/mkcal/pull/24 | 08:21 |
| dcaliste | It will breal all calls to addEvent(foo), but not addEvent(foo, notebookUid). | 08:22 |
| dcaliste | One should use addEvent(foo, {}) (which is equivalent to addEvent(foo, defaultCalendar()) to get back the old behaviour. | 08:23 |
| dcaliste | I think that in most places code was like addEvent(foo, notebookUid), because ExtendedCalendar was using this two argument method and the second is not optional. But I'm not completely sure. | 08:24 |
| pvuorela | i'll check that pr in more detail. a bit pondering how much we benefit in generalizing the calendar class requirement, though all the better the more we can get rid of extra things there. | 08:25 |
| pvuorela | the extendedcalendar appears to have much code we shouldn't really need. | 08:25 |
| dcaliste | Beside deprecating ExtendedCalendar by itself which would save some good portions of code from mKCal that we're not using indeed, I wondering about implementing a calendar that is not based on MemoryCalendar. It's still an idea at the moment, and I only look very quickly at how it would be feasible, but I would like to implement this very thin memory-free calendar that would just be a proxy in the worker thread between the manager and the storag | 08:28 |
| dcaliste | e. | 08:28 |
| pvuorela | yep. | 08:29 |
| dcaliste | Because with the inheritence to MemoryCalendar, it seems to me that the worker thread is storing all incidnces without never usig them actually beside passing them to the manager when needed. | 08:29 |
| dcaliste | So beside the exercise of making storage code less dependant on external code (like the ExtendedCalendar one) and making things more isolated, it may have a practical usage. Well may have… | 08:31 |
| pvuorela | i'm maybe having here problems on imagining what a "non-memory" calendar would be. sort of if there's someting loaded there should be always something in memory. also on kcalendarcore how is memorycalendar not a storage. | 08:35 |
| pvuorela | i.e. why is that even memorycalendar and not calendar + memorystorage. | 08:35 |
| dcaliste | About the second part, MemoryCalendar could have been a storage in a general sense indeed, but when you look at the CalStorage API (load(), save()…) it makes sense that it is not a storage but a calendar. | 08:36 |
| dcaliste | About the first part, my idea of a calendar proxy is this one: | 08:37 |
| dcaliste | - the manager asks for incidence in a range, | 08:37 |
| dcaliste | - the SqliteStorage in the thread loads it from DB, | 08:38 |
| dcaliste | - and call addIncidences() on its proxy calendar, | 08:38 |
| dcaliste | - this calendar is storing these incidences in a list or a hash, | 08:38 |
| dcaliste | - and when storage finishes loading, it is emitting "dataLoaded" and it clears the list or hash after the signal emission. | 08:39 |
| dcaliste | - the manager during the signal emission is taking ownership of the data. | 08:39 |
| dcaliste | The same on save: | 08:39 |
| dcaliste | - the manager passes incidences to be saved, like now, | 08:40 |
| dcaliste | - the proxy calendar is emitting incidenceAdded() with these incidences (or incidenceUpdated()), so the sqlitestorage is adding then in its internal list of things to be saved, | 08:41 |
| dcaliste | - then the thread is calling storage->save(), as now. | 08:41 |
| dcaliste | At no moment the proxy is storing anything, only transiently during calls maybe. | 08:42 |
| dcaliste | Actually, if you look at the Calendar interface, not all methods would have sense with this proxy stuff, but actualt the worker thread and the sqlite storage are using a very resticted set of methods that could be implemented with minimal storage I think. | 08:43 |
| pvuorela | on the kf5 side, the CalStorage api is indeed quite limited. that got me thinking why does it even exist. kf5-calendarcore defines the CalStorage, inherits FileStorage out of it but otherwise doesn't seem to have any references there(?) | 08:44 |
| dcaliste | Indeed, I don't know the details and only guess them from the code, but it seems that when KDE moved to Akonady for storage, all the real storage implementation of KCal was separated into mKCal and the rest became KCalCore. | 08:45 |
| dcaliste | Later one mKCal was even thined further by removing the iCal file storage backend, leaving only the SQLite one. | 08:46 |
| dcaliste | I've promised myself to look into KDE nowadays to see if they even actually have code that inherit from CalStorage or not, but didn't do it up to now. | 08:48 |
| pvuorela | in theory guess we could even drop the CalStorage inheritance on ExtendedStorage | 08:48 |
| dcaliste | Yes, but having the calendar linked to the storage is almost a requirement in the sense that storage is not emitting any loaded or saved signals but rely on working on the calendar directly (it's a calendar observer for instance). | 08:50 |
| dcaliste | There are numerous places where calendar() is used from storage in the code. | 08:51 |
| pvuorela | yes, that part would need reimplementation if the inheritance is dropped. | 08:51 |
| pvuorela | but trivial | 08:52 |
| dcaliste | Indeed, it's just a member addition. But why not keep the inheritence after all, since it is only doing the member addition by itself… From the diagram that is in extendedcalendar.cpp, the ideal situation is to speak to a calendar and let the calendar interface itself with a storage backend. | 08:54 |
| pvuorela | what i started thinking is if we could have storage and the calendar living in different threads. | 08:55 |
| dcaliste | I think about this first, but I think it is complicated in the current way of storage to add element in the calendar. | 08:56 |
| dcaliste | I mean, it is doing it in sync, by calling addIncidence() directly at any moment. | 08:56 |
| dcaliste | Which means that the storage (hash table for instance) for them cannot be in the UI thread, but must be in the storage thread. | 08:57 |
| dcaliste | This thought brings me to the conclusion that the calendar (in the KCalendarCore sense) must be in the storage thread. | 08:57 |
| dcaliste | Which leads to the fact that this calendar should just be a passing way between the storage thread and the UI one, storing transiently the data to handle asynchronous behaviour. | 08:58 |
| pvuorela | not saying it would be easy. but e.g. on load side instead of addIncidence() there could be a batch accumulated and then something for syncing up. | 09:07 |
| pvuorela | and not saying either if it would be a good idea, just that if the whole setup is refactored there could be alterantives. | 09:07 |
| dcaliste | Indeed, that was my crude idea about the proxy stuff, but why not already make this architecture directly in the storage implementation… Interesting. I'm going to look further into that direction. | 09:08 |
| pvuorela | in a way it could be nice if the api was such that it would be possible to implement UI applications without needing to implement data conversions and proxies and whatnot. | 09:09 |
| dcaliste | On the saving part, there already is this accumulation in the storage before actually performing the saving. | 09:09 |
| dcaliste | I fully agree about your remark on UI. | 09:10 |
| dcaliste | That would be great for this to be transparent and just use the calendar API (as it was designed in the diagram from extendedcalendar.cpp in fact). | 09:10 |
| pvuorela | one concern, though, on that direction is if the api is having potenatially blocking or slow operations. | 09:14 |
| dcaliste | The finished() signal in storage observer may be of interest here in the purpose of not adding loaded incidences in sync but accumulating them and letting the obser (meaning the calendar) do the add action when it is ready to do it. Mmh, looks like interesting change here… | 09:14 |
| dcaliste | Indeed, any change should be done with UI thread being never blocked. | 09:14 |
| dcaliste | All loading and saving operation should be async and concluded with a closure (signal or observer method) to let the UI code decide what to do with the result. | 09:15 |
| dcaliste | I got two questions about a comment you made, pvuorela, in the KCalendarCore PR in bindings: | 09:23 |
| dcaliste | - you mentioned an issue when moving event from one notebook to another, may you detail it ? | 09:23 |
| dcaliste | - you also noted that you would like to have the possibility to attach one event to several notebooks. What would be the use case ? | 09:24 |
| pvuorela | yea, so we've never supported moving an event from a notebook to another. when you create an event you select the notebook and it stays there. that's silly but there are reasons. | 09:25 |
| dcaliste | Indeed, but besides some technical hickups, what are the reasons ? Maybe they are just these hickups ! | 09:26 |
| pvuorela | and the reason being that now if you delete an event it stays but gets the "deleted" bit set on sql. thus the sync code can detect it's removed. but if we just change the notebook there's no way to detect the removal. | 09:26 |
| dcaliste | Mmh, why ? | 09:26 |
| pvuorela | and for one event in multiple notebooks, a person could have multiple accounts which received invitation to the same event. if those all just added the event to the calendar the events would collide. thus the sync implementations add some dummy part. | 09:27 |
| dcaliste | I thought about it myself, and see that it made sense to signal to the sync plugins for instance that the event was deleted (from initial notebook), so the deletion can be propagated also. | 09:27 |
| pvuorela | so the sync implementations now fetch the deletions with ::deletedIncidences(...). if a notebook just changed, there's nothing deleted. | 09:30 |
| dcaliste | If you look at implementation in calendarworker.cpp#277, the event will be marked as deleted but not purged. | 09:30 |
| dcaliste | So it should be listed by a call to deletedIncidences(). | 09:30 |
| pvuorela | yes, if the incidene is deleted. but not if the notebook was changed | 09:31 |
| dcaliste | line #277 is for notebook change, as far as I can tell. | 09:31 |
| dcaliste | The logic is : | 09:31 |
| dcaliste | - delete event from initial notebook with mCalendar->deleteEvent(event); | 09:31 |
| dcaliste | - clone the event and add it (with a new UID) to the final notebook, with mCalendar->addEvent(newEvent, notebookUid); | 09:32 |
| dcaliste | For me, this looks functional. Note that I didn't test it actually, so I'm surely missing something. | 09:33 |
| pvuorela | dcaliste: disabled in the ui and changing the uid is a workaround anyhow. | 09:34 |
| dcaliste | Yes, I know that it is disabled in the UI, but for a long time I didn't see why. I thought it was linked to syncing issues, but I couldn't find any problem actually. And since you mentioned it I was wondering. About the UID change, thinking about it, that is an issue for sync. I agree. | 09:37 |
| dcaliste | And not changing the UID will transform the (delete + add with the same UID) into an update. Which is not what we want. | 09:38 |
| pvuorela | to me the whole kf5 calendar feels a bit broken when the incidences are defined by uid+recid, not notebook+uid+recid. | 09:38 |
| dcaliste | I agree. But sadly, upstream is moving more and more in the direction of one ::Calendar for one notebook (<=> iCal resource). For instance, they added all (name, uid, color, owner…) at the level of ::Calendar. So notebook inside ::Calendar is just a way of labelling incidences inside a single iCal resource. | 09:41 |
| dcaliste | This is not fitting us well, because having one calendar for multi iCal resource is a very nice abstraction, and convenient at the UI level. | 09:42 |
| pvuorela | at least it makes the api saner on some parts if calendar == notebook. but on that direction we'd need then something as container of Calendars. | 09:44 |
| dcaliste | Exactly, but then if you think in term of this container and method of this container, you almost end-up to the calendar API. And then I asked myself, "but why having an intermediate level then ?" | 09:46 |
| dcaliste | The only differences in the API would be that all new calendar-metadata methods would have no sense on the container. | 09:47 |
| dcaliste | So maybe it would be good to have a base calendar class without meta-data, but abstracting method that are usefull on a calendar (generally in term of the sheet of paper pin point on the fridge), and then an iCal resource that adds the meta data, and a container that implement calendar with internally a list of iCal resource calendars. | 09:49 |
| dcaliste | But it's far too many changes to be accepted upstream directly. | 09:49 |
| pvuorela | yea, not sure how that mess should be really solved. | 09:50 |
| dcaliste | As long as upstream is not removing the notebook parts from KCalendarCore API we're quite safe. But to go back to the issue of switching notebook, I think I see that the main issue is not to change the UID and make mKCal understand that it's a delete and an addition. | 09:51 |
| dcaliste | I'll try to see if this can be done in a clean way… | 09:51 |
| pvuorela | dcaliste: anything else? could wrap this up. | 09:59 |
| dcaliste | Yes, that's all. Thank you for the fruitfull direction. I think that using the storage observer signal to let the calendar do the addIncidence() on the way it is suiting it is a very nice idea. | 10:00 |
| pvuorela | cheers. i'll head to get some lunch now. | 10:01 |
| dcaliste | Enjoy your meal. | 10:01 |
| *** fifr is now known as Guest2839 | 10:03 | |
| *** attah_ is now known as attah | 11:08 | |
| *** ChanServ changes topic to "https://www.sailfishos.org | Developers: https://sailfishos.org/wiki | Community: https://sailfishos.org/community | Logs: https://irclogs.sailfishos.org/logs/%23sailfishos" | 11:56 | |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!