Created attachment 139274 [details] [review]
Acknowledge rights element, though we obey access instead

Probably a better solution would be to add support for <rights> in GDataEntry.

Created attachment 139461 [details] [review]
atom:rights support in gdata-entry.c 

Ha, I'm silly and confused <rights> with <gphoto:access> anyway. 

Here is one adding support for <atom:rights> to gdata-entry.c.  The atom spec suggests that feeds can have them too, so I'll add analogous logic to gdata-feed.c unless there's a more generic way to handle both, but a few other elements seem to be duplicated between them as well.

Created attachment 139463 [details] [review]
<atom:rights> support for GDataFeed

And here it is for gdata-feed.c and stuff.

You're missing a gdata_entry_set_rights() function.

> +	} else if (xmlStrcmp (node->name, (xmlChar*) "rights") == 0) {
> +		/* atom:rights */
> +		xmlFree (self->priv->rights);
> +		self->priv->rights = (gchar*) xmlNodeListGetString (doc, node->children, TRUE);

This will break things. If you're able to set the property value from a GValue or with a setter function (gdata_entry_set_rights()), you'll end up trying to free memory allocated with g_malloc() with xmlFree(). Bad things will happen.

This is one of the situations where you should g_strdup() what you get from xmlNodeListGetString(), just like the code for the <summary> element does, above.

Could you also add something to ensure that GDataEntry:rights and GDataPicasaWebAlbum:visibility stay synchronised, please?

For the moment, I don't think we need <rights> support in GDataFeed. The spec may say it's possible, but I'm taking a lazy approach to adding element handling; I haven't yet seen any services which use <feed/rights>, so there's no need to complicate the libgdata API with it.

Created attachment 139687 [details] [review]
Updated patch for GDataEntry <rights> support

> You're missing a gdata_entry_set_rights() function.
I wasn't originally intending on adding support for setting it.  I suppose I should really add a query for updating albums and files so I can test actually setting values on the server :|

> > +	} else if (xmlStrcmp (node->name, (xmlChar*) "rights") == 0) {
> > +		/* atom:rights */
> > +		xmlFree (self->priv->rights);
> > +		self->priv->rights = (gchar*) xmlNodeListGetString (doc, node->children, TRUE);
> 
> This will break things. If you're able to set the property value from a GValue
> or with a setter function (gdata_entry_set_rights()), you'll end up trying to
> free memory allocated with g_malloc() with xmlFree(). Bad things will happen.
> This is one of the situations where you should g_strdup() what you get from
> xmlNodeListGetString(), just like the code for the <summary> element does,
> above.
> 
D'oh.  Fixed.  Really must be more alert about this :|

> Could you also add something to ensure that GDataEntry:rights and
> GDataPicasaWebAlbum:visibility stay synchronised, please?
The callbacks also check whether they already match, so that they don't ping-pong against each other's updates. 

> For the moment, I don't think we need <rights> support in GDataFeed. The spec
> may say it's possible, but I'm taking a lazy approach to adding element
> handling; I haven't yet seen any services which use <feed/rights>, so there's
> no need to complicate the libgdata API with it.
Yah.  While the feed for querying an albums files includes <rights> for the feed's album, PicasaWeb's albums will have enough ways to access and modify their visibility :) 

There are a couple questions about what to do with unknown values for rights and GDataPicasaWebVisibility when keeping them in sync.

I've committed the patch with some modifications, most of which are listed below. See what I've committed for my solution to the unknown values for rights.

> +	case PROP_RIGHTS:
> +		self->priv->rights = g_value_dup_string (value);
> +		break;

You should have called gdata_entry_set_rights() here, so that you free the previous rights value and also send a notification of the change to the property.

> +	/* Update our gphoto:visibility */
> +	if (g_strcmp0 ("public", rights) == 0 && visibility != GDATA_PICASAWEB_PUBLIC) {

I'm not a fan of this approach to breaking infinite loops. It's generally the practice to block signal emission of the competing signal while you change the values, instead. See what I've committed to see what I mean.

> +	/* Test visibility and its synchronisation with its GDataEntry's rights */
> +	original_rights = gdata_entry_get_rights (GDATA_ENTRY (album));

This was causing the test suite to crash for me, since you keep a pointer to memory which is subsequently freed, then attempt to use it. It now makes a copy of the original rights instead.

commit c04a3fa92d193c2ae1cd68174129dfb3bb56f2be
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Sun Aug 2 17:08:42 2009 +0100

    Bug 589855 – Obsolete <rights> not handled in GDataPicasaWebAlbum parse_xml()
    
    Changes based on a patch from Richard Schwarting <aquarichy@gmail.com> to
    handle the Atom <rights> element in GDataEntry. Closes: bgo#589855

 docs/reference/gdata-sections.txt                |    2 +
 gdata/gdata-entry.c                              |   72 +++++++++++++++++++++-
 gdata/gdata-entry.h                              |    2 +
 gdata/gdata.symbols                              |    2 +
 gdata/services/picasaweb/gdata-picasaweb-album.c |   63 +++++++++++++++++--
 gdata/tests/picasaweb.c                          |   27 ++++++++
 6 files changed, 160 insertions(+), 8 deletions(-)