To be clear, what I don't like is the source developer having to do things like:
local userdata = {callback = callback, skip = skip, count = count}
[...]
grl.fetch(url, fetch_results_cb, userdata)

The developers shouldn't have to keep track of the user data (or rather, metadata pertaining to the current operation) themselves.

(In reply to Victor Toso from comment #0)
> This is pretty much what is being done with grl_l_callback in order to call
> grl_util_operation_spec_gc
> 
> Let's see how it goes.

This idea was naive. I was thinking about having a closure accesible during the 
operation, that could store as upvalue and return the OperationSpec. The main
issue is with multiple ongoing operations which would need to bound the closure
to the operation-id and seems messy.

The current approach is similar to what we had before, storing the OperationSpec
in the stack. I think it makes more sense to have it in the grl library instead
of _G. I also thought that it could be benefitial to have it as read-only from
Lua side, so I have created a proxy for it. 

I realized a bit late that the whole code related to the grl table and its 
metatable and metamethods could be done in an external lua file and loaded by
lua-library code. Dealing with stack operations in C is not that much of fun.
But it works for now IMHO ;)

Make test works, I did some browse with grilo-test-ui-0.3 and had some bug fixing. Let me know what you think :)

PS: I guess we can squash all revert lua-source api changes?

Created attachment 323443 [details] [review]
lua-factory: use wrapper for lua_pcall

Due improvements later in the patch series, it becames useful to have
such wrapper. We always call the lua_gc after lua_pcall as current logic
flow expects it.

Created attachment 323444 [details] [review]
lua-factory: Make grl and grl.lua read-only

By creating a proxy table with custom metatable in order to avoid
changes in grl and grl.lua by Lua sources.

The proxy redirects read access (__index metamethod) to the original
table but the write access (__newindex metamethod) triggers an error.

We still can do read-write but only internally; For this, we get a
reference to the original table using the __call metamethod. The handler
expects an userdata argument which makes nontrivial to retrieve it from
Lua sources.

Created attachment 323445 [details] [review]
lua-factory: introduce source state table

The major point is being able to track and retrieve ongoing Grilo
operations data under the same Lua Source.

Before d809be39ed2bbd4c410be we were using the global env in lua (_G) in
order to store the operation and retrieve it but the design of the code
was lacking improvements.
e.g. grl.fetch() and grl.unzip() were bounded to OperationSpec

After d809be39ed2bbd4c410be we removed the the OperationSpec opaque from
othe global context and utilities functions but the API changed for
that.

From now on, we will be able to move back to the former API by storing
again the operation details but without losing the benefits from
d809be39ed2bbd4c410be.

The specifics from each operation will now be stored into our
lua-library table: grl
* grl.__priv_state.current_operation hold the operation being executed;
* grl.__priv_state.operations hole a table of all ongoing operations;

The operation is defined by a state table which has three fields at the
moment:
* op_id: the unique operation id given by Grilo
* data: an userdata which points to our opaque (OperationSpec)
* state: holds a string related to the Operation state
  - running: The current ongoing operation
  - waiting: An async call is being performed
  - finalized: Callback was called and this will soon be removed by
    garbage collector

Created attachment 323446 [details] [review]
lua-factory: set state of all operations

This commit uses the functions introduced in previous commit in order to
track the state of each operation and save its state

Created attachment 323447 [details] [review]
Revert "lua-factory: change grl.callback(), grl.fetch() and grl.unzip()"

This reverts commit d809be39ed2bbd4c410be7aa527b36902362511a.

This breaks the current plugins by changing the callback passed as
argument back to grl.callback()

This is not 100% revert. We are keeping the changes to grl.fetch and
grl.unzip.

Created attachment 323448 [details] [review]
Revert "lua-factory: change grl.get_media_keys() into the static table"

This reverts commit 3e409b7727f803e2be4427c7544c679373a7d926.

This breaks the current plugins by changing the static table passed as
argument back grl.get_media_keys()

This is not 100% revert. We are keeping some fixes.

Created attachment 323449 [details] [review]
Revert "lua-factory: change grl.get_options() into the static table"

This reverts commit 2bfcff90d589d4335105a6423616c7de61cf4c71.

Created attachment 323450 [details] [review]
Revert "tests: port lua-factory fake sources to new API"

This reverts commit d322e89e6e78ac7810b7eaf314421659b77d5ca7.

But keeps grl.fetch callback as function instead of string

Created attachment 323451 [details] [review]
Revert "lua-factory: port grl-guardianvideos.lua to the new lua system"

This reverts commit ccfa70cfe911129c8bdb936fda99dd11cc60bff4.

But keeps grl.fetch callback as function instead of string

Created attachment 323452 [details] [review]
Revert "lua-factory: port grl-euronews.lua to the new lua system"

This reverts commit 592bb1c7a5315e4f2582dad415e03e42f5a43cad.

But keeps grl.fetch callback as function instead of string

Created attachment 323453 [details] [review]
Revert "lua-factory: port grl-appletrailers.lua to the new lua system"

This reverts commit bea8e082da513d90660de84f9213a4fd9552d2eb.

But keeps grl.fetch callback as function instead of string

Created attachment 323454 [details] [review]
Revert "lua-factory: port grl-metrolyrics.lua to the new lua system"

This reverts commit 0e71278f1616a55e4e0a3b719abbe978a4ad71af.

But keeps grl.fetch callback as function instead of string

Created attachment 323455 [details] [review]
Revert "lua-factory: port grl-video-title-parsing.lua to the new lua system"

This reverts commit 46b127a9ccee3a01f95bee47e81305238ab17f3a.

Created attachment 323456 [details] [review]
Revert "lua-factory: port grl-pocket.lua to the new lua system"

This reverts commit 368693b7a8599941d5bfe3b73f440e16ff115a24.

But keeps grl.fetch callback as function instead of string

Created attachment 323457 [details] [review]
Revert "lua-factory: port grl-spotify-cover.lua to the new lua system"

This reverts commit 9b329f3e5246477f039a1fb2b129706370237c89.

But keeps grl.fetch callback as function instead of string

Created attachment 323458 [details] [review]
Revert "lua-factory: port grl-radiofrance.lua to the new lua system"

This reverts commit 93547ac94797cdea6670b0d458e838bcdb578f77.

But keeps grl.fetch callback as function instead of string

Created attachment 323459 [details] [review]
Revert "lua-factory: port grl-lastfm-cover.lua to the new lua system"

This reverts commit fbb244ee962ddcf483dc4c6adec30de6c2616436.

But keeps grl.fetch callback as function instead of string

Review of attachment 323443 [details] [review]:

::: src/lua-factory/grl-lua-library.c
@@ +1924,3 @@
+ * order.
+ */
+void

You should return a boolean here, with FALSE on error.

In every grl_lua_library_pcall() call, make sure to check the retval rather than whether error is set.

@@ +1931,3 @@
+{
+  if (lua_pcall (L, nargs, 0, 0)) {
+    gint error_code = (os) ? os->error_code : GRL_CORE_ERROR_OPERATION_CANCELLED;

GRL_CORE_ERROR_OPERATION_CANCELLED or G_IO_ERROR_CANCELLED?

Review of attachment 323445 [details] [review]:

::: src/lua-factory/grl-lua-library.c
@@ +64,3 @@
 } UnzipOperation;
 
+static const gchar *source_op_state_str[] = {

[NUM_STATES]?

(In reply to Victor Toso from comment #2)
> (In reply to Victor Toso from comment #0)
> > This is pretty much what is being done with grl_l_callback in order to call
> > grl_util_operation_spec_gc
> > 
> > Let's see how it goes.
> 
> This idea was naive. I was thinking about having a closure accesible during
> the 
> operation, that could store as upvalue and return the OperationSpec. The main
> issue is with multiple ongoing operations which would need to bound the
> closure
> to the operation-id and seems messy.
> 
> The current approach is similar to what we had before, storing the
> OperationSpec
> in the stack. I think it makes more sense to have it in the grl library
> instead
> of _G. I also thought that it could be benefitial to have it as read-only
> from
> Lua side, so I have created a proxy for it. 
> 
> I realized a bit late that the whole code related to the grl table and its 
> metatable and metamethods could be done in an external lua file and loaded by
> lua-library code. Dealing with stack operations in C is not that much of fun.
> But it works for now IMHO ;)
> 
> Make test works, I did some browse with grilo-test-ui-0.3 and had some bug
> fixing. Let me know what you think :)

I'm not really in a position to review all those Lua factory patches I'm afraid.

I'd just want to make sure that we still get warnings (as in 0.2.x) when a source fails to return a result, or returns a result twice, or has a syntax error.

> PS: I guess we can squash all revert lua-source api changes?

Separately would be better, and as you've already tweaked them.

(In reply to Bastien Nocera from comment #20)
> Review of attachment 323443 [details] [review] [review]:
> 
> ::: src/lua-factory/grl-lua-library.c
> @@ +1924,3 @@
> + * order.
> + */
> +void
> 
> You should return a boolean here, with FALSE on error.
> 
> In every grl_lua_library_pcall() call, make sure to check the retval rather
> than whether error is set.

Sure, I'll fix it

> 
> @@ +1931,3 @@
> +{
> +  if (lua_pcall (L, nargs, 0, 0)) {
> +    gint error_code = (os) ? os->error_code :
> GRL_CORE_ERROR_OPERATION_CANCELLED;
> 
> GRL_CORE_ERROR_OPERATION_CANCELLED or G_IO_ERROR_CANCELLED?

To be honest, this was a bit hacky in order to not fail at this specific commit. All grl_lua_library_pcall after patch "lua-factory: set state of all operations" should have the OperationSpec and the proper error_code related to the operation.

I'll remove this in the later patch and include an assert_nonnull (os) instead

(In reply to Bastien Nocera from comment #22)
> I'm not really in a position to review all those Lua factory patches I'm
> afraid.
> 
> I'd just want to make sure that we still get warnings (as in 0.2.x) when a
> source fails to return a result, or returns a result twice, or has a syntax
> error.

We are covered in the two last cases but I think we don't warn anymore if neither grl.callback() or some async function were called.

> 
> > PS: I guess we can squash all revert lua-source api changes?
> 
> Separately would be better, and as you've already tweaked them.

Okay!

Thanks for the review. I'll address this issues later today.

changes in v2
- grl_lua_library_pcall() is now a boolean which returns FALSE on error
- changed error_code when OperationSpec is NULL to G_IO_ERROR_CANCELLED
- enum2str array initialized with NUM_STATES value
- reused grl_lua_library_push_grl_callback to make lua-library recognize when when lua source does not call to grl.callback (or any async like grl.fetch)
- some fixes related to the rebase

wip (not attached)
- in order to have the warning when neither grl.callback or any async function like grl.fetch is called, we can push a userdata with __gc set that will check the state of source as soon as lua_gc is called

Created attachment 323669 [details] [review]
lua-factory: use wrapper for lua_pcall

Due improvements later in the patch series, it becames useful to have
such wrapper. We always call the lua_gc after lua_pcall as current logic
flow expects it.

Created attachment 323670 [details] [review]
lua-factory: Make grl and grl.lua read-only

By creating a proxy table with custom metatable in order to avoid
changes in grl and grl.lua by Lua sources.

The proxy redirects read access (__index metamethod) to the original
table but the write access (__newindex metamethod) triggers an error.

We still can do read-write but only internally; For this, we get a
reference to the original table using the __call metamethod. The handler
expects an userdata argument which makes nontrivial to retrieve it from
Lua sources.

Created attachment 323671 [details] [review]
lua-factory: introduce source state table

The major point is being able to track and retrieve ongoing Grilo
operations data under the same Lua Source.

Before d809be39ed2bbd4c410be we were using the global env in lua (_G) in
order to store the operation and retrieve it but the design of the code
was lacking improvements.
e.g. grl.fetch() and grl.unzip() were bounded to OperationSpec

After d809be39ed2bbd4c410be we removed the the OperationSpec opaque from
othe global context and utilities functions but the API changed for
that.

From now on, we will be able to move back to the former API by storing
again the operation details but without losing the benefits from
d809be39ed2bbd4c410be.

The specifics from each operation will now be stored into our
lua-library table: grl
* grl.__priv_state.current_operation hold the operation being executed;
* grl.__priv_state.operations hole a table of all ongoing operations;

The operation is defined by a state table which has three fields at the
moment:
* op_id: the unique operation id given by Grilo
* data: an userdata which points to our opaque (OperationSpec)
* state: holds a string related to the Operation state
  - running: The current ongoing operation
  - waiting: An async call is being performed
  - finalized: Callback was called and this will soon be removed by
    garbage collector

Created attachment 323672 [details] [review]
lua-factory: set state of all operations

This commit uses the functions introduced in previous commit in order to
track the state of each operation and save its state

Created attachment 323673 [details] [review]
Revert "lua-factory: change grl.callback(), grl.fetch() and grl.unzip()"

This reverts commit d809be39ed2bbd4c410be7aa527b36902362511a.

This breaks the current plugins by changing the callback passed as
argument back to grl.callback()

This is not 100% revert. We are keeping the changes to grl.fetch and
grl.unzip.

Created attachment 323674 [details] [review]
Revert "lua-factory: change grl.get_media_keys() into the static table"

This reverts commit 3e409b7727f803e2be4427c7544c679373a7d926.

This breaks the current plugins by changing the static table passed as
argument back grl.get_media_keys()

This is not 100% revert. We are keeping some fixes.

Created attachment 323675 [details] [review]
Revert "lua-factory: change grl.get_options() into the static table"

This reverts commit 2bfcff90d589d4335105a6423616c7de61cf4c71.

Created attachment 323699 [details] [review]
Revert "lua-factory: change grl.get_options() into the static table"

This reverts commit 2bfcff90d589d4335105a6423616c7de61cf4c71.

-- changed in v3
* rebased version after pushing a few patches from Bug 732879
* grl.get_requested_keys() works as suggested by commit 72a4e9d19

--- changed in v4

> I'd just want to make sure that we still get warnings (as in 0.2.x) when a
> source fails to return a result, or returns a result twice, or has a syntax
> error.

* I've addressed this with the watchdog patch
* I've included unit tests about it as well

* A few other fixes as well while testing, removing a few asserts, improving a little bit the comments

besides make test, I tested a bit with grilo-test-ui-0.3

Let me know your feeling about this. Should we have more unit tests? Or some time trying to find issues with it?

PS: Attaching all the patches to not mess with the proper order of the series

Created attachment 323812 [details] [review]
lua-factory: use wrapper for lua_pcall

Due improvements later in the patch series, it becames useful to have
such wrapper. We always call the lua_gc after lua_pcall as current logic
flow expects it.

Created attachment 323813 [details] [review]
lua-factory: Make grl and grl.lua read-only

By creating a proxy table with custom metatable in order to avoid
changes in grl and grl.lua by Lua sources.

The proxy redirects read access (__index metamethod) to the original
table but the write access (__newindex metamethod) triggers an error.

We still can do read-write but only internally; For this, we get a
reference to the original table using the __call metamethod. The handler
expects an userdata argument which makes nontrivial to retrieve it from
Lua sources.

Created attachment 323814 [details] [review]
lua-factory: introduce source state table

The major point is being able to track and retrieve ongoing Grilo
operations data under the same Lua Source.

Before d809be39ed2bbd4c410be we were using the global env in lua (_G) in
order to store the operation and retrieve it but the design of the code
was lacking improvements.
e.g. grl.fetch() and grl.unzip() were bounded to OperationSpec

After d809be39ed2bbd4c410be we removed the the OperationSpec opaque from
othe global context and utilities functions but the API changed for
that.

From now on, we will be able to move back to the former API by storing
again the operation details but without losing the benefits from
d809be39ed2bbd4c410be.

The specifics from each operation will now be stored into our
lua-library table: grl
* grl.__priv_state.current_operation hold the operation being executed;
* grl.__priv_state.operations hole a table of all ongoing operations;

The operation is defined by a state table which has three fields at the
moment:
* op_id: the unique operation id given by Grilo
* data: an userdata which points to our opaque (OperationSpec)
* state: holds a string related to the Operation state
  - running: The current ongoing operation
  - waiting: An async call is being performed
  - finalized: Callback was called and this will soon be removed by
    garbage collector

Created attachment 323815 [details] [review]
lua-factory: set state of all operations

This commit uses the functions introduced in previous commit in order to
track the state of each operation and save its state

Created attachment 323816 [details] [review]
Revert "lua-factory: change grl.callback(), grl.fetch() and grl.unzip()"

This reverts commit d809be39ed2bbd4c410be7aa527b36902362511a.

This breaks the current plugins by changing the callback passed as
argument back to grl.callback()

This is not 100% revert. We are keeping the changes to grl.fetch and
grl.unzip.

Created attachment 323817 [details] [review]
Revert "lua-factory: change grl.get_media_keys() into the static table"

This reverts commit 3e409b7727f803e2be4427c7544c679373a7d926.

This breaks the current plugins by changing the static table passed as
argument back grl.get_media_keys()

This is not 100% revert. We are keeping some fixes.

Created attachment 323818 [details] [review]
Revert "lua-factory: change grl.get_options() into the static table"

This reverts commit 2bfcff90d589d4335105a6423616c7de61cf4c71.

Created attachment 323819 [details] [review]
lua-factory: watchdog for lua-sources

This commit allow us to track broken sources such as:
* sources that never call for grl.callback()
* sources that might call grl.callback() after the operation is
  already finished

This is done by using a userdata in the top of the stack as a watchdog
for the current operation. We always push this userdata in
grl_lua_library_pcall function and it will be always freed by Lua's
garbage collection.

By attaching a finalizer function into this userdata, we can check at
the time that lua_gc is called if the state of Operation is correct.

This idea was introcued by Morse in d809be39ed2bb so I modify those
functions to work as the watchdog we need instead of grl_l_callback
closure.

Created attachment 323820 [details] [review]
tests: include tests for erros in lua sources

Three tests were introduced at this time:
* grl.callback is never called;
* grl.callback is called when operation is finished;
* grl.callback is not called after an async operation

Created attachment 323821 [details] [review]
Revert "tests: port lua-factory fake sources to new API"

This reverts commit d322e89e6e78ac7810b7eaf314421659b77d5ca7.

But keeps grl.fetch callback as function instead of string

Created attachment 323822 [details] [review]
Revert "lua-factory: port grl-guardianvideos.lua to the new lua system"

This reverts commit ccfa70cfe911129c8bdb936fda99dd11cc60bff4.

But keeps grl.fetch callback as function instead of string

Created attachment 323823 [details] [review]
Revert "lua-factory: port grl-euronews.lua to the new lua system"

This reverts commit 592bb1c7a5315e4f2582dad415e03e42f5a43cad.

But keeps grl.fetch callback as function instead of string

Created attachment 323824 [details] [review]
Revert "lua-factory: port grl-appletrailers.lua to the new lua system"

This reverts commit bea8e082da513d90660de84f9213a4fd9552d2eb.

But keeps grl.fetch callback as function instead of string

Created attachment 323825 [details] [review]
Revert "lua-factory: port grl-metrolyrics.lua to the new lua system"

This reverts commit 0e71278f1616a55e4e0a3b719abbe978a4ad71af.

But keeps grl.fetch callback as function instead of string

Created attachment 323826 [details] [review]
Revert "lua-factory: port grl-video-title-parsing.lua to the new lua system"

This reverts commit 46b127a9ccee3a01f95bee47e81305238ab17f3a.

Created attachment 323827 [details] [review]
Revert "lua-factory: port grl-pocket.lua to the new lua system"

This reverts commit 368693b7a8599941d5bfe3b73f440e16ff115a24.

But keeps grl.fetch callback as function instead of string

Created attachment 323828 [details] [review]
Revert "lua-factory: port grl-spotify-cover.lua to the new lua system"

This reverts commit 9b329f3e5246477f039a1fb2b129706370237c89.

But keeps grl.fetch callback as function instead of string

Created attachment 323829 [details] [review]
Revert "lua-factory: port grl-radiofrance.lua to the new lua system"

This reverts commit 93547ac94797cdea6670b0d458e838bcdb578f77.

But keeps grl.fetch callback as function instead of string

Created attachment 323830 [details] [review]
Revert "lua-factory: port grl-lastfm-cover.lua to the new lua system"

This reverts commit fbb244ee962ddcf483dc4c6adec30de6c2616436.

But keeps grl.fetch callback as function instead of string

Review of attachment 323812 [details] [review]:

Looks fine to me.

Review of attachment 323813 [details] [review]:

Looks fine to commit, but after the freeze.

Review of attachment 323814 [details] [review]:

Looks fine otherwise.

::: src/lua-factory/grl-lua-common.h
@@ +47,3 @@
 
+typedef enum {
+  LUA_SOURCE_RUNNING,

Make that read LUA_SOURCE_RUNNING = 0 here and...

::: src/lua-factory/grl-lua-library.c
@@ +64,3 @@
 } UnzipOperation;
 
+static const gchar *source_op_state_str[LUA_SOURCE_NUM_STATES] = {

const gchar const?
(the table is const, and so are the strings)

@@ +65,3 @@
 
+static const gchar *source_op_state_str[LUA_SOURCE_NUM_STATES] = {
+  [LUA_SOURCE_RUNNING] = "running",

you can remove the [] = stuff here.

Review of attachment 323815 [details] [review]:

I think that the states should be refcounted.

If I called grl.fetch() and grl.unzip(), or multiple grl.fetch in the same function, you'd get back to running when you get the first completion signal.

Review of attachment 323814 [details] [review]:

And as mentioned in the review of the next patch, the number of running calls needs to be refcounted.

Review of attachment 323816 [details] [review]:

> This is not 100% revert. We are keeping the changes to grl.fetch and grl.unzip.

What changes? The original commit's message was glib, and so is this one.

What are you reverting, and what are you keeping?

Review of attachment 323817 [details] [review]:

> This is not 100% revert. We are keeping some fixes.

> This breaks the current plugins by changing the static table passed as
> argument back grl.get_media_keys()

"current"? current means before or after the patch?

Review of attachment 323818 [details] [review]:

Please explain in the commit message why you're reverting it.

Review of attachment 323819 [details] [review]:

Looks fine otherwise.

::: src/lua-factory/grl-lua-library.c
@@ +2171,3 @@
+  lua_getfield (L, -1, SOURCE_OP_STATE);
+  str = lua_tostring (L, -1);
+  ret = (g_strcmp0 (str, source_op_state_str[state]) == 0);

You might want a function in the previous patch that introduces source states to get the string that corresponds to an actual LuaSourceState value, as a sanity check.

Review of attachment 323820 [details] [review]:

> tests: include tests for erros in lua sources

errors

Looks good otherwise.

And all the .lua ports look ok to commit, but please add some justification behind the reverts, and explain better what you kept.

Hey, sorry for taking some time. In order to have refcounted operations like, doing two grl.fetch or two grl.unzip in a row, it was necessary to improve the code a little.

As it was getting a big messy I've decided to create grl-lua-library-operations to address this work, starting from the wrapper of lua_pcall, the proxy table and all the handling of operations.

The logic is basically the same, what changed:

1-) Moved most of the code from last series to grl-lua-library-operations and better organized what should be exported to grl-lua-library.c and grl-lua-factory.c inside grl-lua-common.h

2-) In order to have the refcount (which is, indeed, a must) the watchdog becomes vital. The watchdog's finalize is the one that checks the source state and does the memory deallocation.

3-) I moved most of unit tests of errors to be in the resolve operation, which removed some code. The search operation is used for the refcount test.

4-) Regarding the read-only proxy table that should be commited after-freeze, there were some bits there necessary for the watchdog patch. What I did then, was keeping it read-write and only setting the read-only in the last patch (which could be commit-after-freeze if I'm right)

PS: Attaching all patches to make it easy to git-bz apply

Created attachment 324454 [details] [review]
lua-factory: use wrapper for lua_pcall

Due improvements later in the patch series, it becomes useful to have
such wrapper. We always call the lua_gc after lua_pcall as current logic
flow expects it.

Created attachment 324455 [details] [review]
lua-factory: Create a proxy for grl and grl.lua

By creating a proxy table with custom metatable in order to have more
control into changes make in grl and grl.lua by Lua sources.

At the moment, the proxy redirects read access (__index metamethod)
and write access (__newindex metamethod) to the original table but later
on we can remove write access from Lua sources to our library.

Internally we are should be using the __call metamethod in order to
retrieve the original table.

Created attachment 324456 [details] [review]
lua-factory: introduce source state table

The major point is being able to track and retrieve ongoing Grilo
operations data under the same Lua Source.

Before d809be39ed2bbd4c410be we were using the global env in lua (_G) in
order to store the operation and retrieve it but the design of the code
was lacking improvements.
e.g. grl.fetch() and grl.unzip() were bounded to OperationSpec

After d809be39ed2bbd4c410be we removed the the OperationSpec opaque from
othe global context and utilities functions but the API changed for
that.

From now on, we will be able to move back to the former API by storing
again the operation details but without losing the benefits from
d809be39ed2bbd4c410be.

The specifics from each operation will now be stored into our
lua-library table: grl
* grl.__priv_state.current_operation hold the operation being executed;
* grl.__priv_state.operations hole a table of all ongoing operations;

The operation is defined by a state table which has three fields at the
moment:
* op_id: the unique operation id given by Grilo
* data: an userdata which points to our opaque (OperationSpec)
* state: holds a string related to the Operation state
  - running: The current ongoing operation
  - waiting: An async call is being performed
  - finalized: Callback was called and this will soon be removed by
    garbage collector

Created attachment 324457 [details] [review]
lua-factory: set state of all operations

This commit uses the functions introduced in previous commit in order to
track the state of each operation and save its state

Created attachment 324458 [details] [review]
Revert "lua-factory: change grl.callback(), grl.fetch() and grl.unzip()"

In order to keep the previous API between Lua sources and Lua-Factory.

This reverts commit d809be39ed2bbd4c410be7aa527b36902362511a.

After applying this patch, it will break the lua sources due the API
change.  We are not passing callback as argument anymore. Lua sources
should use grl.callback().

This is not 100% revert as we are keeping a few changes to grl.fetch and
to grl.unzip:
* Both those functions are still using a lua function as callback which
  was a string before d809be39;
* Both this functions now can handle userdata as the last argument;
* For grl.fetch, the second argument is still an optional table for
  GrlNetWc;

Created attachment 324459 [details] [review]
Revert "lua-factory: change grl.get_media_keys() into the static table"

In order to keep the previous API between Lua sources and Lua-Factory.

This reverts commit 3e409b7727f803e2be4427c7544c679373a7d926.

This breaks the current plugins by changing the static table passed as
argument back grl.get_media_keys()

After applying this patch, it will break the lua sources due the API
change.  We are not passing an static table as GrlMedia anymore. Lua
sources should use grl.get_media_keys().

This is not 100% revert. We are keeping some fixes that was introduced
together with the api change for grilo-0.3 on grl.get_media_keys such as
using GrlMedia instead of specific types for audio, video, image and box
(which is now container)

Created attachment 324460 [details] [review]
Revert "lua-factory: change grl.get_options() into the static table"

In order to keep the previous API between Lua sources and Lua-Factory.

This reverts commit 2bfcff90d589d4335105a6423616c7de61cf4c71.

After applying this patch, it will break the lua sources due the API
change.  We are not passing an static table with all operation options
to the source anymore.  Lua sources should use grl.get_options() and
grl.get_requested_keys().

The main goal of reverted patch was to provide a table with all options
for the operation handler.

Created attachment 324461 [details] [review]
lua-factory: watchdog for lua-sources

This commit allow us to track broken sources such as:
* sources that never call for grl.callback()
* sources that might call grl.callback() after the operation is
  already finished

This is done by using a userdata in the top of the stack as a watchdog
for the current operation. We always push this userdata in
grl_lua_library_pcall function and it will be always freed by Lua's
garbage collection.

By attaching a finalize function into this userdata, we can check at
the time that lua_gc is called if the state of Operation is correct.

This idea was introduced by Morse in d809be39ed2bb so I modify those
functions to work as the watchdog we need instead of grl_l_callback
closure.

Created attachment 324462 [details] [review]
tests: include tests for errors in lua sources

Three tests were introduced at this time:
* grl.callback is never called;
* grl.callback is called when operation is finished;
* grl.callback is not called after an async operation

Created attachment 324463 [details] [review]
Revert "tests: port lua-factory fake sources to new API"

This reverts commit d322e89e6e78ac7810b7eaf314421659b77d5ca7.

But keeps grl.fetch callback as function instead of string

Created attachment 324464 [details] [review]
Revert "lua-factory: port grl-guardianvideos.lua to the new lua system"

This reverts commit ccfa70cfe911129c8bdb936fda99dd11cc60bff4.

But keeps grl.fetch callback as function instead of string

Created attachment 324465 [details] [review]
Revert "lua-factory: port grl-euronews.lua to the new lua system"

This reverts commit 592bb1c7a5315e4f2582dad415e03e42f5a43cad.

But keeps grl.fetch callback as function instead of string

Created attachment 324466 [details] [review]
Revert "lua-factory: port grl-appletrailers.lua to the new lua system"

This reverts commit bea8e082da513d90660de84f9213a4fd9552d2eb.

But keeps grl.fetch callback as function instead of string

Created attachment 324467 [details] [review]
Revert "lua-factory: port grl-metrolyrics.lua to the new lua system"

This reverts commit 0e71278f1616a55e4e0a3b719abbe978a4ad71af.

But keeps grl.fetch callback as function instead of string

Created attachment 324468 [details] [review]
Revert "lua-factory: port grl-video-title-parsing.lua to the new lua system"

This reverts commit 46b127a9ccee3a01f95bee47e81305238ab17f3a.

Created attachment 324469 [details] [review]
Revert "lua-factory: port grl-pocket.lua to the new lua system"

This reverts commit 368693b7a8599941d5bfe3b73f440e16ff115a24.

But keeps grl.fetch callback as function instead of string

Created attachment 324470 [details] [review]
Revert "lua-factory: port grl-spotify-cover.lua to the new lua system"

This reverts commit 9b329f3e5246477f039a1fb2b129706370237c89.

But keeps grl.fetch callback as function instead of string

Created attachment 324471 [details] [review]
Revert "lua-factory: port grl-radiofrance.lua to the new lua system"

This reverts commit 93547ac94797cdea6670b0d458e838bcdb578f77.

But keeps grl.fetch callback as function instead of string

Created attachment 324472 [details] [review]
Revert "lua-factory: port grl-lastfm-cover.lua to the new lua system"

This reverts commit fbb244ee962ddcf483dc4c6adec30de6c2616436.

But keeps grl.fetch callback as function instead of string

Created attachment 324473 [details] [review]
lua-factory: make grl and grl.lua read-only

By using the metamethod __new_index to trigger an error in case Lua
source try to use it.

Comment on attachment 324454 [details] [review]
lua-factory: use wrapper for lua_pcall

As per earlier review

Comment on attachment 324455 [details] [review]
lua-factory: Create a proxy for grl and grl.lua

To commit after the freeze, for 3.22.

It's not required for the rest of the series to work, is it?

Review of attachment 324456 [details] [review]:

We'll need a test case for that if there's not already one in the list. Please file a separate bug if necessary.

::: src/lua-factory/grl-lua-library-operations.c
@@ +561,3 @@
+
+    if (os->lua_source_waiting_ops > 0) {
+      os->lua_source_waiting_ops -= 1;

os->lua_source_waiting_ops--;

@@ +568,3 @@
+    priv_state_operations_update (L, os, state);
+
+    os->lua_source_waiting_ops += 1;

os->lua_source_waiting_ops++;

Review of attachment 324457 [details] [review]:

Looks good.

Review of attachment 324458 [details] [review]:

Great commit message! :)

Review of attachment 324459 [details] [review]:

Looks good.

Review of attachment 324460 [details] [review]:

Looks good.

Review of attachment 324461 [details] [review]:

Looks fine.

Review of attachment 324461 [details] [review]:

Looks fine.

(In reply to Bastien Nocera from comment #87)
> Comment on attachment 324455 [details] [review] [review]
> lua-factory: Create a proxy for grl and grl.lua
> 
> To commit after the freeze, for 3.22.


I changed the patch from last review. This introduces the proxy table but does not make it read-only so, it is the same as it was before.
 
> It's not required for the rest of the series to work, is it?

Part of the code of watchdog was relying on getting the 'original' table using the metamethod __call. I thought it wasn't the effor to change this bits as we could make the proxy read-only for 3.22 but still keep the proxy.

Does it make sense?

Review of attachment 324462 [details] [review]:

Yep.

Attachment 324454 [details] pushed as 311600c - lua-factory: use wrapper for lua_pcall
Attachment 324455 [details] pushed as 7c9315e - lua-factory: Create a proxy for grl and grl.lua
Attachment 324456 [details] pushed as 1074349 - lua-factory: introduce source state table
Attachment 324457 [details] pushed as 89add48 - lua-factory: set state of all operations
Attachment 324458 [details] pushed as fd58add - Revert "lua-factory: change grl.callback(), grl.fetch() and grl.unzip()"
Attachment 324459 [details] pushed as 500935d - Revert "lua-factory: change grl.get_media_keys() into the static table"
Attachment 324460 [details] pushed as a137919 - Revert "lua-factory: change grl.get_options() into the static table"
Attachment 324461 [details] pushed as f2860cd - lua-factory: watchdog for lua-sources
Attachment 324462 [details] pushed as c55a863 - tests: include tests for errors in lua sources
Attachment 324463 [details] pushed as 2fd5cee - Revert "tests: port lua-factory fake sources to new API"
Attachment 324464 [details] pushed as 1934fe4 - Revert "lua-factory: port grl-guardianvideos.lua to the new lua system"
Attachment 324465 [details] pushed as 6b7e154 - Revert "lua-factory: port grl-euronews.lua to the new lua system"
Attachment 324466 [details] pushed as a789a5b - Revert "lua-factory: port grl-appletrailers.lua to the new lua system"
Attachment 324467 [details] pushed as c3c5462 - Revert "lua-factory: port grl-metrolyrics.lua to the new lua system"
Attachment 324468 [details] pushed as 1310c86 - Revert "lua-factory: port grl-video-title-parsing.lua to the new lua system"
Attachment 324469 [details] pushed as ccc3b1b - Revert "lua-factory: port grl-pocket.lua to the new lua system"
Attachment 324470 [details] pushed as cee1083 - Revert "lua-factory: port grl-spotify-cover.lua to the new lua system"
Attachment 324471 [details] pushed as 2e0d1c8 - Revert "lua-factory: port grl-radiofrance.lua to the new lua system"
Attachment 324472 [details] pushed as e473ad9 - Revert "lua-factory: port grl-lastfm-cover.lua to the new lua system"
Attachment 324473 [details] pushed as 3b61064 - lua-factory: make grl and grl.lua read-only