After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 657141 - Backend should ask eds for the default backend, not hardcode it
Backend should ask eds for the default backend, not hardcode it
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: e-d-s backend
git master
Other Linux
: Normal normal
: folks-0.6.3
Assigned To: folks-maint
folks-maint
Depends on: 658324 659075
Blocks: 659040
 
 
Reported: 2011-08-23 09:14 UTC by Alexander Larsson
Modified: 2011-09-16 20:16 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add writeable store property to Folks backends (5.18 KB, patch)
2011-08-24 18:07 UTC, Raul Gutierrez Segales
needs-work Details | Review
Patch refreshed after review comments (9.92 KB, patch)
2011-08-25 16:27 UTC, Raul Gutierrez Segales
none Details | Review
Patch refreshed after review comments (9.92 KB, patch)
2011-08-25 16:27 UTC, Raul Gutierrez Segales
reviewed Details | Review
Updated patch (18.79 KB, patch)
2011-09-02 18:12 UTC, Raul Gutierrez Segales
needs-work Details | Review
patch (22.21 KB, patch)
2011-09-03 09:22 UTC, Raul Gutierrez Segales
needs-work Details | Review
patch (15.27 KB, patch)
2011-09-15 15:37 UTC, Raul Gutierrez Segales
needs-work Details | Review
patch (16.12 KB, patch)
2011-09-16 10:16 UTC, Raul Gutierrez Segales
accepted-commit_now Details | Review

Description Alexander Larsson 2011-08-23 09:14:50 UTC
If eds is set up such that the gmail addressbook is the default we should pick that up. Currently we just hardcode "system" which means we always use the local database which is often not what people want.
Comment 1 Raul Gutierrez Segales 2011-08-23 15:21:37 UTC
(In reply to comment #0)
> If eds is set up such that the gmail addressbook is the default we should pick
> that up. Currently we just hardcode "system" which means we always use the
> local database which is often not what people want.

Well, Folks uses GConf keys (or gsettings eventually) to define the primary store. I am guessing this makes sense while we still have the key-file backend around. Once that is gone and primary stores can only come from e-d-s, then querying e-d-s for the default address book would be a better way of defining the primary store. 

With this in mind, I had filed this for Gnome Contacts some time ago:

https://bugzilla.gnome.org/show_bug.cgi?id=654908
Comment 2 Travis Reitter 2011-08-23 15:56:00 UTC
(In reply to comment #1)
> (In reply to comment #0)
> > If eds is set up such that the gmail addressbook is the default we should pick
> > that up. Currently we just hardcode "system" which means we always use the
> > local database which is often not what people want.
> 
> Well, Folks uses GConf keys (or gsettings eventually) to define the primary
> store. I am guessing this makes sense while we still have the key-file backend
> around. Once that is gone and primary stores can only come from e-d-s, then
> querying e-d-s for the default address book would be a better way of defining
> the primary store. 
> 
> With this in mind, I had filed this for Gnome Contacts some time ago:
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=654908

I don't see why we couldn't just request the default from e-d-s in the place we currently just hardcode "system".
Comment 3 Alexander Larsson 2011-08-23 18:26:59 UTC
Yeah, It might be interesting for some to have folks specific store selection, but by default we should use the desktop-wide default addressbook, which is the one set as default in e-d-s.
Comment 4 Raul Gutierrez Segales 2011-08-24 16:49:18 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > If eds is set up such that the gmail addressbook is the default we should pick
> > > that up. Currently we just hardcode "system" which means we always use the
> > > local database which is often not what people want.
> > 
> > Well, Folks uses GConf keys (or gsettings eventually) to define the primary
> > store. I am guessing this makes sense while we still have the key-file backend
> > around. Once that is gone and primary stores can only come from e-d-s, then
> > querying e-d-s for the default address book would be a better way of defining
> > the primary store. 
> > 
> > With this in mind, I had filed this for Gnome Contacts some time ago:
> > 
> > https://bugzilla.gnome.org/show_bug.cgi?id=654908
> 
> I don't see why we couldn't just request the default from e-d-s in the place we
> currently just hardcode "system".

Note that this (selecting the writeable PersonaStore) happens from the IndividualAggregator - where we can't have any explicit dependency with any given backend (in this case, the e-d-s one). So we'll need some way to set generic properties on PersonaStores so we can then query them from the IndividualAggregator without creating hard dependencies.
Comment 5 Raul Gutierrez Segales 2011-08-24 17:50:09 UTC
Initial take at this, following what was discussed on IRC with Philip:

http://cgit.collabora.com/git/user/rgs/folks/log/?h=bgo-657141
Comment 6 Raul Gutierrez Segales 2011-08-24 18:07:27 UTC
Created attachment 194648 [details] [review]
Add writeable store property to Folks backends
Comment 7 Philip Withnall 2011-08-24 18:19:13 UTC
Review of attachment 194648 [details] [review]:

I think we need to consider change notification for the property. Apart from that the patch looks good.

::: backends/eds/eds-backend.vala
@@ +148,3 @@
       unowned GLib.SList<weak E.SourceGroup> groups =
           this._ab_sources.peek_groups ();
+      unowned E.Source? default_source = list.peek_default_source ();

Is it possible to get a change notification for this? For example, if the user changes their default address book in Evo while folks is being used elsewhere?

@@ +229,3 @@
+      if (this._default_source_uri == s.peek_relative_uri ())
+        {
+          this._writeable_store_id = store.id;

Need to call this.notify_property().

@@ +246,3 @@
+      if (this._writeable_store_id == store.id)
+        {
+          this._writeable_store_id = null;

Need to call this.notify_property().

::: folks/backend.vala
@@ +127,3 @@
+   * This will be used to identify which {@link PersonaStore} from within
+   * this backend should be used by the {@link IndividualAggregator} as
+   * the primary (writeable) one.

Mention that `null` means the backend has no preferred writeable store at the moment.

You should probably also mention that the preferred writeable store could change, so clients (i.e. IndividualAggregator) should listen for changes to the property.

::: folks/individual-aggregator.vala
@@ +485,3 @@
       if (store.type_id == this._configured_writeable_store_type_id)
         {
+          if (backend.writeable_store_id == store.id)

I think this needs some extra conditions to make sure that iff the user has set FOLKS_WRITEABLE_STORE or the GConf key, we don't change this._writeable_store to a store which isn't what they set.

Also, the IndividualAggregator needs to listen to change notifications for backend.writeable_store_id and update this._writeable_store as appropriate.
Comment 8 Raul Gutierrez Segales 2011-08-25 16:27:11 UTC
Created attachment 194711 [details] [review]
Patch refreshed after review comments
Comment 9 Raul Gutierrez Segales 2011-08-25 16:27:37 UTC
Created attachment 194712 [details] [review]
Patch refreshed after review comments
Comment 10 Philip Withnall 2011-08-25 23:25:38 UTC
Review of attachment 194712 [details] [review]:

::: backends/eds/eds-backend.vala
@@ +219,3 @@
       var store = new Edsf.PersonaStore (s);
 
+      store.notify["is-default-adddresso-book"].connect (

s/addresso/address/

@@ +255,3 @@
+          this._writeable_store_id == store.id)
+        {
+          this._writeable_store_id = null;

Is it possible to reset the writeable store ID back to an (appropriate) different store here? e.g. Could we iterate through the source list and check for the new default?

::: backends/eds/lib/edsf-persona-store.vala
@@ +127,2 @@
   /**
+   * Whether this PersonaStore represents a default address book

s/a/the/?

::: folks/individual-aggregator.vala
@@ +484,3 @@
+              this._configured_writeable_store_id = store.id;
+              this._writeable_store.is_writeable = true;
+              this._writeable_store.trust_level = PersonaStoreTrust.FULL;

Shouldn't we notify of the change to IndividualAggregator.primary_store here?

@@ +518,3 @@
+              store.is_writeable = true;
+              store.trust_level = PersonaStoreTrust.FULL;
+              this._writeable_store = store;

We should also notify of the change to IndividualAggregator.primary_store here and in the rest of this function. This should probably be a separate commit.
Comment 11 Raul Gutierrez Segales 2011-08-25 23:58:08 UTC
(In reply to comment #10)
> Review of attachment 194712 [details] [review]:
> @@ +255,3 @@
> +          this._writeable_store_id == store.id)
> +        {
> +          this._writeable_store_id = null;
> 
> Is it possible to reset the writeable store ID back to an (appropriate)
> different store here? e.g. Could we iterate through the source list and check
> for the new default?

We could.. not sure we want to (think convoluted setups, maaany address books). 

> @@ +518,3 @@
> +              store.is_writeable = true;
> +              store.trust_level = PersonaStoreTrust.FULL;
> +              this._writeable_store = store;
> 
> We should also notify of the change to IndividualAggregator.primary_store here
> and in the rest of this function. This should probably be a separate commit.

Why? This is all part of "setting the writeable store to whatever the chosen backend says it shold be". What different logical changes do you see?
Comment 12 Raul Gutierrez Segales 2011-08-26 00:01:47 UTC
Oh - I think you meant the missing notifications for primary store changes (if so, sorry for the extra question).
Comment 13 Philip Withnall 2011-08-26 08:05:49 UTC
(In reply to comment #12)
> Oh - I think you meant the missing notifications for primary store changes (if
> so, sorry for the extra question).

I did. Sorry if I didn't make that clear. I was suggesting it should be a separate commit because those notifications are missing at the moment and should be added anyway, regardless of the work to fix this bug.
Comment 14 Raul Gutierrez Segales 2011-08-29 12:46:44 UTC
Didn't make this release; punting to 0.6.2.
Comment 15 Raul Gutierrez Segales 2011-09-02 18:09:58 UTC
Latest iteration of this lives here:

http://cgit.collabora.com/git/user/rgs/folks/log/?h=bgo-657141
Comment 16 Raul Gutierrez Segales 2011-09-02 18:12:40 UTC
Created attachment 195516 [details] [review]
Updated patch

Went over review comments and added a test case.
Comment 17 Travis Reitter 2011-09-02 18:46:20 UTC
Review of attachment 195516 [details] [review]:

+  private void _check_if_default ()

_notify_if_default() would be a better name


+      store.notify["is-default-address-book"].connect (
+          this._default_store_cb);
+
+      if (store.is_default_address_book)
+        {
+          this._writeable_store_id = store.id;
+          this.notify_property ("writeable-store-id");
+        }
+
       store.removed.connect (this._store_removed_cb);
 
       this._persona_stores.set (store.id, store);
@@ -217,10 +235,29 @@ public class Folks.Backends.Eds.Backend : Folks.Backend
       this.persona_store_added (store);
     }
 
+  private void _default_store_cb (Object store_obj, ParamSpec ps)
+    {
+      Edsf.PersonaStore store = (Edsf.PersonaStore) store_obj;
+      if (store.is_default_address_book)
+        {
+          this._writeable_store_id = store.id;
+          this.notify_property ("writeable-store-id");
+        }
+    }

Factor out some common code here


+  *  - if link_personas () is called while we are swapping
+  *    stores the Universe might collapse. Should we hold
+  *    a lock then or set this._linking_enabled = false?

Locking sounds appropriate here. link_personas() and the various entry points to changing or invalidating the writeable store should use the same lock.

And because locking is so tricky, we should be extra careful to have thorough tests for this (ie, add some test cases to change-primary-store that involve many racy linkings with many store swaps).
Comment 18 Raul Gutierrez Segales 2011-09-03 09:22:13 UTC
Created attachment 195558 [details] [review]
patch

Addressed what was pointed out by Travis.
Comment 19 Philip Withnall 2011-09-03 10:22:18 UTC
Review of attachment 195558 [details] [review]:

::: backends/eds/eds-backend.vala
@@ +297,3 @@
+
+  private void _notify_if_writeable (Edsf.PersonaStore store,
+      StoreAction action)

This function could do with a comment.

::: backends/eds/lib/edsf-persona-store.vala
@@ +148,2 @@
   /**
+   * Whether this PersonaStore represents the default address book

Do we expect more than one store to ever have this set to true?

Might also help to define what is meant by “the default address book” — i.e. the one the user's ticked as the default in Evolution; the one which Evolution uses by default for type-ahead e-mail address completion, etc.

::: folks/individual-aggregator.vala
@@ +79,3 @@
   private static const string _FOLKS_CONFIG_KEY =
     "/system/folks/backends/primary_store";
+  private bool _user_set_writeable_store;

This could do with a comment explaining what it means/is used for.

@@ +534,3 @@
+  *
+  *  - all around the IndividualAggregator we have
+  *    assertions about the primary store, it's trust-level

s/it's/its/

@@ +535,3 @@
+  *  - all around the IndividualAggregator we have
+  *    assertions about the primary store, it's trust-level
+  *    and writeability and the state of this attributes

s/this/these/ :-)

@@ +541,3 @@
+  *    store.
+  *  - we protect ourselves with a lock so link_personas ()
+  *    won't run while we are swapping the primary store

I'm not entirely convinced this is true. IIRC, we established before that lock() in Vala translates to a GRecMutex — so re-entrant calls to this code from itself or from link_personas() (or vice-versa) won't be mutually exclusive. Put another way, IIRC lock() only provides mutual exclusion between strictly different threads. (Please, please correct me if I'm wrong here.)

Therefore, there needs to be some reasoning in this comment as to why this code will never be called re-entrantly from link_personas() (and vice-versa).

What would make this all simpler is if you could guarantee that this code is only ever called in the main thread, and then also guarantee that link_personas() never causes the main loop to be iterated. If that's the case, there's no need for any of the locking or any exclusion variables.

@@ +563,3 @@
+            {
+              previous_writeable_store.is_writeable = false;
+              previous_writeable_store.trust_level = PersonaStoreTrust.NONE;

Should this not be PersonaStoreTrust.PARTIAL for some stores?
Comment 20 Raul Gutierrez Segales 2011-09-03 17:01:59 UTC
(In reply to comment #19)

(...)

> @@ +563,3 @@
> +            {
> +              previous_writeable_store.is_writeable = false;
> +              previous_writeable_store.trust_level = PersonaStoreTrust.NONE;
> 
> Should this not be PersonaStoreTrust.PARTIAL for some stores?

It should be whatever the default trust of the Store was before it was promoted to trust-level = FULL.

We should re-iterate over the defintions of primary-store, having a writeable store and the trust-level of PersonaStores... And also re-think where and when they should be modified.
Comment 21 Travis Reitter 2011-09-05 16:51:04 UTC
(Punting bugs to 0.6.3)
Comment 22 Travis Reitter 2011-09-05 17:15:29 UTC
Note the TODO in attachment 195603 [details] [review] on bug 657783; that may get merged before the fix for this bug, in which case the TODO needs to be addressed for this bug to be truly fixed.
Comment 23 Raul Gutierrez Segales 2011-09-15 15:37:37 UTC
Created attachment 196646 [details] [review]
patch

Here goes a simpler approach, based on top of the recent clean ups that have been done to the IndividualAggregator.
Comment 24 Philip Withnall 2011-09-15 18:05:31 UTC
Review of attachment 196646 [details] [review]:

::: folks/individual-aggregator.vala
@@ +101,3 @@
   private uint _non_quiescent_backend_count = 0;
   private bool _is_quiescent = false;
+  private bool _user_configured_primary_store = false;

It's probably worth adding a comment explaining that this variable is true iff the primary store has been set from environment variables or GConf.

@@ +625,3 @@
+                }
+
+              this._primary_store.is_primary_store = true;

Might be an idea to freeze notifications on both stores before changing their is_primary_store properties, then unfreeze notifications once both properties have been set. That way, client code handling notifications for those properties never sees a state where no persona store has is_primary_store == true.

@@ +632,3 @@
+
+  private void _backend_persona_store_added_cb (Backend backend,
+      PersonaStore store)

What about the case of if the store already has user_set_default == true, and _persona_store_user_set_default_changed_cb() never gets called on it?

::: folks/persona-store.vala
@@ +649,3 @@
+  /**
+   * Whether this {@link PersonaStore} has been marked as the default
+   * store by the user. I.e.: a PersonaStore for the e-d-s backend would

“as the default store _in its backend_ by the user”?

::: tests/eds/change-primary-store.vala
@@ +73,3 @@
+      this._main_loop.run ();
+
+     assert (this._new_primary_store_found);

Whitespace problem.

@@ +108,3 @@
+    }
+
+  private PersonaStore? _get_store (BackendStore store, string store_id)

Might be better to call this “_get_persona_store” so that people don't get confused between the backend store and persona stores.

::: tests/lib/eds/backend.vala
@@ +113,3 @@
     }
 
+  public void set_default ()

Perhaps this should be called “set_as_default” instead? “set_default” implies to me that it should take a parameter which says what the new default should be.
Comment 25 Raul Gutierrez Segales 2011-09-16 10:16:17 UTC
Created attachment 196705 [details] [review]
patch

Updated version.
Comment 26 Philip Withnall 2011-09-16 19:31:29 UTC
Review of attachment 196705 [details] [review]:

Looks good to me, but I think it would be good to get a second opinion. Travis?

::: tests/eds/change-primary-store.vala
@@ +122,3 @@
+  private void _primary_store_cb (Object ia_obj, ParamSpec ps)
+    {
+      IndividualAggregator  ia = (IndividualAggregator) ia_obj;

Double space.
Comment 27 Travis Reitter 2011-09-16 19:55:18 UTC
Review of attachment 196705 [details] [review]:

Looks good to me.
Comment 28 Raul Gutierrez Segales 2011-09-16 20:16:56 UTC
Merged:

commit 9ec53e830cf1ff12681b58f068a08cec6511dd8d
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Thu Sep 1 19:29:06 2011 +0100

    e-d-s: test case for changing primary store

commit b1f9c2df3c5d62184ac51a44195e44401db74fe8
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Thu Sep 1 19:31:28 2011 +0100

    e-d-s: allow setting default address books in tests

commit 00ada566e82fef5ed87793b2de7c5cb6e2910ef2
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Thu Sep 15 15:38:11 2011 +0100

    e-d-s: notify when an address book is configured as default
    
    Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=657141

commit d797e8e101252ddd674ffdffab61d9409219a48f
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Thu Sep 15 15:33:58 2011 +0100

    core: add Folks.PersonaStore.user-set-default property
    
    With this property we can tell if a Store (i.e.: an e-d-s address
    book) has been marked by the user as his default store (either from
    Evo or from GOA, etc).
    
    Helps: https://bugzilla.gnome.org/show_bug.cgi?id=657141

commit 32c95ea8884b415d7835fd1fe147db195fc61f5f
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Thu Sep 15 14:43:36 2011 +0100

    IndividualAggregator: improve naming for primary store related methods
    
    Renamed _set_primary_store () to _configure_primary_store () and refactor
    the code that actually sets the primary store into _set_primary_store ().
    
    Helps: https://bugzilla.gnome.org/show_bug.cgi?id=657141