Created attachment 341618 [details] [review]
proposed patch ][

The same as the one before, only added the new symbols into gdata/gdata-core.symbols (I had it added in that built gdata/gdata.symbols instead, but I didn't notice it's not part of the patch).

(Thanks for the patches: I do intend to review them as soon as I can. Sorry for the wait.)

Review of attachment 341618 [details] [review]:

This looks like a good start. The code in GDataTasksQuery can be made more generic and moved into GDataQuery, and you could then integrate it into gdata-service.c so that gdata_query_next() and gdata_query_previous() work automatically — grep for _gdata_query_set_next_uri().

Some unit tests would be useful too. They could be added to gdata/tests/general.c. Thanks!

::: gdata/gdata-core.symbols
@@ +46,3 @@
 gdata_feed_get_start_index
 gdata_feed_get_total_results
+gdata_feed_get_next_page_token

These new symbols should also be listed in docs/reference/gdata-sections.txt.

::: gdata/gdata-feed.c
@@ +95,3 @@
 	PROP_TOTAL_RESULTS,
+	PROP_RIGHTS,
+	PROP_NEXT_PAGE_TOKEN

Nitpick: Please add a trailing comma to the new line to reduce diff noise next time.

@@ +303,3 @@
+	 * GDataFeed:next-page-token:
+	 *
+	 * The next page token for GTask feeds. Pass this to gdata_tasks_query_set_page_token()

Please don’t explicitly refer to GTask here, since GDataFeed is a generic class. Thankfully, the nextPageToken parameter is used by several APIs (at least Tasks, Calendar, Drive), so it can be handled generically here.

@@ +307,3 @@
+	 *
+	 * Since: 0.17.7
+	 **/

Nitpick: ‘*/’ rather than ‘**/’ please, since that’s what gtk-doc prefers.

(I need to convert the rest of the comment endings similarly.)

@@ +1098,3 @@
+ * @self: a #GDataFeed
+ *
+ * Returns the next page token for a GTask query result, or %NULL if not set.

Again, please don’t explicitly refer to GTask here. Refer back to GDataFeed:next-page-token so that there’s a canonical place for next page tokens to be documented (in the property documentation).

@@ +1100,3 @@
+ * Returns the next page token for a GTask query result, or %NULL if not set.
+ *
+ * Return value: the next page token

Needs `(nullable)`.

@@ +1103,3 @@
+ *
+ * Since: 0.17.7
+ **/

Please use ‘*/’ rather than ‘**/’.

::: gdata/services/tasks/gdata-tasks-query.c
@@ +66,3 @@
 	PROP_SHOW_DELETED,
 	PROP_SHOW_HIDDEN,
+	PROP_PAGE_TOKEN

Nitpick: Missing a trailing comma.

@@ +186,3 @@
+	 * Since: 0.17.7
+	 */
+	g_object_class_install_property (gobject_class, PROP_PAGE_TOKEN,

Since page tokens are common to several APIs (at least Tasks, Calendar, YouTube and Drive), and all use the `pageToken` and `nextPageToken` parameters, please move all this code to GDataQuery, rather than GDataTasksQuery. Then it can be shared.

@@ +301,3 @@
 	}
 
+	if (priv->page_token && *priv->page_token) {

The coding convention in libgdata is a bit more explicit than this. Please use:
`if (priv->page_token != NULL && *priv->page_token != '\0')`.

@@ +715,3 @@
+	g_return_if_fail (GDATA_IS_TASKS_QUERY (self));
+
+	if (page_token && !*page_token)

`if (page_token != NULL && *page_token == '\0')`

(In reply to Philip Withnall from comment #3)
> This looks like a good start. The code in GDataTasksQuery can be made more
> generic and moved into GDataQuery, and you could then integrate it into
> gdata-service.c so that gdata_query_next() and gdata_query_previous() work
> automatically — grep for _gdata_query_set_next_uri().

I'm unsure about this. The "move to GDataQuery" makes sense, but I do not know what to do with the gdata_query_previous(). The server returns only the nextPageToken, I wouldn't be surprised when it's invalidated when used/the next call, and even if not, then to be able to use the gdata_query_previous() there should be made some array of the already used page tokens? That feels awkward.

> Some unit tests would be useful too. They could be added to
> gdata/tests/general.c. Thanks!

Eeks... I'm not familiar with the libgdata itself, I'm just a humble sporadic user of it. I'm afraid I cannot do all the changes, without spending too much time learning libgdata internals deeply.

I can surely fix the coding style issues, that's pretty simple to do.

> `if (page_token != NULL && *page_token == '\0')`

I see. I do not like it in general if-s, due to possible performance issues, but I use it similarly in the g_return_if_fail() family functions for better readability. None of it is critical here. It's just a different project has different habits. That's quite common.

Just a note, from bug #777573, completed tasks do not seem to be deleted from the server, thus even the user has there 7 active tasks, eds is not able to receive them due to missing pagination.

I'm reopening the bug #775699, because of the undergo review and basically a request to make it completely differently, which the change in bug #775699 counted with the proposed patch (honestly, I didn't expect any problem with it, that's why I committed to the eds first).

Sorry for being slow; I haven’t yet found the time to review and update this properly. Don’t make big changes in EDS for the sake of this not being merged: I’ll try and work on it tonight.

Created attachment 344193 [details] [review]
core: Support pagination using page tokens from JSON

WIP

Based on a patch by Milan Crha <mcrha@redhat.com>.

Here’s a WIP patch for what I have in mind. It passes the current unit tests in libgdata, but still needs to have some new tests written, and still needs to actually be tested against the live GTasks service. The documentation and commit message also need to be done.

I will try to continue work on this tomorrow. I’m basically posting the patch here as a progress report — don’t bother testing EDS against it unless you’re especially keen.

I managed to get a bit of manual testing done tonight, and it looks like this patch does the trick. However, I have not managed to write some unit tests for it yet, and I don’t want to commit it without those.

Milan, if you get some time to test EDS against this patch, that would be very helpful; otherwise please hold on a few more days while I find some time to write some unit tests.

(In reply to Philip Withnall from comment #9)
> Milan, if you get some time to test EDS against this patch, that would be
> very helpful;

There is no gdata_query_set_page_token(), referenced in the patch, thus either the comment referencing it is wrong, or the function is missing, or I miss something. It's fairly easy to test your change for me, I've the environment set up.

> otherwise please hold on a few more days while I find some time to write
> some unit tests.

Thanks, I appreciate it. You know, it would be ideal to get this out in a stable release too, which would be possible, from my point of view, because you are adding new API, not changing nor removing any existing.

Review of attachment 344193 [details] [review]:

::: gdata/gdata-feed.c
@@ +306,3 @@
+	 * gdata_query_set_page_token() to advance to the next page when
+	 * querying APIs which use page tokens rather than page numbers or
+	 * offsets.

This documentation is completely out of date and needs updating.

(In reply to Milan Crha from comment #10)
> (In reply to Philip Withnall from comment #9)
> > Milan, if you get some time to test EDS against this patch, that would be
> > very helpful;
> 
> There is no gdata_query_set_page_token(), referenced in the patch, thus
> either the comment referencing it is wrong, or the function is missing, or I
> miss something. It's fairly easy to test your change for me, I've the
> environment set up.

Oops, I need to update that documentation.

You don’t need to call gdata_query_set_next_page_token(); that’s taken care of by the #GDataService when it returns the results to you. Just create a GDataQuery, set its query parameters (if you want), pass it to gdata_tasks_service_*() when you do your query, then call gdata_query_next_page() on it before you call gdata_tasks_service_*() again to do the next query.

See what this demo does for an example: https://git.gnome.org/browse/libgdata/tree/demos/docs-list/docs-list.c#n101

> > otherwise please hold on a few more days while I find some time to write
> > some unit tests.
> 
> Thanks, I appreciate it. You know, it would be ideal to get this out in a
> stable release too, which would be possible, from my point of view, because
> you are adding new API, not changing nor removing any existing.

Yeah, will do.

I see. It's a bad API, I would like to stop before asking the server. It's cleaner approach. Nonetheless, the patch fails to increment on the page, it keeps receiving the first page data. My log shows more than 120 tries, then I'm kicked off from the server with:

>  (evolution-calendar-factory-subprocess:14282): libgdata-WARNING **: Unknown
>  error code ‘quotaExceeded’ in domain ‘usageLimits’ received with location
>  type ‘(null)’, location ‘(null)’, extended help ‘(null)’ and message
>  ‘Quota Exceeded’.

The URL it failed with shows:

> (evolution-calendar-factory-subprocess:14282): libgdata-DEBUG: > GET
> /tasks/v1/lists/AAAAAAA/tasks?maxResults=100&
> showCompleted=true&showDeleted=false&showHidden=true HTTP/1.1

and it doesn't change between the calls. I do call

   gdata_query_next_page (GDATA_QUERY (tasks_query));

before invoking gdata_tasks_service_query_tasks() here:

https://git.gnome.org/browse/evolution-data-server/tree/src/calendar/backends/gtasks/e-cal-backend-gtasks.c#n540

The code currently looks like this there:

   #ifdef HAVE_LIBGDATA_TASKS_PAGINATION_FUNCTIONS
      gdata_query_next_page (GDATA_QUERY (tasks_query));

      g_clear_object (&feed);
      feed = gdata_tasks_service_query_tasks (gtasks->priv->service,
          gtasks->priv->tasklist, GDATA_QUERY (tasks_query), cancellable,
          NULL, NULL, &local_error);
   #endif

(In reply to Milan Crha from comment #13)
> I see. It's a bad API, I would like to stop before asking the server. It's
> cleaner approach.

Not sure what you mean by that. Could you expand on it please?

> The URL it failed with shows:
> 
> > (evolution-calendar-factory-subprocess:14282): libgdata-DEBUG: > GET
> > /tasks/v1/lists/AAAAAAA/tasks?maxResults=100&
> > showCompleted=true&showDeleted=false&showHidden=true HTTP/1.1

Hmm. Could you give a copy of the JSON for one of the returned feeds please? I wonder if the nextPageToken parsing is working.

(In reply to Philip Withnall from comment #14)
> Not sure what you mean by that. Could you expand on it please?

As you have it currently done I call gdata_query_next_page() and then blindly follow with gdata_tasks_service_query_tasks() even if there was no nextPageToken provided by the server, thus I either repeat in a never-ending cycle (which happened to me now) or the request to the server is useless. A better would be to have gdata_query_next_page() return TRUE when there actually was any move forward to the next page, thus I could break the cycle without re-assuring with the server.

I'm also a bit afraid of the generality of this request. Like the GTaskQuery doesn't use indexed pagination, but the default for this gdata_query_next_page() is to use the indexed query, thus without having called _gdata_query_set_next_page_token() the struct would have set incorrect pagination type. There would be the GTask query to set the type in its construction and the parent struct should claim an issue if a different kind of pagination would be requested.

> I wonder if the nextPageToken parsing is working.

The parsing did work in my initial patch, that's how I tested it. Here's a cut and censored version of the server response:

> {
>  "kind": "tasks#tasks",
>  "etag": "\"xxxxxx\"",
>  "nextPageToken": "xxxxxx",
>  "items": [
>   {
>    "kind": "tasks#task",
>    "id": "xxxxxx",
>    "etag": "\"xxxxxx\"",
>    "title": "230",
>    "updated": "2016-12-08T11:29:06.000Z",
>    "selfLink": "https://www.googleapis.com/tasks/v1/lists/xxxxx/tasks/xxxxxx",
>    "position": "00000000000017822061",
>    "status": "needsAction"
>   },
> ......
>   }
>  ]
> }

(In reply to Milan Crha from comment #15)
> (In reply to Philip Withnall from comment #14)
> > Not sure what you mean by that. Could you expand on it please?
> 
> As you have it currently done I call gdata_query_next_page() and then
> blindly follow with gdata_tasks_service_query_tasks() even if there was no
> nextPageToken provided by the server, thus I either repeat in a never-ending
> cycle (which happened to me now) or the request to the server is useless. A
> better would be to have gdata_query_next_page() return TRUE when there
> actually was any move forward to the next page, thus I could break the cycle
> without re-assuring with the server.

That makes sense; thanks.

> I'm also a bit afraid of the generality of this request. Like the GTaskQuery
> doesn't use indexed pagination, but the default for this
> gdata_query_next_page() is to use the indexed query, thus without having
> called _gdata_query_set_next_page_token() the struct would have set
> incorrect pagination type. There would be the GTask query to set the type in
> its construction and the parent struct should claim an issue if a different
> kind of pagination would be requested.

That also makes sense.

I’m probably not going to get any time before the end of the weekend to take another look at this; I’ll take another look on Monday, sorry.

(In reply to Philip Withnall from comment #16)
> I’m probably not going to get any time before the end of the weekend to take
> another look at this; I’ll take another look on Monday, sorry.

That's okay, no need to apologize, I'm happy you are looking on this.

ping ;)

My current progress is here: https://gitlab.com/pwithnall/libgdata/tree/775813-pagination

I think it works, but I need to finish testing and tidying things up. If you could test it, that would be helpful.

Okay, it's better (at commit_9b5c83b0f5ab4fa17df2), as it moves between pages, but it doesn't stop at the last page, it uses the last known token instead.

From the log:

> {
>  "kind": "tasks#tasks",
>  "etag": "\"NEVtLf5Q_dTURZbE3E-zlPpPgGk/LTIwNTUyMjgwNDA\"",
>  "nextPageToken": "MDMxNzg0NTk3NjY0ODc0NTI3MjA6MDoxMDUwMTE2Nzk1",
>  "items": [ ......

> GET /tasks/v1/lists/XXXXX/tasks?max-results=100&pageToken=MDMxNzg0NTk3NjY0ODc0NTI3MjA6MDoxMDUwMTE2Nzk1&maxResults=100&showCompleted=true&showDeleted=false&showHidden=true HTTP/1.1
> Soup-Debug-Timestamp: 1486576549
> Soup-Debug: SoupSession 1 (0xdc39b0), SoupMessage 5 (0x7f9c441650a0), SoupSocket 1 (0x7f9c30003f30)
> ....
> {
>  "kind": "tasks#tasks",
>  "etag": "\"NEVtLf5Q_dTURZbE3E-zlPpPgGk/LTIwNTUyMjgwNDA\"",
>  "items": [ ......

> GET /tasks/v1/lists/XXXXXX/tasks?max-results=100&pageToken=MDMxNzg0NTk3NjY0ODc0NTI3MjA6MDoxMDUwMTE2Nzk1&maxResults=100&showCompleted=true&showDeleted=false&showHidden=true HTTP/1.1
> Soup-Debug-Timestamp: 1486576549
> Soup-Debug: SoupSession 1 (0xdc39b0), SoupMessage 6 (0x7f9c2800b680), SoupSocket 1 (0x7f9c30003f30)
> .....
> {
>  "kind": "tasks#tasks",
>  "etag": "\"NEVtLf5Q_dTURZbE3E-zlPpPgGk/LTIwNTUyMjgwNDA\"",
>  "items": [

and it repeats it ad infinity/until kicked off by the server.

That's with the changes from comment #13 on the eds side.

(In reply to Milan Crha from comment #20)
> Okay, it's better (at commit_9b5c83b0f5ab4fa17df2), as it moves between
> pages, but it doesn't stop at the last page, it uses the last known token
> instead.
> 
> …
>
> That's with the changes from comment #13 on the eds side.

Are you checking to see if the number of results from gdata_feed_get_entries is 0? That indicates the last page. I know the API is not great, but I can’t fix it without breaking backwards compatibility, which I don’t want to do.

See https://git.gnome.org/browse/libgdata/tree/demos/docs-list/docs-list.c#n89.

No, I do not. I added a debug print there and it shows this:
>   ecb_gtasks_update_thread: read entries:100 max expected:100 feed's nextPageToken:AAA
>   ecb_gtasks_update_thread: read entries:100 max expected:100 feed's nextPageToken:BBB
>   ecb_gtasks_update_thread: read entries:38 max expected:100 feed's nextPageToken:(null)
>   ecb_gtasks_update_thread: read entries:38 max expected:100 feed's nextPageToken:(null)
>   ...
>   ecb_gtasks_update_thread: read entries:38 max expected:100 feed's nextPageToken:(null)

There is no 0 involved. I'm also reusing the query object, the same as the code in the demos you linked to above.

I guess the gdata-service.c should do something else in case the gdata_feed_get_next_page_token() returns NULL, but it's tricky, to not start from the first page again.

I can use the call to gdata_feed_get_next_page_token() to stop my cycle, it's better than asking the server whether everything is fine, which fully covers my first concern in comment #15.

Created attachment 346211 [details] [review]
core: Support pagination using page tokens from JSON

Based on a patch by Milan Crha <mcrha@redhat.com>.

This reworks how pagination is implemented so that multiple pagination
mechanisms are supported explicitly, making the code a lot clearer. A
lot of the new services use pageToken parameters, which we did not
previously support — so this fixes support for pagination in the Google
Tasks service, for example.

This also means that we can drop the hacky pagination support from
GDataDocumentsService.

Created attachment 346212 [details] [review]
tasks: Fix pagination in Google Tasks API

The pageToken URI parameter was never being appended to queries, so they
could never retrieve more than the first page of results.

Created attachment 346213 [details] [review]
demos: Add a demo of Google Tasks

This is a simple read-only demo of the Tasks service, similar to the
Calendar demo.

Review of attachment 346213 [details] [review]:

Just a little thing I noticed here, not a real review of the change by any means.

::: demos/tasks/tasks-cli.c
@@ +291,3 @@
+			retval = 0;
+			g_object_unref (feed);
+			goto done;

call 'break' here, thus the 'g_print()' after the 'while' is actually used

I tested these patches with evolution-data-server and they work fine, applied on top of libgdata commit d9c1e590ac36.

(In reply to Milan Crha from comment #26)
> Review of attachment 346213 [details] [review] [review]:
> 
> Just a little thing I noticed here, not a real review of the change by any
> means.
> 
> ::: demos/tasks/tasks-cli.c
> @@ +291,3 @@
> +			retval = 0;
> +			g_object_unref (feed);
> +			goto done;
> 
> call 'break' here, thus the 'g_print()' after the 'while' is actually used

Whoops, yes. Thanks for the mini-review. If you fancy doing a more thorough review, that would also be welcome. :-)

I’ll try and get this lot merged and a release made tonight, although that might be blocked by some other bugs I noticed while fixing this.

Review of attachment 346211 [details] [review]:

Just one thing noticed (read-review), which may or may not be a problem.

::: gdata/gdata-query.c
@@ +1125,1 @@
+	_gdata_query_set_pagination_type (self, GDATA_QUERY_PAGINATION_URIS);

I see an inconsistency here, the set_next_token(), neither set_next_uri() do not change the pagination type, it rather checks that the pagination_type matches expected type.

(In reply to Philip Withnall from comment #28)
> Whoops, yes. Thanks for the mini-review. If you fancy doing a more thorough
> review, that would also be welcome. :-)

I did a little read-review, as you can see above. Nothing else stroke to me.

I’m terrible at keeping my promises with this bug, sorry. I will try to do the other fixes and a release on Sunday.

Attachment 346211 [details] pushed as 38b934a - core: Support pagination using page tokens from JSON
Attachment 346212 [details] pushed as a6b9808 - tasks: Fix pagination in Google Tasks API
Attachment 346213 [details] pushed as fd1451a - demos: Add a demo of Google Tasks

libgdata 0.17.7 released, finally. Sorry for the delay.