Created attachment 201183 [details] [review]
core: Add _full variants for functions that take callbacks

The _full variants take an extra GDestroyNotify parameter - due to the
fact that Grilo's callbacks have 'notified' scope (they may be called
asynchronously and multiple times) this is required by language
bindings.

Signed-off-by: Sam Thursfield <sam.thursfield@codethink.co.uk>

Created attachment 201184 [details] [review]
bindings/vala: Make callback parameters 'owned'

Callbacks in Vala were previously very broken: because all function
parameters default to being 'unowned', callback data would be freed as
soon as the function returned.

Signed-off-by: Sam Thursfield <sam.thursfield@codethink.co.uk>

Do you have a test/example code that can make use of it in vala? (bonus point if you also have that in python or js). Can be helpful as a basis to test your patches.

Created attachment 202143 [details] [review]
Test for _search() and _search_full()

Here is a test I wrote to test grl_media_source_search_full(). I test _search() as well since there was nothing to test it.

The current implementation does not pass this test because the notify function is called too early and with the wrong user_data when GRL_RESOLVE_IDLE_RELAY and/or GRL_RESOLVE_FULL is used.

Ideally, similar tests should be implemented for the other operations.

Suggestion to fix the issue: look at all the places where the callback might be called with an error and/or with remaining set to 0 (yeah, there are way too many places), and call notify at that point, _after_ the callback, ensuring the right user_data is used. Having the notify function and its user_data available at all these places might be tedious to implement though.

Created attachment 202385 [details] [review]
tests/media_source: Test browse() and get_media_from_uri()

Created attachment 202386 [details] [review]
core: Fix edge cases for destroy notify calls

I've added a couple more tests, I'm not immediately clear how to test the others so I'll wait and see if you have any pointers.

The tests now all pass, I think that I've now successfully navigated this twisty little maze of callbacks ... :)

I hope I'm not thinking that too late, but I thought of an alternative way of implementing that, that would be simpler, less intrusive, and allow more easily to change the way that mess of callbacks is organised in the future.

In a nutshell, I think the _full version of methods can be implemented as wrappers around the original methods.

For instance, here is a pseudo-code for what search_full would look like:

struct callback_data { callback; notify; user_data};

search_full(<some_args>, callback, user_data, notify)
{
 struct callback_data data = {callback, notify, user_data};
 search(<some_args>, magic_callback, /* passed as user_data */data);
}

magic_callback(<args>, remaining, data, error)
{
  data->callback (<args>, remaining, error);
  if (error != NULL || remaining == 0) {
    data->notify (data->user_data);
  }
}

This takes advantage of the fact that the callback is _always_ called with an error or remaining == 0 the last time it is called, and it is never called again if called in that way. That way, we now that when this happens, we want to call the notify() method after calling the callback.
The big advantage of that idea is that it separates cleanly the logic of the notify stuff from the rest.

Created attachment 203272 [details] [review]
core: Add full version of media source operations (with notify)

Here is the implementation of what I suggested

Created attachment 203273 [details] [review]
tests: add tests for some media source operations using the fs plugin.

Here are the tests that go with the other patch. This might need some work to be merged with the tests by Sam that seem to add more stuff (forgot to look at them before writing these additional tests, god am I absent minded).

Created attachment 203276 [details] [review]
core: set to async the scope of callbacks that are called only once

Finally, for the operations that call their callback only once (i.e. everything but browse, search and query), I believe that the real solution is to use (scope async).

Using (scope async) won't fix the Vala bindings. Vala only understands 'unowned' and 'owned' delegates. If a C function doesn't take a destroy notify, Vala will treat it as an unowned delegate and will therefore free the user data straight after the function call, without waiting for the callback to run. It wouldn't be trivial to fix this.

I made a new set patches myself that add a _full variant to every function, I'll upload them later today

Created attachment 203409 [details] [review]
core: Add _full variants for all functions with callbacks

Language bindings need to explicitly pass destructors for asynchronous
callbacks. This is now possible using this extra API.

Signed-off-by: Sam Thursfield <sam.thursfield@codethink.co.uk>

I see three different issues here:
 A. _search(), _query() and _browse() should have variants with a GDestroyNotify so that bindings can know easily when to destroy the data associated with the callback
 B. the async methods that call their callback only once and are marked as (scope notify) should be marked as (scope async), because that is how they work
 C. vala does not support (scope async)

Clearly, (A) and (B) are bugs in Grilo that should be fixed in Grilo. (A) should be fixed by adding "_full" wrappers around the original method calls  (at least for 0.1.x) and (B) should be fixed by changing the annotations for these methods.

On the other hand, (C) is a bug[1] in vala, and I believe that it makes more sense to fix it once and for all in vala than to put workarounds in all the libraries that use (scope async).

If really you cannot fix the bug in vala nor wait for it to be fixed, I think that the work around should rather be on the side of the application.
I don't know vala much, but I don't feel that implementing (scope async) would be very complicated anyway, and that's what [2] seems to suggest.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=656552
[2] https://bugzilla.gnome.org/show_bug.cgi?id=656552#c2

I've commented on https://bugzilla.gnome.org/show_bug.cgi?id=656552 - I guess we should see what the Vala developers say to this.

I'm closing this bug because 0.1 branch is mostly deprecated, and all the work is happening in 0.2

So it is not worth to fix this.