(In reply to comment #0)
> The main purpose of this change is to support the lossless expression of our
> postal addresses as vCard attributes (and vice versa).
> 
> Proposed API adds:
> 
> public class FieldVariantDetails
> {
>   public GVariant value { get; set; }
>   public MultiMap<string, string> parameters { get; set; }
> } 

Presumably “parameters” is a replacement for “types”, but what's the GVariant for?

(In reply to comment #1)
> (In reply to comment #0)
> > The main purpose of this change is to support the lossless expression of our
> > postal addresses as vCard attributes (and vice versa).
> > 
> > Proposed API adds:
> > 
> > public class FieldVariantDetails
> > {
> >   public GVariant value { get; set; }
> >   public MultiMap<string, string> parameters { get; set; }
> > } 
> 
> Presumably “parameters” is a replacement for “types”, but what's the GVariant
> for?

The idea was that we could stuff any type into value (so we don't have to make this PostalAddress-specific), but it should actually be GValue, not GVariant.

(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > The main purpose of this change is to support the lossless expression of our
> > > postal addresses as vCard attributes (and vice versa).
> > > 
> > > Proposed API adds:
> > > 
> > > public class FieldVariantDetails
> > > {
> > >   public GVariant value { get; set; }
> > >   public MultiMap<string, string> parameters { get; set; }
> > > } 
> > 
> > Presumably “parameters” is a replacement for “types”, but what's the GVariant
> > for?
> 
> The idea was that we could stuff any type into value (so we don't have to make
> this PostalAddress-specific), but it should actually be GValue, not GVariant.

Sorry, I'm still not sure what you mean. :-( Could you give an example, perhaps?

Ugh, GValue is teh suck. Why not make FieldDetailed (or suchlike) a superclass of PostalAddress instead, then it can be reused by other classes if needed.

Or, if you really want to combine them, make it

class FieldAnyDetails<T> {
  T value;
  MultiMap<string, string> parameters;
}

Although I'm not sure how well that would map into C.

Btw, both of these approaches can be used by the current FieldDetails too.

Another alternative is to make the parameter part an interface that the various classes implement.

Wouldn't it be easier to just make:

interface PostalAddressDetails
{
-  public abstract Set<PostalAddress> postal_addresses { get; set; }
+  public abstract MultiMap<string, PostalAddress> postal_addresses { get; set; }
}

so you can conveniently map types/PostalAddresses.

And then for extra params:

PostalAddress
{
-  public Set<string> types
+  public MultiMap<string, string> parameters { get; set; }
}

I think this actually what was suggested by Alex here:

https://bugzilla.gnome.org/show_bug.cgi?id=638281#c102

I don't see why you would want a MultiMap for the PostalAddressDetails, but the Set type => MultiMap paramaters is what i suggested. That is basically what travis proposed though, except his is a bit generalized so it could be used for other properties too.

Created attachment 191530 [details] [review]
Support vCard-like parameters for postal addresses

Patches from branch:

http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo653679-postal-address-changes-2

============

This rebases FieldDetails upon a generic AbstractFieldDetails, then adds PostalAddressFieldDetails : AbstractFieldDetails and adjusts PostalAddressDetails and all dependencies accordingly.

This should be a suitable pattern for creating class-specific *FieldDetails as described in bug #653682 comment #1 and closing bug #653682 and bug #653680.

After all this, I'll cut out FieldDetails entirely.

I haven't had a chance to review the patch properly yet, but one thought has come to mind: why not just make FieldDetails abstract, instead of adding a new AbstractFieldDetails and then removing FieldDetails? Since the methods on both of them are the same, it seems to me that we can avoid a load of s/FieldDetails/AbstractFieldDetails/ changes in (e.g.) GNOME Contacts that way.

(In reply to comment #10)
> I haven't had a chance to review the patch properly yet, but one thought has
> come to mind: why not just make FieldDetails abstract, instead of adding a new
> AbstractFieldDetails and then removing FieldDetails? Since the methods on both
> of them are the same, it seems to me that we can avoid a load of
> s/FieldDetails/AbstractFieldDetails/ changes in (e.g.) GNOME Contacts that way.

The main reason I didn't do it that way was that it seemed better to make an obviously-incompatible change than to make a non-obviously-incompatible change (making FieldDetails suddenly abstract).

On the other hand, it's probably worth avoiding churn in the clients using the FieldDetails functions which will still be applicable to subclassed objects. I'll just s/AbstractFieldDetails/FieldDetails/ for my next version.

Review of attachment 191530 [details] [review]:

A few minor comments. I'd go ahead and commit once these (and the s/AbstractFieldDetails/FieldDetails/ change) are fixed.

::: folks/abstract-field-details.vala
@@ +36,3 @@
+ * @since UNRELEASED
+ */
+public abstract class Folks.AbstractFieldDetails<T> : Object

The generic type parameter doesn't appear to be being used for anything. Would it make sense to add a “value” property of type T, or just to remove the type parameter?

::: folks/field-details.vala
@@ +53,2 @@
    *
+   * See {@link Folks.VcardParams.parameters}.

s/VcardParams/AbstractFieldDetails/? (Though this is no longer relevant.)

::: folks/postal-address-details.vala
@@ +199,3 @@
 /**
+ * Object representing a PostalAddress value that can have some parameters
+ * associated with it.

Might be helpful to add a brief paragraph on how PostalAddress.types is implemented using the parameters of the FieldDetails instance.

@@ +212,3 @@
+   *
+   * The value of the field, the exact content depends on what the structure is
+   * used for.

I don't think this description's relevant any more. It should always be a postal address, should it not?

@@ +222,3 @@
+   * The parameters of this PostalAddressFieldDetails.
+   *
+   * See {@link Folks.VcardParams.parameters}.

s/VcardParams/FieldDetails/, I think. This documentation comment is also missing a @since tag.

Created attachment 192063 [details] [review]
Support vCard-like parameters for postal addresses (try 2)

This adds a number of significant clean-ups since the original branch. Examples include: adding a parameters argument to FieldDetails, adding and using equal() and hash() functions for all structures using AbstractFieldDetails-derived objects).

It should also address all the original review points.

The patterns used in this branch should be applicable for all the other places we do or should use FieldDetails now.

Patches from branch:

http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo653679-postal-address-changes-9

(In reply to comment #13)
> Created an attachment (id=192063) [details] [review]
> Support vCard-like parameters for postal addresses (try 2)

And, of course, I'll apply the necessary changes to the EDS backend and tests when I rebase before merging.

Review of attachment 192063 [details] [review]:

Looks OK to me.

::: folks/abstract-field-details.vala
@@ +78,3 @@
+        {
+          return null;
+        }

The two paragraphs above are fairly similar. The first one could probably be removed.

@@ +89,3 @@
+   * `parameter_value` is added to the existing ones.
+   *
+   * @param parameter_name the name of the parameter

Missing space before the equals sign.

::: folks/field-details.vala
@@ +20,1 @@
  */

Should probably mention that null is equivalent to the empty collection of parameters.

::: folks/postal-address-details.vala
@@ +199,3 @@
 /**
+ * Object representing a PostalAddress value that can have some parameters
+ * associated with it.

See my third point from the review in comment #12.

Aaargh, Splinter has mucked the review up. How are you generating your patches, because it's making poor Splinter cry.

See below for the corrected locations my comments were referring to.

(In reply to comment #15)
> Review of attachment 192063 [details] [review]:
> 
> Looks OK to me.
> 
> ::: folks/abstract-field-details.vala
> @@ +78,3 @@
> +        {
> +          return null;
> +        }
> 
> The two paragraphs above are fairly similar. The first one could probably be
> removed.

This refers to the documentation comment for AbstractFieldDetails.hash().

> @@ +89,3 @@
> +   * `parameter_value` is added to the existing ones.
> +   *
> +   * @param parameter_name the name of the parameter
> 
> Missing space before the equals sign.

This refers to the default implementation of AbstractFieldDetails.hash().

> ::: folks/field-details.vala
> @@ +20,1 @@
>   */
> 
> Should probably mention that null is equivalent to the empty collection of
> parameters.

This refers to the documentation for the “parameters” argument to the FieldDetails constructor.

> ::: folks/postal-address-details.vala
> @@ +199,3 @@
>  /**
> + * Object representing a PostalAddress value that can have some parameters
> + * associated with it.
> 
> See my third point from the review in comment #12.

This is referring to the correct place.

commit ac0217fabf327e3a04701d33ca75332d1a3bfa06
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Thu Jul 14 11:38:31 2011 -0700

    Support vCard-like parameters for addresses in PostalAddressDetails.
    
    This also removes the older, less-functional, less-consistent
    PostalAddress.types.
    
    Helps: bgo#653679 - Change PostalAddressDetails.postal_addresses to
    support vCard-like arbitrary parameters

commit 13b45043d212db0efe363dd9eab4116e4d715bc7
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Tue Jul 12 11:48:43 2011 -0700

    Add test for parameter equality in FieldDetails.
    
    Helps: bgo#653679 - Change PostalAddressDetails.postal_addresses to
    support vCard-like arbitrary parameters

commit 136ae0924b2605580342b60087b2c124819d15a8
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Tue Jul 12 11:17:12 2011 -0700

    Add basic equality testing for FieldDetails.
    
    Helps: bgo#653679 - Change PostalAddressDetails.postal_addresses to
    support vCard-like arbitrary parameters

commit a51d03d7366d5803532b3395292ce7137c0b08ef
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Fri Jul 15 16:36:33 2011 -0700

    Add parameters as an optional argument to the FieldDetails constructor.
    
    Helps: bgo#653679 - Change PostalAddressDetails.postal_addresses to
    support vCard-like arbitrary parameters

commit 486a993849b4bf6a1bef6937af62e642d4bfbfec
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Wed Jul 13 15:35:00 2011 -0700

    Implement FieldDetails.equal()/hash() in terms of the default.
    
    This allows explicit references to FieldDetails.equal()/hash() for
    structures storing them (in case it changes from the
    AbstractFieldDetails implementation in the future).
    
    Helps: bgo#653679 - Change PostalAddressDetails.postal_addresses to
    support vCard-like arbitrary parameters

commit 8b08293c7037fffa5a46c540b46900e83d549cfb
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Mon Jul 11 17:20:54 2011 -0700

    Add equal() and hash() functions to AbstractFieldDetails.
    
    Helps: bgo#653679 - Change PostalAddressDetails.postal_addresses to
    support vCard-like arbitrary parameters

commit af60852d7edb10054621bbfb67134d93cf4dfb1b
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Thu Jul 14 16:57:01 2011 -0700

    Centralize parameters to AbstractFieldDetails.
    
    This cuts some lines of code from the derived classes.
    
    Helps: bgo#653679 - Change PostalAddressDetails.postal_addresses to
    support vCard-like arbitrary parameters

commit 5f0ab0032ba17ce54334387a129ea0ee0f7c64c9
Author: Travis Reitter <travis.reitter@collabora.co.uk>
Date:   Thu Jul 7 11:10:58 2011 -0700

    Rebase FieldDetails upon an abstract and generic version of itself.
    
    This should be useful for cases where the value has a very specific type
    (eg, for PostalAddressDetails.postal_addresses). We'll create subclasses of
    FieldDetails for those cases.
    
    When we've created all the appropriate derived classes, we'll remove
    FieldDetails entirely.

    Helps: bgo#653679 - Change PostalAddressDetails.postal_addresses to
    support vCard-like arbitrary parameters