Rework ExtendedStorage API and service plugin API to remove any Notebook::Ptr #30

dcaliste · 2022-04-14T09:26:32Z

@pvuorela, this is a proposition to remove completely the Notebook::Ptr style, using either notebook ids or reference to notebooks.

This is done in steps:

commit a498d17, keep API compatibility and use internally pointers (out) or references (in) at SqliteFormat level. The ownsership is taken by QSharedPointer() in SqliteStorage.
commit 252bf4f, keep API compatibility and move notebook modifications (UID) out of ExtendedStorage so it can use const references on Notebooks later.
commit f675e67, update ExtendedStorage API not to use Notebook::Ptr anymore. Internal storage is done with Notebook objects and they are copied on retrieval. I've changed also the ::deleteNotebook() API to take id instead of object. The same for the ::defaultNotebook(), it's now dealing with an id.
commit 6d1128b, completely nuke Notebook::Ptr and Notebook::List definition from the API. It requires in addition to update all service plugins.

What do you think ? Applying these API changes will imply to update:

nemo-qml-plugin-calendar, but only in the worker thread code,
buteo-sync-plugin-caldav, nothing painfull,
all Exchange plugins, no idea how painful it can be.

The changes in ExtendedStorage API will correct some weirdness (in my opinion) like the fact that you need to use the exact Notebook::Ptr object when updating, while any change to the DB file is actually detaching your Ptr from the one that is stored, thus making any update call fails.

pvuorela · 2022-04-21T12:44:16Z

src/notebook.h

    /**
      Constructs a new Notebook object.
    */
    explicit Notebook();

-    explicit Notebook(const QString &name, const QString &description);
+    explicit Notebook(const QString &name, const QString &description,


Old thing, but the explicit keyword doesn't look needed in any of these. Remove if you feel like it.

Ok, I've removed them.

pvuorela · 2022-04-21T12:49:53Z

src/extendedstorage.cpp

-    }
-
-    if (!nb || d->mNotebooks.contains(nb->uid())) {
+    if (d->mNotebooks.contains(nb.uid())) {


Should we guard for notebook validity somewhere around here?

As you mentioned later, isValidNotebook() is more about testing if notebook is writable. But that's a good remark anyway, and I've added a commit that guard against writing to the database runtime only notebooks.

Meant here more Notebook::isValid() type of validity check. Now it looks like this will proceed adding a notebook without a uid.

pvuorela · 2022-04-21T13:01:09Z

src/extendedstorage.cpp

-    if (!nb.isNull()) {
-        if (nb->isRunTimeOnly() || nb->isReadOnly()) {
+    const Notebook &nb = notebook(notebookUid);
+    if (nb.isValid()) {


This is an interesting method. Could assume it's the same as notebook(uid).isValid() but it's not exactly. Three cases using it, pondering if it's needed or should the name be adjusted a little, e.g. notebookWritable() or something to that direction.

You're right, I've renamed the method to isWritableNotebook(). I makes more sense, I agree.

pvuorela · 2022-04-21T13:08:04Z

src/sqlitestorage.cpp

        qCDebug(lcMkcal) << "Storage is empty, initializing";
-        createDefaultNotebook();
+        if (!setDefaultNotebook(Notebook(QString::fromLatin1("Defaul"), QString()))) {


pvuorela · 2022-04-21T13:14:46Z

src/sqlitestorage.cpp

@@ -1800,7 +1807,7 @@ bool SqliteStorage::loadNotebooks()
    sqlite3_stmt *stmt = NULL;
    bool isDefault;

-    Notebook::Ptr nb;
+    Notebook *nb;


Minor detail, but I could move to the while() declaration to keep the scope minimal.

As you suggested, I've turn the SqliteFormat method into returning a Notebook and not a pointer to one. I've updated here accordingly.

pvuorela · 2022-04-21T13:16:33Z

src/sqliteformat.cpp

@@ -1298,10 +1340,10 @@ bool SqliteFormat::Private::insertCalendarProperty(const QByteArray &id,
 }
 //@endcond

-Notebook::Ptr SqliteFormat::selectCalendars(sqlite3_stmt *stmt, bool *isDefault)
+Notebook* SqliteFormat::selectCalendars(sqlite3_stmt *stmt, bool *isDefault)


Hm, does the pointer return value save much when the result gets deleted anyway? Perhaps could alternatively just return a Notebook instance which wouldn't need the deletion.

I agree, it's now returning a Notebook instance. instead of a pointer on a Notebook instance.

dcaliste · 2022-04-27T13:37:31Z

I'm propagating changes in CalDAV plugin at the moment and this is raising a concern : there could be cases where the notebook is read-only, but we would like to get the upstream changes. The current pattern is to get the corresponding Notebook::Ptr and to make it read/write, do the calendar modifications and save them. This is working without touching the read-only flag status in the DB itself because we're dealing with a pointer to the internal Notebook in the ExtendedStorage object and calling the isValidNotebook() in sqlitestorage.cpp will return true, even if the DB flag means read-only. The same scheme is done in the birthday plugin in contactsd for instance.

Now changing for Notebook references make the whole pattern to fail. Except if we flip the flag in the DB itself with a call to ExtendedStorage::updateNotebook() and set it back after changes. This is very ugly though and creates possibilities of race conditions where the UI reports writable birthday calendar during the modifications for instance...

What do you think about removing the || isReadOnly() from isValidNotebook() ? The read-only flag would only be used as a hint for UI operations, but not enforced at the DB where we can still do all desired modifications when calling save() ? I may rename the isValidNotebook() to isStoredNotebook() or something to reflect this ?

dcaliste · 2022-04-27T13:51:30Z

Maybe an additional question regarding API changes : since we're not dealing with pointers anymore, I've found the need for a ExtendedStorage::containsNotebook() method, to know if we should call addNotebook() or updateNotebook(). One can still use mStorage->notebook(mNotebook.uid()).isValid() but it is overly verbose in my opinion and it requires a useless copy. But thinking more about it, we may drop addNotebook() and updateNotebook() completely and create a new storeNotebook() which internally update or add the notebook, depending if it already exists or not. What do you think ?

pvuorela

What do you think about removing the || isReadOnly() from isValidNotebook() ? The read-only flag would only be used as a hint for UI operations, but not enforced at the DB where we can still do all desired modifications when calling save() ? I may rename the isValidNotebook() to isStoredNotebook() or something to reflect this ?

Makes sense removing the isReadOnly() check. Better consider that more as a hint towards application code. It's strange usage now when readonly status gets changed without updateNotebook() and it's assumed effective.

Maybe an additional question regarding API changes : since we're not dealing with pointers anymore, I've found the need for a ExtendedStorage::containsNotebook() method, to know if we should call addNotebook() or updateNotebook().
[...] But thinking more about it, we may drop addNotebook() and updateNotebook() completely and create a new storeNotebook() which internally update or add the notebook, depending if it already exists or not. What do you think ?

The containsNotebook() could nicely fit the API, I'm ok with such. For latter I do sort of like if the additions and updates are separated for keeping it clearer which one is happening.

All in all this is looking mostly fine, but haven't tested much beyond compilation. Would need some api adaptation PR at least for nemo-calendar. Suppose that's coming up too?

pvuorela · 2022-05-02T13:20:01Z

src/extendedstorage.cpp

-    }
-
-    if (!nb || d->mNotebooks.contains(nb->uid())) {
+    if (d->mNotebooks.contains(nb.uid())) {


Meant here more Notebook::isValid() type of validity check. Now it looks like this will proceed adding a notebook without a uid.

pvuorela · 2022-05-02T13:30:21Z

src/extendedstorage.cpp

    }
+    const Notebook &nb =  d->mNotebooks.value(nbid);


Nit: extra space after =

dcaliste · 2022-05-09T14:27:37Z

It's strange usage now when readonly status gets changed without updateNotebook() and it's assumed effective.

Exactly, I've removed it's usage. I took the opportunity to actually completely remove isWritableNotebook() from the API and implement its behaviour where it was used :

in sqlite storage, it's just checking runtime status and existing associated notebook (if validating),
in service handler, to keep backward compatibility, it's checking runtime status and read-only one.

Would need some api adaptation PR at least for nemo-calendar. Suppose that's coming up too?

I didn't try it yet in QML bindings, but you can see it in action for buteo plugin : see sailfishos/buteo-sync-plugin-caldav#11

dcaliste · 2022-05-10T08:17:44Z

Since we're already modifying the API in servicehandler.h, I've added a commit 9e34637 that is simplifying the [send/update]Invitation() functions. The ExtendedCalendar::Ptr, ExtendedStorage::Ptr and Notebook::Ptr arguments were just there to get the Notebook object, either directly if the invitation was not part of the calendar, or by a fetch in the storage if the invitation was in the calendar. This can be simplified by only passing the notebook id and the storage. I've updated the nemo-qml-plugin-calendar PR accordingly.

@pvuorela do you know if there are other places where this ServiceHandler::send/updateInvitation API is in use ?

pvuorela · 2022-05-17T08:20:56Z

src/sqlitestorage.cpp

-            (*savedIncidences) << *it;
+        if (dbop == DBInsert || dbop == DBUpdate) {
+            const Notebook &notebook = mStorage->notebook(notebookUid);
+            // Nota here : allow to save/delete incidences in a read-only


Nit: "nota" sound like a typo.

It was a contraction of latine "nota bene" ;) Maybe a bit over contracted actually. I've updated for a more English "notice:" construction.

pvuorela · 2022-05-17T09:21:52Z

src/sqlitestorage.cpp

@@ -1169,6 +1171,8 @@ int SqliteStorage::Private::loadIncidences(sqlite3_stmt *stmt1,
            //                  << incidence->dtStart() << endDateTime
            //                  << "in calendar";
            count += 1;
+        } else {
+            delete incidence;


Is the above break; case leaking memory now since selectComponents() return value is owned by the caller? Pondering should it still just return Incidence::Ptr since incidences are going to end up as such and there are these cases where it's ignored.

You're right. I've reverted these changes at the moment. Ideally it's in preparation of SqliteStorage not assuming that loaded incidences will be used in the same thread. But these changes can be done at that moment.

src/extendedstorage.cpp

Don't assume that incidences are shared pointer when it's not necessary, like in internal database reads.

…o JB#58152

Use direct reference to ExtendedStorage when there is no intent to keep a pointer on this objects. Use the notebook uid as argument instead of passing the calendar to get it.

…ebooks

dcaliste · 2022-08-16T09:51:21Z

I've fixed the conflict and checked that it compiles and pass the tests.

pvuorela · 2023-01-25T13:21:35Z

With the simpler notebook api change in, think we could close this now.

pvuorela reviewed Apr 21, 2022

View reviewed changes

dcaliste force-pushed the noptr branch from 6d1128b to 2f1561d Compare April 27, 2022 12:15

pvuorela reviewed May 2, 2022

View reviewed changes

dcaliste force-pushed the noptr branch from 2f1561d to 26b9a61 Compare May 9, 2022 13:36

dcaliste force-pushed the noptr branch from 26b9a61 to a0304f1 Compare May 10, 2022 08:12

This was referenced May 10, 2022

[contactsd] Follow API changes on mKCal::Notebook. sailfishos/contactsd#3

Closed

[buteo-sync-plugins-social] Update following Notebook::Ptr removal. sailfishos/buteo-sync-plugins-social#5

Closed

dcaliste force-pushed the noptr branch from a0304f1 to ad03549 Compare May 17, 2022 08:09

pvuorela reviewed May 17, 2022

View reviewed changes

dcaliste force-pushed the noptr branch from ad03549 to 5d76c1b Compare May 17, 2022 08:35

pvuorela reviewed May 17, 2022

View reviewed changes

dcaliste force-pushed the noptr branch from 5d76c1b to d37cdfb Compare May 17, 2022 12:18

pvuorela approved these changes May 17, 2022

View reviewed changes

llewelld reviewed May 24, 2022

View reviewed changes

src/extendedstorage.cpp Show resolved Hide resolved

dcaliste added 8 commits August 16, 2022 11:23

[mkcal] Remove non implemeted notebook from a list.

730d24f

[mkcal] Use references or pointers in SqliteFormat.

8988793

Don't assume that incidences are shared pointer when it's not necessary, like in internal database reads.

[mkcal] Move notebook UID creation to notebook class.

1d3cee8

[mkcal] Use notebook references in ExtendedStorage API. Contributes t…

bcd452b

…o JB#58152

[mkcal] Update service plugin API non to use Notebook::Ptr.

4c5904b

[mkcal] Remove ::Ptr from servicehandler API.

cbd4381

Use direct reference to ExtendedStorage when there is no intent to keep a pointer on this objects. Use the notebook uid as argument instead of passing the calendar to get it.

[mkcal] Protect against notebook storage changes for runtime only not…

cc9d8ca

…ebooks

[mkcal] Remove isValidNotebook() from public API.

697643e

dcaliste force-pushed the noptr branch from d37cdfb to 697643e Compare August 16, 2022 09:49

dcaliste mentioned this pull request Oct 11, 2022

Protect against notebook storage changes for runtime only notebooks and other clean ups #34

Merged

pvuorela closed this Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework ExtendedStorage API and service plugin API to remove any Notebook::Ptr #30

Rework ExtendedStorage API and service plugin API to remove any Notebook::Ptr #30

dcaliste commented Apr 14, 2022

pvuorela Apr 21, 2022

dcaliste Apr 27, 2022

pvuorela Apr 21, 2022

dcaliste Apr 27, 2022 •

edited

pvuorela May 2, 2022

pvuorela Apr 21, 2022

dcaliste Apr 27, 2022

pvuorela Apr 21, 2022

dcaliste Apr 27, 2022

pvuorela Apr 21, 2022

dcaliste Apr 27, 2022

pvuorela Apr 21, 2022

dcaliste Apr 27, 2022

dcaliste commented Apr 27, 2022

dcaliste commented Apr 27, 2022

pvuorela left a comment

pvuorela May 2, 2022

pvuorela May 2, 2022

dcaliste May 9, 2022

dcaliste commented May 9, 2022

dcaliste commented May 10, 2022 •

edited

pvuorela May 17, 2022

dcaliste May 17, 2022

pvuorela May 17, 2022

dcaliste May 17, 2022

dcaliste commented Aug 16, 2022

pvuorela commented Jan 25, 2023

Rework ExtendedStorage API and service plugin API to remove any Notebook::Ptr #30

Rework ExtendedStorage API and service plugin API to remove any Notebook::Ptr #30

Conversation

dcaliste commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcaliste Apr 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcaliste commented Apr 27, 2022

dcaliste commented Apr 27, 2022

pvuorela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcaliste commented May 9, 2022

dcaliste commented May 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcaliste commented Aug 16, 2022

pvuorela commented Jan 25, 2023

dcaliste Apr 27, 2022 •

edited

dcaliste commented May 10, 2022 •

edited