[gnome-contacts/nielsdg/dirty: 4/4] Keep track if a chunk has changed

[Niels De Graef <nielsdg src gnome org>]

commit 380136c4e39f1f6d5ce5894b2b9f3506c3a9d1a6
Author: Niels De Graef <nielsdegraef gmail com>
Date:   Sun Oct 9 10:07:04 2022 +0200

    Keep track if a chunk has changed
    
    Introduce a property `dirty` which signifies if a `Contacts.Chunk`
    chagned compared to its original value. That way, we can make sure we
    don't try to save a property that didn't change, saving us some
    necessary work. Although normally folks does a similar check, it's still
    good to prevent going into folks (or anything similar) in the first
    placE.
    
    As a side effect, it solves a problem we currently had with the
    `NicknameChunk`: when calling `save_to_persona()`, it erroneously
    represented an empty value as both `""` and `null`. Since those are
    different, it would try to save them and the E-D-S would time out in
    that case (and throw an appropriate error). As a consequence, when
    editing a contact, Contacts would always get blocked on the "nickname"
    property.
    
    Fixes: https://gitlab.gnome.org/GNOME/gnome-contacts/-/issues/271

 meson.build                                  |   2 +-
 src/core/contacts-addresses-chunk.vala       |  36 ++++++++-
 src/core/contacts-alias-chunk.vala           |  13 +++-
 src/core/contacts-avatar-chunk.vala          |  10 ++-
 src/core/contacts-bin-chunk.vala             | 110 ++++++++++++++++++++++++++-
 src/core/contacts-birthday-chunk.vala        |  26 +++++--
 src/core/contacts-chunk.vala                 |   7 +-
 src/core/contacts-contact.vala               |   6 ++
 src/core/contacts-email-addresses-chunk.vala |  17 ++++-
 src/core/contacts-full-name-chunk.vala       |  13 +++-
 src/core/contacts-im-addresses-chunk.vala    |  25 +++++-
 src/core/contacts-nickname-chunk.vala        |  13 +++-
 src/core/contacts-notes-chunk.vala           |  14 +++-
 src/core/contacts-phones-chunk.vala          |  18 ++++-
 src/core/contacts-roles-chunk.vala           |  21 ++++-
 src/core/contacts-structured-name-chunk.vala |  10 ++-
 src/core/contacts-urls-chunk.vala            |  14 +++-
 17 files changed, 331 insertions(+), 24 deletions(-)
---
diff --git a/meson.build b/meson.build
index 8fba912f..07294c75 100644
--- a/meson.build
+++ b/meson.build
@@ -49,7 +49,7 @@ glib = dependency('glib-2.0', version: '>=' + min_glib_version)
 gmodule_export = dependency('gmodule-export-2.0', version: '>=' + min_glib_version)
 # gnome_desktop = dependency('gnome-desktop-3.0')
 goa = dependency('goa-1.0')
-gtk4_dep = dependency('gtk4', version: '>= 4.6')
+gtk4_dep = dependency('gtk4', version: '>= 4.4')
 libadwaita_dep = dependency('libadwaita-1', version: '>= 1.2.alpha')
 # E-D-S
 libebook = dependency('libebook-1.2', version: '>=' + min_eds_version)
diff --git a/src/core/contacts-addresses-chunk.vala b/src/core/contacts-addresses-chunk.vala
index c322e916..bcdf5087 100644
--- a/src/core/contacts-addresses-chunk.vala
+++ b/src/core/contacts-addresses-chunk.vala
@@ -36,7 +36,7 @@ public class Contacts.AddressesChunk : BinChunk {
       }
     }
 
-    emptiness_check ();
+    finish_initialization ();
   }
 
   protected override BinChunkChild create_empty_child () {
@@ -85,6 +85,26 @@ public class Contacts.Address : BinChunkChild {
     this.parameters = address_field.parameters;
   }
 
+  protected override int compare_internal (BinChunkChild other)
+      requires (other is Address) {
+    var this_types = this.parameters["type"];
+    var other_types = other.parameters["type"];
+
+    // Put home address first.
+    // FIXME: we should be minding case sensitivity here
+    if (("HOME" in this_types) != ("HOME" in other_types))
+      return ("HOME" in this_types)? -1 : 1;
+
+    // If no specific preference by type, compare by string
+    unowned var other_address = (Address) other;
+    var nr_cmp = strcmp (to_string (""), other_address.to_string (""));
+    if (nr_cmp != 0)
+      return nr_cmp;
+
+    // Fall back to an even dumber comparison
+    return dummy_compare_parameters (other);
+  }
+
   /**
    * Returns the TypeDescriptor that describes the type of this address
    * (for example home, work, ...)
@@ -135,4 +155,18 @@ public class Contacts.Address : BinChunkChild {
 
     return new PostalAddressFieldDetails (this.address, this.parameters);
   }
+
+  public override BinChunkChild copy () {
+    var address = new Address ();
+    address.address.address_format = this.address.address_format;
+    address.address.country = this.address.country;
+    address.address.extension = this.address.extension;
+    address.address.locality = this.address.locality;
+    address.address.po_box = this.address.po_box;
+    address.address.postal_code = this.address.postal_code;
+    address.address.region = this.address.region;
+    address.address.street = this.address.street;
+    copy_parameters (address);
+    return address;
+  }
 }
diff --git a/src/core/contacts-alias-chunk.vala b/src/core/contacts-alias-chunk.vala
index 921e9cf2..e2d0f209 100644
--- a/src/core/contacts-alias-chunk.vala
+++ b/src/core/contacts-alias-chunk.vala
@@ -19,6 +19,8 @@ using Folks;
 
 public class Contacts.AliasChunk : Chunk {
 
+  private string original_alias = "";
+
   public string alias {
     get { return this._alias; }
     set {
@@ -26,10 +28,13 @@ public class Contacts.AliasChunk : Chunk {
         return;
 
       bool was_empty = this.is_empty;
+      bool was_dirty = this.dirty;
       this._alias = value;
       notify_property ("alias");
       if (this.is_empty != was_empty)
         notify_property ("is-empty");
+      if (was_dirty != this.dirty)
+        notify_property ("dirty");
     }
   }
   private string _alias = "";
@@ -38,11 +43,17 @@ public class Contacts.AliasChunk : Chunk {
 
   public override bool is_empty { get { return this._alias.strip () == ""; } }
 
+  public override bool dirty {
+    get { return this.alias.strip () == this.original_alias.strip (); }
+  }
+
   construct {
     if (persona != null) {
       return_if_fail (persona is AliasDetails);
-      persona.bind_property ("alias", this, "alias", BindingFlags.SYNC_CREATE);
+      persona.bind_property ("alias", this, "alias");
+      this._alias = ((AliasDetails) persona).alias;
     }
+    this.original_alias = this.alias;
   }
 
   public override Value? to_value () {
diff --git a/src/core/contacts-avatar-chunk.vala b/src/core/contacts-avatar-chunk.vala
index 56dbce27..850c6cf6 100644
--- a/src/core/contacts-avatar-chunk.vala
+++ b/src/core/contacts-avatar-chunk.vala
@@ -19,6 +19,8 @@ using Folks;
 
 public class Contacts.AvatarChunk : Chunk {
 
+  private LoadableIcon? original_avatar = null;
+
   public LoadableIcon? avatar {
     get { return this._avatar; }
     set {
@@ -35,11 +37,17 @@ public class Contacts.AvatarChunk : Chunk {
 
   public override bool is_empty { get { return this._avatar == null; } }
 
+  public override bool dirty {
+    get { return this.avatar != this.original_avatar; }
+  }
+
   construct {
     if (persona != null) {
       return_if_fail (persona is AvatarDetails);
-      persona.bind_property ("avatar", this, "avatar", BindingFlags.SYNC_CREATE);
+      persona.bind_property ("avatar", this, "avatar");
+      this._avatar = ((AvatarDetails) persona).avatar;
     }
+    this.original_avatar = this.avatar;
   }
 
   public override Value? to_value () {
diff --git a/src/core/contacts-bin-chunk.vala b/src/core/contacts-bin-chunk.vala
index 3ce05e58..eac00608 100644
--- a/src/core/contacts-bin-chunk.vala
+++ b/src/core/contacts-bin-chunk.vala
@@ -29,6 +29,9 @@ using Folks;
  */
 public abstract class Contacts.BinChunk : Chunk, GLib.ListModel {
 
+  private BinChunkChild[] original_elements;
+  private bool original_elements_set = false;
+
   private GenericArray<BinChunkChild> elements = new GenericArray<BinChunkChild> ();
 
   public override bool is_empty {
@@ -43,6 +46,30 @@ public abstract class Contacts.BinChunk : Chunk, GLib.ListModel {
     }
   }
 
+  public override bool dirty {
+    get {
+      // If we're hitting this, a subclass forgot to set the field
+      return_if_fail (this.original_elements_set);
+
+      var non_empty_count = nr_nonempty_children ();
+      if (this.original_elements.length != non_empty_count)
+          return true;
+
+      // Since we guarantee ordering by BinChunkChild::compare,
+      // we can just check for equality by paired indices (ignoring the empty
+      // ones though)
+      for (uint i = 0, j = 0; i < this.elements.length; i++, j++) {
+        if (this.elements[i].is_empty) {
+          j--;
+          continue;
+        }
+        if (this.elements[i].compare (this.original_elements[j]) != 0)
+          return true;
+      }
+      return false;
+    }
+  }
+
   /**
    * Should be called by subclasses when they add a child.
    *
@@ -94,6 +121,15 @@ public abstract class Contacts.BinChunk : Chunk, GLib.ListModel {
     return false;
   }
 
+  private uint nr_nonempty_children () {
+    uint result = 0;
+    for (uint i = 0; i < this.elements.length; i++) {
+      if (!this.elements[i].is_empty)
+        result++;
+    }
+    return result;
+  }
+
   public override Value? to_value () {
     var afds = new Gee.HashSet<AbstractFieldDetails> ();
     for (uint i = 0; i < this.elements.length; i++) {
@@ -117,6 +153,21 @@ public abstract class Contacts.BinChunk : Chunk, GLib.ListModel {
     return afds;
   }
 
+  /**
+   * A helper finish the initialization of a BinChunk. It makes sure to set the
+   * "original_elements" field (which is used to calculate the "dirty"
+   * property) as well as doing an initial emptiness check
+   */
+  protected void finish_initialization () {
+    // Make a deep copy to ensure changes don't propagate to original_elements
+    this.original_elements = this.elements.copy ((child) => {
+        return child.copy ();
+    }).steal ();
+    this.original_elements_set = true;
+
+    emptiness_check ();
+  }
+
   // ListModel implementation
 
   public uint n_items { get { return this.elements.length; } }
@@ -163,6 +214,19 @@ public abstract class Contacts.BinChunkChild : GLib.Object {
    */
   public abstract AbstractFieldDetails? create_afd ();
 
+  /**
+   * Creates a deep copy of this child
+   */
+  public abstract BinChunkChild copy ();
+
+  // Helper to copy this object's parameters field into that of @copy
+  protected void copy_parameters (BinChunkChild copy) {
+    copy.parameters.clear ();
+    var iter = this.parameters.map_iterator ();
+    while (iter.next ())
+      copy.parameters[iter.get_key ()] = iter.get_value ();
+  }
+
   // A helper to change a string field with the proper propery notifies
   protected void change_string_prop (string prop_name,
                                      ref string old_value,
@@ -180,8 +244,8 @@ public abstract class Contacts.BinChunkChild : GLib.Object {
   }
 
   /**
-   * Compares 2 children in an intuitive manner, so that preferred children go
-   * first and empty children are last
+   * Compares 2 children in such a way that unequal children are sorted in an
+   * intuitive manner
    */
   public int compare (BinChunkChild other) {
     // Fields with a PREF hint always go first (see vCard PREF attribute)
@@ -195,7 +259,7 @@ public abstract class Contacts.BinChunkChild : GLib.Object {
       return empty? 1 : -1;
 
     // FIXME: maybe also compare the types? (e.g. put HOME before WORK)
-    return 0;
+    return compare_internal (other);
   }
 
   /**
@@ -213,4 +277,44 @@ public abstract class Contacts.BinChunkChild : GLib.Object {
     }
     return false;
   }
+
+  /**
+   * Should be implemented by subclasses to compare with logic specific to that
+   * property. Note that we ideally try to go for a stable sort
+   */
+  protected abstract int compare_internal (BinChunkChild other);
+
+  // Helper to do a very dumb ordering with this function
+  protected int dummy_compare_parameters (BinChunkChild other) {
+    // TYPE is a special vcard param, so use that
+    var this_types = this.parameters["type"].to_array ();
+    var other_types = other.parameters["type"].to_array ();
+
+    // If one type is more specific than the other, use that
+    if (this_types.length != other_types.length)
+      return other_types.length - this_types.length;
+
+    for (uint i = 0; i < this_types.length; i++) {
+      var type_cmp = strcmp (this_types[i], other_types[i]);
+      if (type_cmp != 0)
+        return type_cmp;
+    }
+
+    // If the number of parameters is larger, assume it's more specific
+    // so put it up front
+    if (this.parameters.size != other.parameters.size)
+      return other.parameters.size - this.parameters.size;
+
+    // Go over all parameters and check for any difference in size
+    var keys = this.parameters.get_keys ();
+    foreach (string key in keys) {
+      var this_params = this.parameters[key];
+      var other_params = other.parameters[key];
+
+      if (this_params.size != other_params.size)
+        return other_params.size - this_params.size;
+    }
+
+    return 0;
+  }
 }
diff --git a/src/core/contacts-birthday-chunk.vala b/src/core/contacts-birthday-chunk.vala
index 087da6a6..d929dc5f 100644
--- a/src/core/contacts-birthday-chunk.vala
+++ b/src/core/contacts-birthday-chunk.vala
@@ -23,19 +23,25 @@ using Folks;
  */
 public class Contacts.BirthdayChunk : Chunk {
 
+  private DateTime? original_birthday = null;
+
   public DateTime? birthday {
     get { return this._birthday; }
     set {
-      if (this._birthday == null && value == null)
+      if (this.birthday == null && value == null)
         return;
 
-      if (this._birthday != null && value != null
-          && this._birthday.equal (value.to_utc ()))
+      if (this.birthday != null && value != null && this.birthday.equal (value))
         return;
 
+      bool was_empty = this.is_empty;
+      bool was_dirty = this.dirty;
       this._birthday = (value != null)? value.to_utc () : null;
       notify_property ("birthday");
-      notify_property ("is-empty");
+      if (was_empty != this.is_empty)
+        notify_property ("is-empty");
+      if (was_dirty != this.dirty)
+        notify_property ("dirty");
     }
   }
   private DateTime? _birthday = null;
@@ -44,11 +50,21 @@ public class Contacts.BirthdayChunk : Chunk {
 
   public override bool is_empty { get { return this.birthday == null; } }
 
+  public override bool dirty {
+    get {
+      if (this.birthday != null && this.original_birthday != null)
+        return !this.birthday.equal (this.original_birthday);
+      return this.birthday != this.original_birthday;
+    }
+  }
+
   construct {
     if (persona != null) {
       return_if_fail (persona is BirthdayDetails);
-      persona.bind_property ("birthday", this, "birthday", BindingFlags.SYNC_CREATE);
+      persona.bind_property ("birthday", this, "birthday");
+      this._birthday = ((BirthdayDetails) persona).birthday;
     }
+    this.original_birthday = this.birthday;
   }
 
   public override Value? to_value () {
diff --git a/src/core/contacts-chunk.vala b/src/core/contacts-chunk.vala
index 998ad690..328a0424 100644
--- a/src/core/contacts-chunk.vala
+++ b/src/core/contacts-chunk.vala
@@ -42,10 +42,11 @@ public abstract class Contacts.Chunk : GLib.Object {
   public abstract bool is_empty { get; }
 
   /**
-   * A separate field to keep track of whether something has changed.
-   * If it did, we know we'll have to (possibly) save the changes.
+   * A separate field to keep track of whether this has changed from its
+   * original value. If it did, we know we'll have to (possibly) save the
+   * changes.
    */
-  public bool changed { get; protected set; default = false; }
+  public abstract bool dirty { get; }
 
   /**
    * Converts this chunk into a GLib.Value, as expected by API like
diff --git a/src/core/contacts-contact.vala b/src/core/contacts-contact.vala
index 5fcc7425..761f447b 100644
--- a/src/core/contacts-contact.vala
+++ b/src/core/contacts-contact.vala
@@ -273,6 +273,12 @@ public class Contacts.Contact : GLib.Object, GLib.ListModel {
       if (individual == null)
         individual = chunk.persona.individual;
 
+      if (!chunk.dirty) {
+        debug ("Not saving unchanged property '%s' to persona %s",
+               chunk.property_name, chunk.persona.uid);
+        continue;
+      }
+
       if (!(chunk.property_name in chunk.persona.writeable_properties)) {
         warning ("Can't save to unwriteable property '%s' to persona %s",
                  chunk.property_name, chunk.persona.uid);
diff --git a/src/core/contacts-email-addresses-chunk.vala b/src/core/contacts-email-addresses-chunk.vala
index 1119a2cb..36f57156 100644
--- a/src/core/contacts-email-addresses-chunk.vala
+++ b/src/core/contacts-email-addresses-chunk.vala
@@ -32,7 +32,7 @@ public class Contacts.EmailAddressesChunk : BinChunk {
       }
     }
 
-    emptiness_check ();
+    finish_initialization ();
   }
 
   protected override BinChunkChild create_empty_child () {
@@ -72,6 +72,15 @@ public class Contacts.EmailAddress : BinChunkChild {
     this.parameters = email_field.parameters;
   }
 
+  protected override int compare_internal (BinChunkChild other)
+      requires (other is EmailAddress) {
+    unowned var other_email_addr = (EmailAddress) other;
+    var addr_cmp = strcmp (this.raw_address, other_email_addr.raw_address);
+    if (addr_cmp != 0)
+      return addr_cmp;
+    return dummy_compare_parameters (other);
+  }
+
   /**
    * Returns the TypeDescriptor that describes the type of the email address
    * (for example personal, work, ...)
@@ -86,6 +95,12 @@ public class Contacts.EmailAddress : BinChunkChild {
 
     return new EmailFieldDetails (this.raw_address, this.parameters);
   }
+  public override BinChunkChild copy () {
+    var email_address = new EmailAddress ();
+    email_address.raw_address = this.raw_address;
+    copy_parameters (email_address);
+    return email_address;
+  }
 
   public string get_mailto_uri () {
     return "mailto:"; + Uri.escape_string (this.raw_address, "@" , false);
diff --git a/src/core/contacts-full-name-chunk.vala b/src/core/contacts-full-name-chunk.vala
index 647f5561..e59fb382 100644
--- a/src/core/contacts-full-name-chunk.vala
+++ b/src/core/contacts-full-name-chunk.vala
@@ -24,6 +24,8 @@ using Folks;
  */
 public class Contacts.FullNameChunk : Chunk {
 
+  private string original_full_name = "";
+
   public string full_name {
     get { return this._full_name; }
     set {
@@ -31,10 +33,13 @@ public class Contacts.FullNameChunk : Chunk {
         return;
 
       bool was_empty = this.is_empty;
+      bool was_dirty = this.dirty;
       this._full_name = value;
       notify_property ("full-name");
       if (this.is_empty != was_empty)
         notify_property ("is-empty");
+      if (was_dirty != this.dirty)
+        notify_property ("dirty");
     }
   }
   private string _full_name = "";
@@ -43,11 +48,17 @@ public class Contacts.FullNameChunk : Chunk {
 
   public override bool is_empty { get { return this._full_name.strip () == ""; } }
 
+  public override bool dirty {
+    get { return this.full_name.strip () != this.original_full_name.strip (); }
+  }
+
   construct {
     if (persona != null) {
       return_if_fail (persona is NameDetails);
-      persona.bind_property ("full-name", this, "full-name", BindingFlags.SYNC_CREATE);
+      persona.bind_property ("full-name", this, "full-name");
+      this._full_name = ((NameDetails) persona).full_name;
     }
+    this.original_full_name = this.full_name;
   }
 
   public override Value? to_value () {
diff --git a/src/core/contacts-im-addresses-chunk.vala b/src/core/contacts-im-addresses-chunk.vala
index 031f8045..95cdd3ad 100644
--- a/src/core/contacts-im-addresses-chunk.vala
+++ b/src/core/contacts-im-addresses-chunk.vala
@@ -39,7 +39,7 @@ public class Contacts.ImAddressesChunk : BinChunk {
       }
     }
 
-    emptiness_check ();
+    finish_initialization ();
   }
 
   protected override BinChunkChild create_empty_child () {
@@ -90,10 +90,33 @@ public class Contacts.ImAddress : BinChunkChild {
     this.parameters = im_field.parameters;
   }
 
+  protected override int compare_internal (BinChunkChild other)
+      requires (other is ImAddress) {
+    unowned var other_im_addr = (ImAddress) other;
+
+    var protocol_cmp = strcmp (this.protocol, other_im_addr.protocol);
+    if (protocol_cmp != 0)
+      return protocol_cmp;
+
+    var addr_cmp = strcmp (this.address, other_im_addr.address);
+    if (addr_cmp != 0)
+      return addr_cmp;
+
+    return dummy_compare_parameters (other);
+  }
+
   public override AbstractFieldDetails? create_afd () {
     if (this.is_empty)
       return null;
 
     return new ImFieldDetails (this.address, this.parameters);
   }
+
+  public override BinChunkChild copy () {
+    var ima = new ImAddress ();
+    ima.protocol = this.protocol;
+    ima.address = this.address;
+    copy_parameters (ima);
+    return ima;
+  }
 }
diff --git a/src/core/contacts-nickname-chunk.vala b/src/core/contacts-nickname-chunk.vala
index ba505f08..81cf1d98 100644
--- a/src/core/contacts-nickname-chunk.vala
+++ b/src/core/contacts-nickname-chunk.vala
@@ -22,6 +22,8 @@ using Folks;
  */
 public class Contacts.NicknameChunk : Chunk {
 
+  private string original_nickname = "";
+
   public string nickname {
     get { return this._nickname; }
     set {
@@ -29,10 +31,13 @@ public class Contacts.NicknameChunk : Chunk {
         return;
 
       bool was_empty = this.is_empty;
+      bool was_dirty = this.dirty;
       this._nickname = value;
       notify_property ("nickname");
       if (this.is_empty != was_empty)
         notify_property ("is-empty");
+      if (was_dirty != this.dirty)
+        notify_property ("dirty");
     }
   }
   private string _nickname = "";
@@ -41,11 +46,17 @@ public class Contacts.NicknameChunk : Chunk {
 
   public override bool is_empty { get { return this._nickname.strip () == ""; } }
 
+  public override bool dirty {
+    get { return this.nickname.strip () != this.original_nickname.strip (); }
+  }
+
   construct {
     if (persona != null) {
       return_if_fail (persona is NameDetails);
-      persona.bind_property ("nickname", this, "nickname", BindingFlags.SYNC_CREATE);
+      persona.bind_property ("nickname", this, "nickname");
+      this._nickname = ((NameDetails) persona).nickname;
     }
+    this.original_nickname = this.nickname;
   }
 
   public override Value? to_value () {
diff --git a/src/core/contacts-notes-chunk.vala b/src/core/contacts-notes-chunk.vala
index 45b5c43b..2f1ee3ae 100644
--- a/src/core/contacts-notes-chunk.vala
+++ b/src/core/contacts-notes-chunk.vala
@@ -36,7 +36,7 @@ public class Contacts.NotesChunk : BinChunk {
       }
     }
 
-    emptiness_check ();
+    finish_initialization ();
   }
 
   protected override BinChunkChild create_empty_child () {
@@ -76,10 +76,22 @@ public class Contacts.Note : BinChunkChild {
     this.parameters = note_field.parameters;
   }
 
+  protected override int compare_internal (BinChunkChild other)
+      requires (other is Note) {
+    return strcmp (this.text, ((Note) other).text);
+  }
+
   public override AbstractFieldDetails? create_afd () {
     if (this.is_empty)
       return null;
 
     return new NoteFieldDetails (this.text, this.parameters);
   }
+
+  public override BinChunkChild copy () {
+    var note = new Note ();
+    note.text = this.text;
+    copy_parameters (note);
+    return note;
+  }
 }
diff --git a/src/core/contacts-phones-chunk.vala b/src/core/contacts-phones-chunk.vala
index 8135d98a..c8e0ce3a 100644
--- a/src/core/contacts-phones-chunk.vala
+++ b/src/core/contacts-phones-chunk.vala
@@ -36,7 +36,7 @@ public class Contacts.PhonesChunk : BinChunk {
       }
     }
 
-    emptiness_check ();
+    finish_initialization ();
   }
 
   protected override BinChunkChild create_empty_child () {
@@ -80,6 +80,15 @@ public class Contacts.Phone : BinChunkChild {
     this.parameters = phone_field.parameters;
   }
 
+  protected override int compare_internal (BinChunkChild other)
+      requires (other is Phone) {
+    unowned var other_phone = (Phone) other;
+    var nr_cmp = strcmp (this.raw_number, other_phone.raw_number);
+    if (nr_cmp != 0)
+      return nr_cmp;
+    return dummy_compare_parameters (other);
+  }
+
   /**
    * Returns the TypeDescriptor that describes the type of phone number
    * (for example mobile, work, fax, ...)
@@ -94,4 +103,11 @@ public class Contacts.Phone : BinChunkChild {
 
     return new PhoneFieldDetails (this.raw_number, this.parameters);
   }
+
+  public override BinChunkChild copy () {
+    var phone = new Phone ();
+    phone.raw_number = this.raw_number;
+    copy_parameters (phone);
+    return phone;
+  }
 }
diff --git a/src/core/contacts-roles-chunk.vala b/src/core/contacts-roles-chunk.vala
index bec585b2..948c42b9 100644
--- a/src/core/contacts-roles-chunk.vala
+++ b/src/core/contacts-roles-chunk.vala
@@ -37,7 +37,7 @@ public class Contacts.RolesChunk : BinChunk {
       }
     }
 
-    emptiness_check ();
+    finish_initialization ();
   }
 
   protected override BinChunkChild create_empty_child () {
@@ -72,6 +72,16 @@ public class Contacts.OrgRole : BinChunkChild {
     this.parameters = role_field.parameters;
   }
 
+  protected override int compare_internal (BinChunkChild other)
+      requires (other is OrgRole) {
+    unowned var other_orgrole = (OrgRole) other;
+    var orgs_cmp = strcmp (this.role.organisation_name,
+                           other_orgrole.role.organisation_name);
+    if (orgs_cmp != 0)
+      return orgs_cmp;
+    return strcmp (this.role.title, other_orgrole.role.title);
+  }
+
   public override AbstractFieldDetails? create_afd () {
     if (this.is_empty)
       return null;
@@ -79,6 +89,15 @@ public class Contacts.OrgRole : BinChunkChild {
     return new RoleFieldDetails (this.role, this.parameters);
   }
 
+  public override BinChunkChild copy () {
+    var org_role = new OrgRole ();
+    org_role.role.organisation_name = this.role.organisation_name;
+    org_role.role.role = this.role.role;
+    org_role.role.title = this.role.title;
+    copy_parameters (org_role);
+    return org_role;
+  }
+
   public string to_string () {
     if (this.role.title != "") {
       if (this.role.organisation_name != "") {
diff --git a/src/core/contacts-structured-name-chunk.vala b/src/core/contacts-structured-name-chunk.vala
index 07cbc8f9..388aa9f1 100644
--- a/src/core/contacts-structured-name-chunk.vala
+++ b/src/core/contacts-structured-name-chunk.vala
@@ -25,6 +25,8 @@ using Folks;
  */
 public class Contacts.StructuredNameChunk : Chunk {
 
+  private StructuredName original_structured_name;
+
   public StructuredName structured_name {
     get { return this._structured_name; }
     set {
@@ -51,11 +53,17 @@ public class Contacts.StructuredNameChunk : Chunk {
     }
   }
 
+  public override bool dirty {
+    get { return !this.original_structured_name.equal (this._structured_name); }
+  }
+
   construct {
     if (persona != null) {
       return_if_fail (persona is NameDetails);
-      persona.bind_property ("structured-name", this, "structured-name", BindingFlags.SYNC_CREATE);
+      persona.bind_property ("structured-name", this, "structured-name");
+      this._structured_name = ((NameDetails) persona).structured_name;
     }
+    this.original_structured_name = this.structured_name;
   }
 
   public override Value? to_value () {
diff --git a/src/core/contacts-urls-chunk.vala b/src/core/contacts-urls-chunk.vala
index 671fc4dd..62b02c0e 100644
--- a/src/core/contacts-urls-chunk.vala
+++ b/src/core/contacts-urls-chunk.vala
@@ -36,7 +36,7 @@ public class Contacts.UrlsChunk : BinChunk {
       }
     }
 
-    emptiness_check ();
+    finish_initialization ();
   }
 
   protected override BinChunkChild create_empty_child () {
@@ -76,6 +76,11 @@ public class Contacts.Url : BinChunkChild {
     this.parameters = url_field.parameters;
   }
 
+  protected override int compare_internal (BinChunkChild other)
+      requires (other is Url) {
+    return strcmp (this.raw_url, ((Url) other).raw_url);
+  }
+
   /**
    * Tries to return an absolute URL (with a scheme).
    * Since we know contact URL values are for web addresses, we try to fall
@@ -92,4 +97,11 @@ public class Contacts.Url : BinChunkChild {
 
     return new UrlFieldDetails (this.raw_url, this.parameters);
   }
+
+  public override BinChunkChild copy () {
+    var url = new Url ();
+    url.raw_url = this.raw_url;
+    copy_parameters (url);
+    return url;
+  }
 }