Going to implement something like GDATA_TYPE_CALENDAR_FEED.  

Yes, that's the right thing to do. However, you should check first to see if the feed class should be specific to the query which is producing that unhandled XML, or if the class can be general across all PicasaWeb queries. (i.e. Does it make sense to expose a user, nickname, thumbnail, quota limit, etc. across all PicasaWeb queries? Do we get given the XML from all of them?)

Note also that <icon> support should be added to GDataQuery, rather than in a new class.

(In reply to comment #2)
> Note also that <icon> support should be added to GDataQuery, rather than in a
> new class.

Whoops. It should be added to GDataFeed, sorry.

Hehe.  I started figuring that.  I'm starting to look at the other services again to get the best idea of how to do this.

I noticed that the properties of GDataPicasaWebAlbum, which we currently get from <entry> nodes from a <feed> of queried albums, are also contained as elements of the feed in a query for an album's photo entries.  

Would we want to bother parsing those again?  GDataPicasaWebService's API for listing files for an album requires supplying GDataPicasaWebAlbum which assumes someone already parsed the <feed> of album entries from the albums queries, so it doesn't seem necessary.

(In reply to comment #4)
> Would we want to bother parsing those again?  GDataPicasaWebService's API for
> listing files for an album requires supplying GDataPicasaWebAlbum which assumes
> someone already parsed the <feed> of album entries from the albums queries, so
> it doesn't seem necessary.

No, we don't want to parse them again. Let's not make the API unnecessarily bloated. Patches welcome to ignore those nodes when they're children of a <feed>.

Created attachment 144696 [details] [review]
Patch adding support for atom:icon to GDataFeed

GDataFeed support for atom:icon, modelled after support for atom:logo, + a test using a PicasaWeb Album's feed.

Created attachment 144698 [details] [review]
Patch adding support for gphoto:access as visibility to GDataPicasaWebFile

Now that I've written this, I'm beginning to wonder whether it was decided to not support visibility directly in GDataPicasaWebFile yet because PicasaWeb doesn't yet offer fine-grained access control over individual photos and videos, only their albums.

The <gphoto:access> element is included in photos' GDataEntry elements, it's nice to have the visibility information without grabbing the album first, but I understand if you'd rather not store information accessible through the album again on the file. 

If we don't end up supporting <gphoto:access> on photo's themselves, is there some appropriate way to suppress the "Unhandled XML" messages?


About the patch:
Based off of GDataPicasaWebAlbum's support for gphoto:access as visibility.

One issue with that is, in reusing GDataPicasaWebVisibility, I am including "gdata-picasaweb-album.h" in gdata-picasaweb-file.h for its definition.  Should GDataPicasaWebVisibility have its definition stored in gdata-picasaweb-enums.h instead (which seems to be autogenerated)?

Yah, no setters since we cannot control access rights at the file level right now.

WHAT THERE IS NOW:

Concerning remaining "Unhandled XML" messages, 

So it seems the main elements I'm interested in from a GDataFeed of albums from gdata_picasaweb_service_query_all_albums() are these:
<gphoto:user>libgdata.picasaweb</gphoto:user>
<gphoto:nickname>libgdata.picasaweb</gphoto:nickname>
<gphoto:thumbnail>http://lh6.ggpht.com/_1kdcGyvOb8c/AAAA9mDag3s/AAAAAAAAAAA/Jq-NWYWKFao/s64-c/libgdata.picasaweb.jpg</gphoto:thumbnail>
<gphoto:quotalimit>1073741824</gphoto:quotalimit>
<gphoto:quotacurrent>6712246</gphoto:quotacurrent>
<gphoto:maxPhotosPerAlbum>500</gphoto:maxPhotosPerAlbum>

These seem to describe the user who is logged in and their account, as opposed to the owners of the albums in the feed.  That's good.


From the GDataFeed of a given album and its photos, got from gdata_picasaweb_service_query_files(), I get 
<gphoto:id>5348972684386663249</gphoto:id>
<gphoto:location/>
<gphoto:access>private</gphoto:access>
<gphoto:timestamp>1245404752000</gphoto:timestamp>
<gphoto:numphotos>287</gphoto:numphotos>
<gphoto:numphotosremaining>213</gphoto:numphotosremaining>
<gphoto:bytesUsed>5002462</gphoto:bytesUsed>
<gphoto:user>libgdata.picasaweb</gphoto:user>
<gphoto:nickname>libgdata.picasaweb</gphoto:nickname>
<gphoto:allowPrints>true</gphoto:allowPrints>
<gphoto:allowDownloads>true</gphoto:allowDownloads>

id, location, access, timestamp, numphotos, numphotosremaining, bytesUsed, user, and nickname seem to be redundant with elements contained within the album's entry from the feed of albums, and which we capture in GDataPicasaWebAlbum already. 

allowPrints and allowDownloads seem to not be available elsewhere.   



FINALLY, THE POINT: 

So, I'm inclined to add two new Feed classes, with names along the line of 
from gdata_picasaweb_service_query_all_albums*:
GDataPicasaWebAlbumsFeed
from gdata_picasaweb_service_query_files*:
GDataPicasaWebFilesFeed 

It's a bit unintuitive to think that your accounts information is in the *AlbumsFeed, but it would avoid adding a *_query_account API which might duplicate the work of an album query (perhaps with max-results query limit of 0 :) to give some comparable but different class like a "GDataPicasaWebAccount".  

YouTube, Documents, and Calendar don't seem to deal with having multiple types of Feed, so I can't go-with-the-flow like usual :)

(In reply to comment #6)
>  	/**
> +	 * GDataFeed:icon:
> +	 *
> +	 * The URI of an icon for the feed.
> +	 *
> +	 * API reference: <ulink type="http" url="http://code.google.com/apis/gdata/docs/2.0/reference.html">atom:icon</ulink>
> +	 **/

It needs a "Since" clause, and that URI is wrong. It should point to atomenabled.org.

Other than that, the first patch looks good.

(In reply to comment #7)
> Created an attachment (id=144698) [details]
> Patch adding support for gphoto:access as visibility to GDataPicasaWebFile
> 
> Now that I've written this, I'm beginning to wonder whether it was decided to
> not support visibility directly in GDataPicasaWebFile yet because PicasaWeb
> doesn't yet offer fine-grained access control over individual photos and
> videos, only their albums.

Correct. When PicasaWeb gains that ability, we can add visibility support to GDataPicasaWebFile.

> If we don't end up supporting <gphoto:access> on photo's themselves, is there
> some appropriate way to suppress the "Unhandled XML" messages?

Just add a block for it in parse_xml which does nothing, along with a suitable comment (and perhaps a reference to this bug report).

(In reply to comment #8)
> So, I'm inclined to add two new Feed classes, with names along the line of 
> from gdata_picasaweb_service_query_all_albums*:
> GDataPicasaWebAlbumsFeed
> from gdata_picasaweb_service_query_files*:
> GDataPicasaWebFilesFeed 
> 
> It's a bit unintuitive to think that your accounts information is in the
> *AlbumsFeed, but it would avoid adding a *_query_account API which might
> duplicate the work of an album query (perhaps with max-results query limit of 0
> :) to give some comparable but different class like a "GDataPicasaWebAccount".  

I'd be inclined to go with the GDataPicasaWebAccount route (though I'd call it GDataPicasaWebUser). It seems cleaner to me.

Created attachment 144996 [details] [review]
Patch adding support for atom:icon to GDataFeed with since and proper uri

This adds the Since: clauses to the get_icon() method and property installation comments.  

It also links to the way more appropriate http://www.atomenabled.org/developers/syndication/atom-format-spec.php#element.icon.

Created attachment 144997 [details] [review]
Patch capturing and suppressing gphoto:access on GDataPicasaWebFile

We now capture it and ignore it on the file, including a link to this bug.

So, in gdata_picasaweb_service_query_all_albums* and gdata_picasaweb_query_files, we pass our self, GDataPicasaWebService to gdata_service_query(). 

gdata_service_query() gets the class of the service (so, GDataPicasaWebServiceClass) and uses its 'feed_type' property to tell which type the feed is. 

For the feed of albums, I could set GDataPicasaWebServiceClass's feed_type (in gdata_picasaweb_service_class_init) to GDATA_TYPE_PICASAWEB_USER and then the feed will be parsed into GDataPicasaWebUser, a child of GDataFeed.  

However, this prevents me from separately parsing the feed for the files' feed, which had the two properties allowDownloads and allowPrints unique to it, and a bunch of properties that were redundant with the GDataPicasaWebAlbum if supplied, which I was hoping to capture and ignore them as per comment 5.  

Ideas:
* modify gdata_service_query to make feed_type its own parameter, bringing changes to all its clients but enabling more flexibility for different feeds for the same service
* decide that the two unique properties are probably not very relevant to people other than Google (they're not in the API reference other, though they also appear in the examples at http://code.google.com/apis/picasaweb/docs/2.0/developers_guide_protocol.html).  Then, we could ignore them and the properties redundant with GDataPicasaWebAlbum entries.  

Also, I'm wondering how a client should best obtain GDataPicasaWebUser.  In my current code, it's just like how you get a GDataPicasaWebAlbum from a GDataEntry (i.e. "GDataPicasaWebAlbum *album = GDATA_PICASAWEB_ALBUM (entry);", but "GDataPicasaWebUser *user = GDATA_PICASAWEB_USER (feed);"  I was wondering whether a gdata_picasaweb_service_get_user () would be more intuitive, though it would be slightly redundant, as it would need to also get a GDataFeed of albums.

commit 9b08a9ae6140ac7398ae4fd87910f555f95600a0
Author: Richard Schwarting <aquarichy@gmail.com>
Date:   Thu Oct 8 22:45:49 2009 +0100

    [picasaweb] Suppress gphoto:access on GDataPicasaWebFile
    
    Handle the gphoto:access element for files with a stub, such that a
    warning isn't printed. Proper support will be added when individual file
    permissions are supported by PicasaWeb. Helps: bgo#589858

 gdata/services/picasaweb/gdata-picasaweb-file.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

commit ceb151b4f8b80722516478a577297431eed056d9
Author: Richard Schwarting <aquarichy@gmail.com>
Date:   Thu Oct 8 22:43:42 2009 +0100

    [core] Add support for atom:icon to GDataFeed
    
    Add support for the atom:icon element to GDataFeed. Helps: bgo#589858

 docs/reference/gdata-sections.txt |    1 +
 gdata/gdata-feed.c                |   44 +++++++++++++++++++++++++++++++++++++
 gdata/gdata-feed.h                |    1 +
 gdata/gdata.symbols               |    1 +
 gdata/tests/picasaweb.c           |    1 +
 5 files changed, 48 insertions(+), 0 deletions(-)

(In reply to comment #12)
*snip*
> Ideas:
> * modify gdata_service_query to make feed_type its own parameter, bringing
> changes to all its clients but enabling more flexibility for different feeds
> for the same service
> * decide that the two unique properties are probably not very relevant to
> people other than Google (they're not in the API reference other, though they
> also appear in the examples at
> http://code.google.com/apis/picasaweb/docs/2.0/developers_guide_protocol.html).
>  Then, we could ignore them and the properties redundant with
> GDataPicasaWebAlbum entries.  

If they're not in the API reference, I'd consider that a reason for not implementing them. Just ignore them for now.

> Also, I'm wondering how a client should best obtain GDataPicasaWebUser.  In my
> current code, it's just like how you get a GDataPicasaWebAlbum from a
> GDataEntry (i.e. "GDataPicasaWebAlbum *album = GDATA_PICASAWEB_ALBUM (entry);",
> but "GDataPicasaWebUser *user = GDATA_PICASAWEB_USER (feed);"  I was wondering
> whether a gdata_picasaweb_service_get_user () would be more intuitive, though
> it would be slightly redundant, as it would need to also get a GDataFeed of
> albums.

I'd go with gdata_picasaweb_service_get_user().

Created attachment 145575 [details] [review]
Patch adding GDataPicasaWebUser class, handling various user feed elements

Hello.  

This adds GDataPicasaWebUser which subclasses GDataFeed and handles various elements particular to a user and not a single album.

It adds gdata_picasaweb_service_get_user () which lets you name a user or, if you're authenticated, NULL, and it will return the object for that user.  PicasaWeb returns 0 for some values which are only visible to the account holder (when authenticated as such), so currently I do that too with a note in the documentation alerting users to that.  

Album queries no longer automatically have ?access=all appended if PUBLIC or PRIVATE wasn't explicitly specified.  Querying albums and accounts with that parameter when unauthenticated results in a 403 Forbidden error.  Omitting it has the effect of all for authenticated users, and the effect of public for unauthenticated, just like if you visited it in the browser. 

Fields from the GDataFeed got when querying an album's files which are redundant with the album's properties are now ignored.  (I think it was all of said fields, without anything special in that feed.)

The two undocumented fields, allowDownloads and allowPrints, from querying a user's albums are ignored, too, as discussed above.

When I implemented some of the georss tags, I went to ignore the <gml:Envelope> node because it wasn't documented in PicasaWeb's API either, but you expressed the following preference: "We shouldn't ignore it; the 'Unhandled XML' messages for it should continue to be printed on the command line, so that we don't forget about implementing it in the future, when it's appropriate." and made commit 953d0c148e412b45bd7f1d0e8f36309f256d131f, setting the G_LOG_DOMAIN so command-line clients could ignore them themselves. 

Has your preference changed for ignoring these versus leaving them in?  The unhandled XML message for gml:Envelope is the last unhandled tag that gets reported now, and I'm don't think Google will necessarily introduce it into a future API.  

Cheers!

Comment on attachment 145575 [details] [review]
Patch adding GDataPicasaWebUser class, handling various user feed elements

I'm an idiot.  I can query for users as an entry.  I think my preference is to implement a GDataPicasaWebFeed to catch and ignore redundant elements (i.e. a query of albums have elements redundant with a user's GDataEntry) and have a GDataPicasaWebUser that gets produced from GDataEntry in a feed from a query on users.

Hmm.  Maybe this is actually sufficient, just using the GDataFeed of albums?  

If you'd prefer GDataPicasaWebUser reflected its GDataEntry (from http://picasaweb.google.com/data/*entry*/api/user/<userid>/) instead of the elements in the albums feed (from http://picasaweb.google.com/data/*feed*/api/user/<userid>/), I'll rework the patch and have have a GDataPicasaWebFeed separate from GDataPicasaWebUser to catch the redundant fields returned in the queries.

(In reply to comment #17)
> Hmm.  Maybe this is actually sufficient, just using the GDataFeed of albums?  
> 
> If you'd prefer GDataPicasaWebUser reflected its GDataEntry (from
> http://picasaweb.google.com/data/*entry*/api/user/<userid>/) instead of the
> elements in the albums feed (from
> http://picasaweb.google.com/data/*feed*/api/user/<userid>/), I'll rework the
> patch and have have a GDataPicasaWebFeed separate from GDataPicasaWebUser to
> catch the redundant fields returned in the queries.

That sounds better. I don't want to have the properties of a GDataPicasaWebUser depending on various elements in a <feed> which aren't well defined and could disappear. If it's based on an <entry> then at least the properties are defined in the API documentation.

(Sorry for the late reply: I've been busy with university work.)

Yah, I agree, I was just being lazy for a moment and wanting to work on other things :)  I feel stupid for missing this in the first place.

(Hurrah for school! :D  If you don't mind mail building up, I don't mind belated replies :)

Created attachment 145916 [details] [review]
Patch adding support for GDataPicasaWebUser and GDataPicasaFeed

Hi again.

GDataPicasaWebUser now is built from the relevant <entry>, and doesn't touch the feed. And we now have GDataPicasaWebFeed, which basically just grabs all elements in a feed that are redundant with separate entries and ignores them. 

This leaves only one message unhandled: 
libgdata-Message: Unhandled XML in GDataGeoRSSWhere: <gml:Envelope><gml:lowerCorner>45.2605939 12.1238603</gml:lowerCorner><gml:upperCorner>45.6080781 12.5537077</gml:upperCorner></gml:Envelope>

whose use isn't documented by Google, but which I imagine is used to frame the map's scale/zoom.

Review of attachment 145916 [details] [review]:

Looks good to me, though untested. I'll apply it and fix the issues below (they're just here for reference purposes). Thanks.

::: gdata/services/picasaweb/gdata-picasaweb-feed.c
@@ +41,3 @@
+
+struct _GDataPicasaWebFeedPrivate {
+	char nodata; 

Instead of doing this, you should just remove the entire struct.

@@ +72,3 @@
+{
+	/* Chain up to the parent class */
+	G_OBJECT_CLASS (gdata_picasaweb_feed_parent_class)->finalize (object);

If the finalize function just chains up to the parent, there's no need to include it.

@@ +80,3 @@
+	switch (property_id) {
+		default:
+			/* We don't have any other property... */

No properties? Don't need _get_property then.

@@ +88,3 @@
+parse_xml (GDataParsable *parsable, xmlDoc *doc, xmlNode *node, gpointer user_data, GError **error)
+{
+	if (xmlStrcmp (node->name, (xmlChar*) "user") == 0) {

Most of these if statements could be ORed together.

::: gdata/services/picasaweb/gdata-picasaweb-feed.h
@@ +48,3 @@
+typedef struct {
+	GDataFeed parent;
+	GDataPicasaWebFeedPrivate *priv;

This can just be replaced with some padding instead of creating a dummy private struct.

@@ +63,3 @@
+} GDataPicasaWebFeedClass;
+
+GType gdata_picasaweb_feed_get_type (void);

Should have the G_GNUC_CONST property.

::: gdata/services/picasaweb/gdata-picasaweb-service.c
@@ +62,3 @@
 /*
+ * This constructs the URI we want to access for querying albums or a
+ * user.  Type should be either entry (for just a user) or feed (for

"entry" or "feed".

@@ +132,3 @@
+	message = _gdata_service_query (GDATA_SERVICE (self), uri, NULL, cancellable, NULL, NULL, error);
+	if (message == NULL)
+		return NULL; /* RHSTODO: shouldn't we set an error?  (gdata_youtube_service_query_single_video doesn't) */

error should have been set by _gdata_service_query if it returns NULL.

::: gdata/services/picasaweb/gdata-picasaweb-user.c
@@ +46,3 @@
+	gint64 quota_limit;
+	gint64 quota_current;
+	gint max_photos_per_album;

Should these three be initialised to -1?

@@ +93,3 @@
+	 * GDataPicasaWebUser:nickname:
+	 *
+	 * The nickname of the user as specified by the user.

Not brilliantly worded. How about "The user's chosen nickname; how their name should be displayed."?

@@ +107,3 @@
+	 * GDataPicasaWebUser:quota-limit:
+	 *
+	 * The total amount of space in bytes available to the user. 

-1 means?

@@ +283,3 @@
+ * Gets the #GDataPicasaWebUser:user property.
+ *
+ * Return value: the feed's user, or %NULL

"the user's username"?

@@ +300,3 @@
+ * Gets the #GDataPicasaWebUser:nickname property.
+ *
+ * Return value: the nickname of the feed's user's nickname, or %NULL

"the user's nickname"?

@@ +317,3 @@
+ * Gets the #GDataPicasaWebUser:quota-limit property.  Note that
+ * this information is not available when accessing feeds which we
+ * haven't authenticated, and 0 is returned.

0 -> %0

Unless it should be %-1?

@@ +319,3 @@
+ * haven't authenticated, and 0 is returned.
+ *
+ * Return value: the maximum capacity in bytes for this feed's account or %-1

s/ or/, or/

@@ +338,3 @@
+ * haven't authenticated, and 0 is returned.
+ *
+ * Return value: the current number of bytes in used by this feed's account, or %-1

s/in used/in use/

::: gdata/services/picasaweb/gdata-picasaweb-user.h
@@ +63,3 @@
+} GDataPicasaWebUserClass;
+
+GType gdata_picasaweb_user_get_type (void);

G_GNUC_CONST

@@ +65,3 @@
+GType gdata_picasaweb_user_get_type (void);
+
+const gchar * gdata_picasaweb_user_get_user (GDataPicasaWebUser *self);

s/* /*/

::: gdata/tests/picasaweb.c
@@ +596,3 @@
+	GError *error;
+
+	error = NULL;

Could just initialise error to NULL when you declare it?

commit 68b774ab2134e311cd564139aa899d8fe8ecbb65
Author: Richard Schwarting <aquarichy@gmail.com>
Date:   Sun Oct 25 14:46:23 2009 +0000

    Bug 589858 — Handle gphoto XML elements found in 'GDataFeed'
    
    Adds a GDataPicasaWebUser, which exposes information about each
    registered user on PicasaWeb, queryable from the PicasaWeb service. The
    patch also suppresses gphoto XML elements on PicasaWeb feeds, since
    they duplicate data available in other, better places.
    Closes: bgo#589858

 docs/reference/gdata-sections.txt                  |   39 +++++++++++++
 gdata/gdata.h                                      |    1 +
 gdata/gdata.symbols                                |    9 +++
 gdata/services/picasaweb/Makefile.am               |    8 ++-
 gdata/services/picasaweb/gdata-picasaweb-query.c   |    2 +-
 gdata/services/picasaweb/gdata-picasaweb-service.c |   60 +++++++++++++++++--
 gdata/services/picasaweb/gdata-picasaweb-service.h |    3 +
 gdata/tests/picasaweb.c                            |   22 +++++++
 8 files changed, 134 insertions(+), 10 deletions(-)

I have a not-so-little checklist of things to keep in mind for libgdata, like initialising values on the same line and such.  Sorry that things get through :|  I also wasn't sure what to do with an empty object, and though the structure might be retained for future use (though I can't readily imagine why they'd have feed-only data.) 

I'm curious about the "Looks good to me, though untested." comment.  Was it causing you problems? 

Thanks.

(In reply to comment #23)
> I have a not-so-little checklist of things to keep in mind for libgdata, like
> initialising values on the same line and such.  Sorry that things get through

Don't worry about it; the code's good.

> I also wasn't sure what to do with an empty object, and though the
> structure might be retained for future use (though I can't readily imagine why
> they'd have feed-only data.) 

It's better to not have the object there and reserve a pointer for it, than to have it there wasting a lot more space unnecessarily. It can always be added if necessary.

> I'm curious about the "Looks good to me, though untested." comment.  Was it
> causing you problems? 

No, I just didn't test it more thoroughly than running the test case.