Bug 70950 - add PBAP mode to API
Summary: add PBAP mode to API
Status: RESOLVED FIXED
Alias: None
Product: SyncEvolution
Classification: Unclassified
Component: PIM Manager (show other bugs)
Version: 1.3.99.3
Hardware: Other All
: highest enhancement
Assignee: Patrick Ohly
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 72112 72114
  Show dependency treegraph
 
Reported: 2013-10-28 10:02 UTC by Patrick Ohly
Modified: 2014-02-14 10:23 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Ohly 2013-10-28 10:02:27 UTC
Currently, incremental syncing (text first, then photos) can only be controlled by an env variable (SYNCEVOLUTION_PBAP_MODE). It would be better to have an explicit parameter to SyncPeer() and the syncevolution command line for that, because changing the env variable of a running syncevo-dbus-server is not possible.

The command line switch could also be used to enable "ephemeral" syncing where no meta data gets written. That would bring debugging via the command line one step closer to syncing via SyncPeer().
Comment 1 Patrick Ohly 2014-01-15 13:29:47 UTC
Here's a proposal for the PIM Manager API. This also includes enhancements for suspend/resume (bug #72112) and enhanced progress (bug #72114).

The additional paragraphs about "a peer can have
only one running sync at a time" and "same contact gets counted twice" do not change the API. They are merely clarifications.


diff --git a/src/dbus/server/pim/pim-manager-api.txt b/src/dbus/server/pim/pim-manager-api.txt
index 3283c7c..97128ca 100644
--- a/src/dbus/server/pim/pim-manager-api.txt
+++ b/src/dbus/server/pim/pim-manager-api.txt
@@ -51,6 +51,10 @@ address book. That information is passed via D-Bus as a
 string-to-string dict, with keys and their values being defined by the
 implementation.
 
+For the sake of API and implementation simplicity, a peer can have
+only one running sync at a time. The API uses the peer ID for
+information about such a running sync (see "SyncProgress" signal).
+
 Address books
 -------------
 
@@ -254,12 +258,45 @@ Methods:
          updated. The caller needs to check the local cache to find out
          what it contains.
 
+    dict SyncPeerWithFlags(string uid, dict flags)
+
+         Same as SyncPeer() with additional control over the sync via
+         optional flags. The flags and their values are
+         implementation-dependent.
+
     void StopSync(string uid)
 
          Stop any running sync for the given peer. The SyncPeer() method
          which started such a sync will return with an "aborted" error
          once the sync was stopped.
 
+    void SuspendSync(string uid)
+
+         Suspend any running sync for the given peer. The SyncPeer()
+         method which started such a sync will continue to run. If
+         suspending is not possible for some reason, the sync will
+         automatically abort or fail (depending on the peer). It is
+         okay to suspend twice. Suspend and resume calls are not
+         nested, so a suspended sync resumes after one ResumeSync(),
+         regardless how often SuspendSync() was called.
+
+    void ResumeSync(string uid)
+
+         Resume a previously suspended sync. It is not an error to call
+         this when there is no sync running or it was not suspended.
+
+    dict GetPeerStatus(string uid)
+
+         Returns information about the peer and its sync status. The returned
+         dictionary contains:
+
+         "status": string "idle", "syncing", "suspended" - describes whether
+         a sync is currently running. Idle means "no active sync"; there might
+         be a pending SyncPeer().
+
+         Implementations can also provide information similar to the data
+         provided by the SyncProgress signal.
+
     dict of UID to string key/value dict GetAllPeers()
 
          Returns information about all currently configured peers.

diff --git a/src/dbus/server/pim/README b/src/dbus/server/pim/README
index a83fe52..c444c40 100644
--- a/src/dbus/server/pim/README
+++ b/src/dbus/server/pim/README
@@ -326,8 +326,9 @@ Not supported via the API at the moment:
 Syncing
 =======
 
-SetSync() in SyncEvolution will return a dict with all of the
-following entries set:
+SyncPeer() and SyncPeerWithFlags() in SyncEvolution will return a dict
+with all of the following entries set:
+
     "modified": boolean - data was modified
     "added" : integer - number of new contacts
     "updated" : integer - number of updated contacts
@@ -337,14 +338,66 @@ In other words, the caller can reliably detect when nothing changed,
 but when contacts were modified or added, it needs to read them to
 determine which kind of properties were modified or added.
 
+It is possible that the same contact gets counted twice, for example
+when adding it in the first text-only cycle and later updating it with
+photo data in the second cycle, or updating some text first and the
+photo later.
+
+Additional control over syncing is possible via the flags dictionary
+in SyncPeerWithFlags(). The following keys are possible:
+
+    "progress-frequency": float - desired frequency in Hz (= events
+    per second) of the SyncProgress "progress" event. Default is 1/s.
+    "pbap-sync": string "text"|"all"|"incremental" - synchronize only
+    text properties, all properties or first text, then all.
+
+When doing text syncing, local photo data is preserved. The
+incremental sync has the same effect as a text-only sync followed by
+an all-properties sync. The only difference is that it looks and
+behaves like a single sync of the peer.
+
 The SyncProgress is triggered by SyncEvolution with three different
 keys (in this order, with "modified" occuring zero or more times):
-  "started" "modified"* "done"
+"started" ("modified" | "progress")* "done"
 
 "started" and "done" send an empty data dictionary. "modified" sends
-the same dictionary as the one returned by SyncPeer(), if contact data
-was modified. So by definition, "modified" will be True in the
-dictionary, but is included anyway for the sake of consistency.
+the same dictionary as the one returned by SyncPeer(), if and only if
+contact data was modified. So by definition, "modified" will be True
+in the dictionary, but is included anyway for the sake of consistency.
+
+"progress" sends the same dictionary as "modified" with one additional
+key:
+
+    "percent": float - value from 0 (start of sync) to 1 (end of sync)
+
+"progress" is guaranteed to be emitted at the end of each cycle. In
+contrast to "modified", it will also be sent throughout the sync *if
+and only if* progress has been made, regardless whether that progress
+led to contact data changes or an increase in the completion
+percentage. That way, the event also serves as a heartbeat mechanism.
+
+The completion percentage is based purely on the number of contacts on
+the peer, which is the only thing known about the peer at the start of
+a sync. In a simple sync involving just one cycle, the percentage is
+"number of processed contacts / total contacts".
+
+In a sync involving two cycles, it is "number of processed contacts /
+2 / total contacts". Because each contact gets processed once per
+cycle, that gives 50% completion at the end of the first cycle and
+100% at the end of the second.
+
+Note that progress will not be linear over time even for a simple
+sync, because some contacts will take more time to download and
+process than others. For an incremental sync, the first cycle is
+likely to be much faster than the second one because the second one
+includes photo data.
+
+GetPeerStatus() returns two additional entries in the dictionary:
+
+    "progress": same dictionary as the SyncProgress "progress" value
+    "last-progress": float - time in seconds since the last "progress"
+    event. This is included to let a polling client determine whether
+    the sync is still alive.
 
 
 Contact Data
Comment 2 Patrick Ohly 2014-01-15 13:30:32 UTC
Does SyncPeerWithFlags() and the "pbap-sync" flag make sense? Okay to implement?
Comment 3 Eugenio Parodi 2014-01-15 18:03:30 UTC
(In reply to comment #2)
> Does SyncPeerWithFlags() and the "pbap-sync" flag make sense? Okay to
> implement?

yes it make perfectly sense.

is it possible add also the "picture only" sync?:
     "pbap-sync": string "text"|"pictures"|"all"|"incremental" - 
          synchronize only text properties,
          only pictures, all properties or first text, then all
This extra field may give a better control over the synchronization process.

Sometimes some redundant data may be useful.
in the "progress event" and GetPeerStatus() dictionary, is it possible to add also a "SyncType" key?
     SyncType : 
        "text", "pictures", "all", "incremental-text", "incremental-pictures" 

This may be helpful to have a perfect knowledge of the exact sync-status,
in particular, if we reach "incremental-pictures" status, we know that the addressbook is consistent enough to be browsed.

Thanks.
Comment 4 Patrick Ohly 2014-01-15 19:02:45 UTC
"picture-only" is not technically possible, because the text fields are required to find matches with existing local contacts.

One could do a "picture-with-minimal-text-fields", but that is not going to provide much benefits (none?).

If some future PBAP enhancement makes a "picture-only" mode more efficient than "all", then we can still add that at that point.

Regarding "SyncType", yes, something like that can be added. But it is not clear to me what you expect to see reported there. Is this a correct description of what you had in mind?

'text' - text-only sync active
'all' - non-incremental text and picture sync active
'incremental-text' - incremental sync active, currently syncing text
'incremental-picture' - incremental sync active, done with text, now syncing pictures

I would call that 'sync-cycle', to make it more explicit that this is about what is currently going on and may change as we enter another cycle.
Comment 5 Eugenio Parodi 2014-01-16 10:00:47 UTC
(In reply to comment #4)
> "picture-only" is not technically possible, because the text fields are
> required to find matches with existing local contacts.
> 
> One could do a "picture-with-minimal-text-fields", but that is not going to
> provide much benefits (none?).
> 
> If some future PBAP enhancement makes a "picture-only" mode more efficient
> than "all", then we can still add that at that point.
> 
> Regarding "SyncType", yes, something like that can be added. But it is not
> clear to me what you expect to see reported there. Is this a correct
> description of what you had in mind?
> 
> 'text' - text-only sync active
> 'all' - non-incremental text and picture sync active
> 'incremental-text' - incremental sync active, currently syncing text
> 'incremental-picture' - incremental sync active, done with text, now syncing
> pictures
> 
> I would call that 'sync-cycle', to make it more explicit that this is about
> what is currently going on and may change as we enter another cycle.

My idea of SyncType has few purposes.
1) Basically to know the exact moment when the text has been synced in order to browse the addressbook.
2) It happen to me writing a PIM gui few time ago, the module that usually initiate a task (i.e. StartSync) is separate from the module that show the results (i.e. GetPeerStatus) and maybe a third module would like to monitor the Sync to display the "Search" button in  the exact moment that the text has been synced.


About the Sync Pictures Only.
You are right, but it may be useful to have it.
I think we should consider to add few restrictions 
i.e.
"picture" require a previous sync with "text"
This feature is not necessary but will provide the right flexibility to manage the sync in the way the client would prefer (i.e. multiple "pictures-only" sync after some specific event)

Below there is an Idea I had:
I was just wondering how much effort is required to make something more generic for the incremental sync:
Some kind of custom incremental sync setup inside the SetPeer api.
By Default (the current one):
   Stage 1: (list of all the fields except "PHOTO")
   Stage 2: ("PHOTO")

But another possible useful configuration may be:
   Stage 1: (FN,NAME,EMAIL,TEL)
   Stage 2: (ADR,GEO,BDAY)
   Stage 3: (PHOTO)
All the other fields are not required for this peer.
Some restrictions must be considered
(i.e. the mandatory vCard fields are included by default in any stage)

This may help the customization of the VCard field required for the Sync because some PIM implementations may not require the full vCard but just a subset of it.

With this customization the SyncType may have as result value "incremental-stage-X".

And possibly a sync of a specific stage would be very useful with the prerequisite of the completed sync of a previous stage.  

This is just an idea but it will add enough flexibility for different use case.
Comment 6 Patrick Ohly 2014-01-16 10:43:44 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > "picture-only" is not technically possible, because the text fields are
> > required to find matches with existing local contacts.
> > 
> > One could do a "picture-with-minimal-text-fields", but that is not going to
> > provide much benefits (none?).
> > 
> > If some future PBAP enhancement makes a "picture-only" mode more efficient
> > than "all", then we can still add that at that point.
> > 
> > Regarding "SyncType", yes, something like that can be added. But it is not
> > clear to me what you expect to see reported there. Is this a correct
> > description of what you had in mind?
> > 
> > 'text' - text-only sync active
> > 'all' - non-incremental text and picture sync active
> > 'incremental-text' - incremental sync active, currently syncing text
> > 'incremental-picture' - incremental sync active, done with text, now syncing
> > pictures
> > 
> > I would call that 'sync-cycle', to make it more explicit that this is about
> > what is currently going on and may change as we enter another cycle.
> 
> My idea of SyncType has few purposes.
> 1) Basically to know the exact moment when the text has been synced in order
> to browse the addressbook.
> 2) It happen to me writing a PIM gui few time ago, the module that usually
> initiate a task (i.e. StartSync) is separate from the module that show the
> results (i.e. GetPeerStatus) and maybe a third module would like to monitor
> the Sync to display the "Search" button in  the exact moment that the text
> has been synced.

Understood. So is the description of 'sync-cycle' and its values above okay?

> About the Sync Pictures Only.
> You are right, but it may be useful to have it.

Agree, but it's not technically feasible.

> I think we should consider to add few restrictions 
> i.e.
> "picture" require a previous sync with "text"

That doesn't help. When I do a PullAll(), even if it is in the same PBAP session, I get no information about which vcard refers to which contact. I still need the text properties in the vcard to find that match a second time.

> Below there is an Idea I had:
> I was just wondering how much effort is required to make something more
> generic for the incremental sync:
> Some kind of custom incremental sync setup inside the SetPeer api.
> By Default (the current one):
>    Stage 1: (list of all the fields except "PHOTO")
>    Stage 2: ("PHOTO")
> 
> But another possible useful configuration may be:
>    Stage 1: (FN,NAME,EMAIL,TEL)
>    Stage 2: (ADR,GEO,BDAY)
>    Stage 3: (PHOTO)
> All the other fields are not required for this peer.
> Some restrictions must be considered
> (i.e. the mandatory vCard fields are included by default in any stage)
> 
> This may help the customization of the VCard field required for the Sync
> because some PIM implementations may not require the full vCard but just a
> subset of it.

There are two different feature request here:
1. make incremental syncing more flexible such that any property can
   be turned off during a sync without removing it locally; PHOTO
   is currently the only property where that is possible.
2. configure the set of properties that the local side is interested
   in.

This sounds similar, but the implementation is different. The first one
involves sync logic, the second only one the set of data retrieved from the peer.

The first one is harder. Right now, specific code has to be written for each property which shall get that special treatmet. Adding more of them would get ugly pretty soon.

Can you name a specific use case where you expect this to be beneficial?

The second one is already possible internally, there just isn't an API for it. This would go into CreatePeer().

Either way, for the sake of avoiding feature creep, let's not mingle that into this issue here. If you want this features, discuss their importance with your colleagues and file new feature requests.
Comment 7 Eugenio Parodi 2014-01-16 13:33:25 UTC
> Can you name a specific use case where you expect this to be beneficial?

An use case based on some feedback I received is:
Stage 1) Sync NAME,TEL,EMAIL 
      - First time DB population it will quickly create a browseable DB
        the fields chosen are the most important required for the 
        system usage.
Stage 2,3,4,...) Sync (Other Important Fields)
      - The DB is populated with extra (less important) fields 
        that may be required by the end user but it is nice to have now.
Stage last) Sync PHOTO or other fields left behind
      - The last stage is left for the most expensive but still required task.
The feedback I received are related to the deep knowledge and control over the sync mechanism.
Comment 8 Patrick Ohly 2014-01-16 13:43:42 UTC
(In reply to comment #7)
> > Can you name a specific use case where you expect this to be beneficial?
> 
> An use case based on some feedback I received is:
> Stage 1) Sync NAME,TEL,EMAIL 
>       - First time DB population it will quickly create a browseable DB
>         the fields chosen are the most important required for the 
>         system usage.
> Stage 2,3,4,...) Sync (Other Important Fields)
>       - The DB is populated with extra (less important) fields 
>         that may be required by the end user but it is nice to have now.

What is the expected benefit of doing stage 2,3,4 etc separately?

"More quickly" is probably the answer, but how quickly does it have to be to outweigh the downside (longer time until the cache is really in sync with the peer)?

I suspect that whoever asks for this overestimates the time that it takes to download those extra fields. Before we implement this, someone should quantify that by running some download experiments. You can use SyncEvolution for that, see the src/pbap/README for instructions on how to filter fields.
Comment 9 Eugenio Parodi 2014-01-16 14:05:19 UTC
> What is the expected benefit of doing stage 2,3,4 etc separately?
> 
> "More quickly" is probably the answer, but how quickly does it have to be to
> outweigh the downside (longer time until the cache is really in sync with
> the peer)?
> 
> I suspect that whoever asks for this overestimates the time that it takes to
> download those extra fields. Before we implement this, someone should
> quantify that by running some download experiments. You can use
> SyncEvolution for that, see the src/pbap/README for instructions on how to
> filter fields.

Steps 2,3,4,5,6... will rarely used in a standard product 
This feature will add the flexibility to define the Sync in the way the user want. If wrongly configured it may end up in a significant slowdown.
But for example, I noticed that the NOTE field may include a big amount of data (i.e. in some business devices), some implementations may like to sync it during the second stage with all the less important fields, the PHOTO will be synced in a third stage.
We cannot predict all the possible corner cases.

I can try to make some download tests.
Comment 10 Patrick Ohly 2014-02-14 10:23:04 UTC
Was implemented in syncevolution-1-3-99-7-20140203.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.