| direc85[m] | usere_: It's "a bit" counter-intuitive, but removing busybox-symlinks-bash with pkcon (maybe with zypper too?) indeed pulls bash in. | 06:26 |
|---|---|---|
| pvuorela | dcaliste: hey | 11:56 |
| dcaliste | Hello pvuorela. Thank you for the reviews. | 11:57 |
| pvuorela | phew, quite a bunch of changes there. | 11:57 |
| dcaliste | Yes. All a bit related to the same problem : migrating to support event sharing the same UIDs in different notebooks, or let say, dropping the notebook support from KCalendarCore. | 11:59 |
| dcaliste | Let start with the latest, you would agree to change QML API to move to a true unique identifier for events ? I share the fact that it may be better to find a more suitable name to avoid confusion with the event uid. | 12:01 |
| pvuorela | yes, overall that seemed good. small details then whether to support getting events with uid+recid etc, but for basic models and so on the single identifier seemed good. also the smaller PR appeared fine. didn't do any testing yet but maybe merging that soonish. | 12:02 |
| dcaliste | Moving to a unique id to store events in hash simplifies code quite much in worker and manager, even at UI level. But changing QML API requires to rely on grep which is less efficient to fine mistakes than compilers :/ | 12:05 |
| dcaliste | But I'm happy you support the idea. It will simplify the transition from Extended* to MultiCalendarStorage API (see mkcal#61). You asked me what is the plan for Extended*. | 12:06 |
| dcaliste | I will drop them completely. | 12:06 |
| dcaliste | From a API point of view, they are broken by the fact that events can share uid between notebooks. | 12:07 |
| pvuorela | alright. | 12:07 |
| dcaliste | Moving to a truly unique identifier allow to have a very simple load() API in MultiCalendarStorage: | 12:07 |
| pvuorela | this just in :D https://github.com/sailfishos/nemo-qml-plugin-calendar/pull/55 | 12:08 |
| dcaliste | load(QDate, QDate), load(TrueUniqueId) and search() | 12:08 |
| dcaliste | nemo-qml-plugin-calendar#55 is interesting. It's better without the logging_p.h though… | 12:09 |
| pvuorela | assuming that's some copy-paste error. now in. | 12:11 |
| dcaliste | Going a bit backward, yes, I plan to remove Extended*, replacing them with two new classes (visible already in mkcal#61): | 12:18 |
| dcaliste | - CalendarStorage (and its SqliteCalendarStorage implementation), that is a CalStorage implementation, so linking a storage (here multi notebook, but we don't care thanks to the backend separation) with a KCalendarCore::MemoryCalendar. | 12:20 |
| dcaliste | - MultiCalendarStorage (and its SqliteLMultiCalendarStorage implemenation), that is NOT a CalStorage implementation, but provides access to several MemoryCalendars based on notebook UID. | 12:20 |
| dcaliste | The first class is designed for Buteo sync plugins, with an API allow to fetch incidences based on UIDs or deleted incidences. | 12:22 |
| dcaliste | The second class is design for multi notebook access with an API fetching incidences based on time ranges, search or unique identifiers. It is specifically designed for the QML plugin. | 12:22 |
| dcaliste | The Buteo CalDAV PR is an example of usage of the first class. | 12:23 |
| dcaliste | There is no PR at the moment for the QML plugin, because it required some preliminary changes, like the service PR that you merged recently and the one on unique identifier API that we were discussing. | 12:24 |
| dcaliste | With both PRs, the change from Extended* to MultiCalendarStorage should be more or less direct. | 12:25 |
| pvuorela | should help then for the code duplication if we can eventually get rid of the Extended* | 12:26 |
| pvuorela | which is nice. | 12:27 |
| dcaliste | I've seen that you mentioned that ExtendedCalendar::defaultStorage() was a convenient entry point. I'll see what I can do. | 12:27 |
| dcaliste | Indeed, the code duplication is (normally) only because of ExtendedStorage. | 12:27 |
| pvuorela | good, thanks. | 12:27 |
| dcaliste | I still keep it otherwise nothing compile for the moment since too many projects still use ExtendedStorage and are not yet migrated. | 12:28 |
| pvuorela | indeed | 12:29 |
| dcaliste | Do you think it's better to have a commit changing MKCAL_EXPORT to use Q_DECL_EXPORT / IMPORT instead ? | 12:40 |
| pvuorela | i think it would be better following the common convention. | 12:40 |
| pvuorela | but no big deal of defining it as it does now. just that the _HIDE seems unnecessary. | 12:41 |
| dcaliste | Yes, ok, I see. But since I'm in the process of writing new classes, it's better to use the usual convention. | 12:43 |
| pvuorela | sure. that could be also a change to get out of the way before the actual notebook separation. | 12:44 |
| pvuorela | dcaliste: created JB#60800 for notebook api migration. | 12:46 |
| dcaliste | Ok, thanks, I'm going to add it where necessary. I've tagged two commits of mkcal#60 with this bug id. | 12:50 |
| pvuorela | merged now the first nemo-calendar pr. | 12:59 |
| dcaliste | Great, thanks. I'm going to open PR in jolla-calendar and jolla-email to go with the second PR. I'll change the uid name also not to be misleading with event uid and try to see how to backwardly be compatible for alarms… | 13:02 |
| pvuorela | nice. | 13:04 |
| pvuorela | hey and to note, i'll be off now rest of the week. | 13:05 |
| dcaliste | Oh, great. Enjoy your days off then ! | 13:06 |
| dcaliste | I've just pushed to mkcal#60 an additional commit that moves all MKCAL_EXPORT to Q_DECL_EXPORT. I've kept it there, because it's a PR that is not breaking any API. It works under the hood dissociating DB access from memory storage but it should not introduce API change. | 13:08 |
| dcaliste | Ah, maybe one question if you still have the time : in the single notebook storage class, I've named "erase()" the routine replacing "deleteNotebook()". I agree that it's not erasing anything in a multi notebook backend. But keeping in mind the mono-notebook API of a CalendarStorage, how could it be named ? | 13:11 |
| dcaliste | Because, even if the actual implementation is to remove the notebook for the DB, from the CalendarStorage class, this call is equivalent to a close() with permanent deletion of all incidences and of the metadata of the notebook. | 13:11 |
| dcaliste | In a single notebook database implementation, it would be implemented with the db file removal. | 13:12 |
| pvuorela | wondering if it's just possible to have deleteNotebook() type of api somewhere. | 13:22 |
| pvuorela | don't think we necessarily need removing the whole databse, at least for now. | 13:23 |
| dcaliste | Ok, indeed, why not delegating this to something out of the class. I've added static Notebook::systemNotebooks() to be able to get the notebook list in sync plugins for instance. Why not adding another static Notebook::deleteSystemNotebook(uid) ? | 13:27 |
| dcaliste | (in the same way that there is for instance QTimeZone::systemTimeZone() to get the default system one). | 13:28 |
| pvuorela | dcaliste: yea indeed. not sure should those be in notebook, but otherwise good. | 13:41 |
| pvuorela | not entirely bad either | 13:41 |
| pvuorela | now -> | 13:42 |
| dcaliste | Enjoy your vacation. Thanks a lot for the reviews, discussions and merges. | 13:42 |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!