Draft: [contactsd] Use Notebook::isEnabled() property to store account status.

[dcaliste]

Since mKCal is now separating incidence visibility from notebook listing, connect the account service status to this new notebook property.

@pvuorela , based on sailfishos/mkcal#69, I'm proposing here to use the new isEnabled() property. I've reworked the detection of the associated service to a notebook. Not sure that it's much better than the profile_id trick.

[dcaliste]

Just for testing how the code looks like, I've added a commit where the matching between service and notebook is done with the profile_id setting value. Finally, as a trick as it is, it looks better than the initial commit. (I still needs to test it properly though to see if it works well).

[pvuorela]

The other commit and pr title have a typo "Notebbok::isEnabled"

[pvuorela]

On the surface no much complaining so far. Not following why two commits, though.

[dcaliste]

Not following why two commits, though.

It's because, I worked on the first commit, trying not to use the profile_id matching (hopping that it was cleaner than expecting the presence of profile_id). But when I finished it, I found that it was not that clean and decided to try with the profile_id matching. That's the second commit. I kept the two commits separated so you can compare. If you think the second commit is nicer, I'll squash and forget about the first one.

[pvuorela]

Tried the PR set and toggling enabled on my google account.

Disabling works fine but enabling does something funny: first it enables the notebooks ok, but then it re-evaluates the state again and isServiceEnabledByProfile() -> account->enabledServices() evaluates to empty list and the account notebooks get disabled again. Restarting contactsd fixes the state.

[pvuorela]

Ok, looks like the first update is by the "sync" manager and the second is by "caldav" manager. Google account not having caldav service so enabledServices() returns an empty list.

[dcaliste]

Indeed, signaling of enabled change for accounts is always emitted. If a change happens in a service of different type from the selected type in the manager, the signal is emitted nonetheless, but enabledServices() or even services() list only the services matching the selected type. So no way to know what has changed. Even more wrong, a manager created for a given type will signal enabled change in accounts that even don't support this service type.

I'm pushing an additional commit that should circumvent these shortcomings. But I'm going to look at a proper fix in libaccount-glib. Even the docstring of the enabledEvent signal mentioned that it is only emitted for changes happening in services matching the service type.

[pvuorela]

Alright, let's see how that libaccounts side turns out.

In a way this enabled signal regardless of services might be even a nice thing in the sense of having a bit of chance to track the changes without knowing all the possible services, but bad if that happens contrary to the documentation. Wonder which should then get fixed, documentation or the behavior :)

[dcaliste]

I would be happy with a signal that is emitted for all services, but the current problem is that it is actually emitted for every service, but then, Account::services() and Account::enabledServices() that allow to do something depending on the change, list a restricted group of services. So we have the signal, but cannot do anything with it.

Except of course, if we have a second manager, that is not restricted and get the account from this second manager. So, you have a restricted manager that emits signal for every services, and a non-restricted manager to handle this signal ! That's a strange situation (that may work…).

No, I prefer to investigate in the direction of only emitting the signal for the subset of services we have access to with a restricted manager.

[dcaliste]

Here is an attempt to change upstream behaviour for the enabledEvent signal:
https://gitlab.com/accounts-sso/libaccounts-glib/-/merge_requests/35

[dcaliste]

I've been scratching my head a lot these last days with this issue and the libaccount-glib PR. Here are my conclusions:

• While having the enabledEvent emitted only for the service type the manager is registered on, would be nice ; it's not sufficiant (see explanations later),
• Our problem is rooted to the fact that we have several (two) service types that can handle notebooks.

I've reworked the PR keeping these two statements into account. I've tried it on a Nextcloud account, while enabling / disabling globally or for the calendar service. It seems to work. It is based on these two elements:

• knowing from a notebook, if a service is enabled for it, requires to search in all the enabled services and not only restricted to the type that generated the event. Thus the generic manager local variable in updateNotebooks().
• we still require specific managers to get the enabledEvent signal. But these managers should not be used to get enabled services, they are just listeners.

Even in the best case from libaccounts-glib, where a manager only emits the enabledEvent signal for its registered service type or for the global setting, we cannot rely on the enabledServices() of this manager. For instance: our notebooks are handled by a caldav service type. If the account becomes globally enabled, the manager restricted on sync will also trigger the enabledEvent signal, but this manager will return an empty list from enabledServices() because there is no service of sync type for this account in our example. Thus the proposed implementation to use a generic manager to list enabled services.

All of this point me to the following thought: as far as I understand libaccounts, we should have a single service type for notebooks. Like we have a single e-mail service type for all things providing emails, generic IMAP, Google… Then we can have a handfull of services of this type (for Nextcloud, Google, EAS, generic CalDAV, webcal…). Let's call this type calendar. How much of transitions it would require to fix this in our setup ?

• create a file calendar.service-type in /usr/share/accounts/service_types: easy,
• update several files in /usr/share/accounts/services/ to replace <type>foo</type> with <type>calendar</type>: not too difficult, but these services have already been cached in the database. So the database entries need to be updated as well. Something like UPDATE Services SET type = 'calendar' WHERE name == 'foo';. That requires a bit of shell scripting to do it automatically for all known services, can be tricky, but tractable.
• hope to find all places in various repos where the type is hard-coded to "caldav" or "sync" or whatever. Basically, it's the nemo QML plugin, Buteo sync plugins and settings QML and UI parts. That can be a long task, with broken things discovered late.

So, not very hopeful. And it can't be done by bits. But that would be a nice evolution towards cleaner code and less headache with libaccounts. Anyway, here is the PR that should tape the issue for the moment…

[dcaliste]

While speaking about transition, it would be simpler if the name of the profile_id key was not prefixed with the service name. It's useless : the key is already stored for the given service and the settings table has a UNIQUE constraint on the triplet (account, service, key). If the key would simply be profile_id, it would save a for loop in the code here.