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

[dcaliste]

@pvuorela, this is a selection of commits from #30, commits that don't change the API and may be reviewed independently.

Once we agree on this I'll make another commit with (simple) API break, like:

• remove completely the isValidNotebook() as we discussed in Rework ExtendedStorage API and service plugin API to remove any Notebook::Ptr #30. It requires a fix in the contact birthday plugin and in the nemo QML calendar plugin, as far as I know.
• change the deleteNotebook() API to use a notebook uid and not a notebook pointer.
• change the defaultNotebook() and setDefaultNotebook() to use uid instead of a notebook pointer.

These API breaks are still less invasive than completely suppressing the ::Ptr part for Notebooks, but they make the ExtendedStorage API simpler and don't require to load Notebook::Ptr from outside mKCal when uids are enough.

What do you think ?

[dcaliste]

@pvuorela, I've added a commit where I stopped using createDefaultNotebook() internally. Do you know if it is used outside mKCal ? As far as I can tell it's not. Would it be a good idea to completely remove it from the API if it's the case ?

[pvuorela]

Now the 0000FF seems to get used for all the notebooks that don't set a color when earlier it was just the default notebook. Allowing and defaulting the user created notebooks to empty value might still make sense, kind of interpreted as not defined?

[dcaliste]

Good point, I've reverted this change and I assign the default colour in sqlitestorage, on initialisation.

[pvuorela]

Seems good overall.

Couple possibly final remarks:

• The second last commit is "Deprecate ExtendedStorage::createDefaultNotebook()" but it doesn't do such deprecation. It only removes the internal usage of it. Could add a comment on the header and maybe make the method call warn.
• The "protect against notebook storage changes for runtime only notebooks" commit message could perhaps have a few more words that it no longer requires sync code to make the notebook temporarily writable and how silly that was.
• Created JB#59227 for no longer requiring read-only calendar sync to temporarily change is writable. Could use that also in the commit message.

Suppose this should be already fine to merge alone. The birthday calendar sync etc should still work, but just good to update to the new behavior, right?

[dcaliste]

Thanks for the comments. I've added a runtime warning when calling createDefaultNotebook() and mentioned it in the documentation. I'll submit a PR on buteo-sync-plugins soon to call the workaround instead of it (create a notebook with default constructor and set it default).

I've reword the read-only commit and I've added the JB tag also to the commit.

Having it in should not impact the existing usage of flipping the read-only flag. Just that it's useless now. I'll create PRs for birthday and buteo plugins as soon as this PR is accepted.

[pvuorela]

From default notebook commit: "Notice: deprecated since 0.6.10. Instead, create a notebook with the default constructor and call setDefaultNotebook()." Being specific, this won't get the same result as there's no name or color on that code path.

There was a question earlier was this used somewhere, think I've maybe answered in irc at some point(?) but fwiw there's a couple instances in buteo-sync-plugins. Suppose that could be dead code.

Before fully removing the method, maybe could now just adjust the text as ~ "create a notebook and call..." without specifying the notebook details.

[dcaliste]

Being specific, this won't get the same result as there's no name or color on that code path.

Yeh, you're right, not the default constructor, but the (name, description, color) one. For simplicity, I've removed the "with the default constructor" part of the sentence, as you suggested.

[dcaliste]

but fwiw there's a couple instances in buteo-sync-plugins.

I'll make a PR there to change it for the "create and setDefault" pattern.