GNOME Bugzilla – Bug 753141
Get rid of the globals in lua-factory
Last modified: 2016-01-30 11:29:42 UTC
There are two major scenarios where lua-factory uses globals where it really shouldn't 1. To store operation spec data. 2. To find callbacks for fetch and unzip. Patch is coming soon.
Created attachment 308620 [details] [review] grilo_patch This patch stops the use of the globals in Lua where they are inappropriate. There are two scenarios taken care of, both of which has upsides and downsides. 1. The storing of the OperationSpec. OperationSpec is stored in a global Lua table, and extracted from there in a a hope that no racing will screw the things up. As a result of this lots of function were made globally accessible, although they really do not make any sence outside of context. This patch - Changes the way the OperationSpec is stored. Now it is stored as a userdata available as an upvalue for only two functions: former grl.callback and grl.get_options that are now passed as C closures directly to the lua function as parameters (named options and callback resp.) - Changes the way async functions behave. They do not require OperationSpec directly now unless they have really something to do with it. - Changes the way the errors are caught. The userdata representing the OperationSpec is linked to its closures, and is marked for GC as soon as the closures leave the scope. The __gc metafunction is called once userdata gets destroyed and check whether the request was finished. This ensures that the check would be done eventually. This patch also invokes GC in several places to speed things up a little. Upsides: - No globals, which is always good :) - Some function (e.g. grl.fetch) now will work even when there is no OperationSpec, like from init. - Several operations performing async fetches simultaniously now will work OK Downsides: - Lots of advanced Lua-magic, like C closures, upvalues, and metatables (__gc) - Error checking relies on Lua GC 2. The callbacks for async operations The callback are passed to the C code as strings, representing the functions in global scope. This makes it impossible to provide additional parameters to it. This patch - Simply changes the strings into Lua references. Upsides: - Now it is possible to provide the additional parameters to the callbacks by using anonymous functions Downsides: - None Tested with my VK plugin, which I now was able to finish in lua. Browse and search work ok, I didn't test query and resolve. All the intricate magic seem to work.
Created attachment 308622 [details] [review] grilo_patch Strict typization in lua-json.c as well.
Created attachment 308631 [details] [review] grilo_patch New version additionally provides a way to provide userdata to callbacks in a way similar to the usual glib async callbacks.
Review of attachment 308631 [details] [review]: Overall, this seems fine, even if I didn't review the grl-lua-library.c part of the patch thoroughly. You'll need to separate out the type changes into another patch, as well as splitting the move of functions, so that the patches are smaller. Please also make sure to run a spellchecker on the commit message, quite a few typos. We'll also need to make sure that the plugins tests all pass, and that there are no changes needed to existing sources. ::: src/lua-factory/grl-lua-factory.c @@ -1265,3 @@ /* Auto-split-threshold */ lua_getfield (L, -6, LUA_SOURCE_AUTO_SPLIT_THRESHOLD); - lua_auto_split_threshold = lua_tonumber (L, -1); Those changes could be made in a separate patch too. ::: src/lua-factory/grl-lua-library.c @@ +277,3 @@ case G_TYPE_FLOAT: if (lua_isnumber (L, -1)) + grl_data_add_float (GRL_DATA (media), key_id, lua_tonumber (L, -1)); Those should happen in a separate patch. @@ +704,2 @@ /** +* grl.get_media_keys Move the functions in a separate patch. We can't easily see what changes there are besides code moving otherwise. ::: src/lua-factory/lua-library/lua-json.c @@ -102,3 @@ break; case G_TYPE_BOOLEAN: - lua_pushnumber (L, json_reader_get_boolean_value (reader)); Those changes can be made in a separate patch.
>Move the functions in a separate patch. We can't easily see what changes there are besides code moving otherwise. Actually, in this case this is exactly code moving, nothing else. Ok, I'll split the patch in chunks.
Created attachment 308770 [details] [review] grilo_patch-1 [1/7] Change to-/is-/push- number to integer whenever applicable. And pushinteger to pushnumber in one odd case.
Created attachment 308771 [details] [review] grilo_patch-2 [2/7] Move get_requested_keys and get_media_keys inside get_options.
Created attachment 308772 [details] [review] grilo_patch-3 [3/7] The main patch. Here the OperationSpec magic is changed.
Created attachment 308773 [details] [review] grilo_patch-4 [4/7] The second main patch. Here the callbacks are changed to functions and Lua userdata added
Created attachment 308774 [details] [review] grilo_patch-5 [5/7] Appendix 1. Small fix to net_wc_new_with_options()
Created attachment 308775 [details] [review] grilo_patch-1
Created attachment 308776 [details] [review] grilo_patch-6 [6/7] Appendix 2. Small fix for GOA functions.
Created attachment 308777 [details] [review] grilo_patch-7 [7/7] Appendix 3. Small fix to get_options.
Review of attachment 308770 [details] [review]: Patch looks good, only one thing: You should bump the check on lua version to 5.3
Review of attachment 308771 [details] [review]: Yes, it does make sense :) So, the main problem of this patch (and probably others from now on) is that it breaks API and our current plugins: $ grep -ni "get_media_keys\|get_requested_keys" * grl-metrolyrics.lua:59: req = grl.get_media_keys() grl-musicbrainz.lua:50: req = grl.get_media_keys() grl-video-title-parsing.lua:111: req = grl.get_media_keys()
(In reply to Victor Toso from comment #14) > Review of attachment 308770 [details] [review] [review]: > > Patch looks good, only one thing: You should bump the check on lua version > to 5.3 Hm, lua 5.2 has pushinteger and tointeger, but no isinteger. Odd. But that indeed means that the check should be updated. (In reply to Victor Toso from comment #15) > Review of attachment 308771 [details] [review] [review]: > > Yes, it does make sense :) > So, the main problem of this patch (and probably others from now on) is that > it breaks API and our current plugins: It will break all the plugins. The good news is - the needed changes would be pretty much trivial. I'll do all of them when this patchset would be ready for merging.
> Tested with my VK plugin, which I now was able to finish in lua. Browse and > search work ok, I didn't test query and resolve. All the intricate magic > seem to work. It would be good to have the vk-plugin attached to test this working as it breaks the other plugins.
I could do that, but it requires VK API token, which you probably do not have, and also VK support for GOA, which is not mainlined yet. I can update some plugin from the default setup for you to test. Just tell me which one.
Review of attachment 308772 [details] [review]: > grl.fetch() and grl.unzip() can now be used from anywhere, but > the plaintext verification is not working anymore. Which we included less then a month ago due https://bugzilla.gnome.org/show_bug.cgi?id=747953 This should keep working. The state of the patch looks good as well, I do need one plugin to better test/understand it. Great work :) ::: src/lua-factory/grl-lua-factory.c @@ +1482,3 @@ lua_pop (L, 1); } + lua_gc (L, LUA_GCCOLLECT, 0); Is it necessary to lua_gc after every operation? Wouldn't be enough to do it when callback is called? ::: src/lua-factory/grl-lua-library.c @@ +1463,3 @@ + os->cb.resolve (os->source, os->operation_id, NULL, os->user_data, NULL); + break; + Why do you call the callback on GC? Isn't it enough to call the callback on grl.callback() and there call the lua_gc() to free things below? @@ +1511,3 @@ + * Creates a C closure using the copy of the value on top of the + * lua stack as a single upvalue. Pushes the original value that + * was on top of the stack back there, making the C closure second. Comment is translating the code, not really helpful. I would prefer having explicit that top of the stack is the userdata for the function and also that this function does not change the stack afterwards (meaning that the caller should remove it).
(In reply to Morse from comment #18) > I could do that, but it requires VK API token, which you probably do not > have, and also VK support for GOA, which is not mainlined yet. > > I can update some plugin from the default setup for you to test. Just tell > me which one. I have jhbuild working here, I can apply the patches, just point me to them ;)
Review of attachment 308777 [details] [review]: Yup, bugfix
Review of attachment 308774 [details] [review]: > Fix the GrlNetWc options being ignored in > some cases Which cases? Can you improve a bit the comment to explain the change?
(In reply to Victor Toso from comment #19) > Review of attachment 308772 [details] [review] [review]: > > > grl.fetch() and grl.unzip() can now be used from anywhere, but > > the plaintext verification is not working anymore. > > Which we included less then a month ago due > https://bugzilla.gnome.org/show_bug.cgi?id=747953 > This should keep working. Ok, so I see two solutions. First - we can put the relevant information (whether plaintext search is allowed) in the lua global space, just like the GOA info is stored. The second would be to parse the source struct for the "net:plaintext" tag every time a fetch is done. I actually have the code for both already, whichever you prefer. > ::: src/lua-factory/grl-lua-factory.c > @@ +1482,3 @@ > lua_pop (L, 1); > } > + lua_gc (L, LUA_GCCOLLECT, 0); > > Is it necessary to lua_gc after every operation? Wouldn't be enough to do it > when callback is called? It is pointless to call it from inside the callback, because the callback itself is what we want to collect. callback+userdata (which is OperationSpec) constitute a C closure which we created when the operation started and want to collect when everything is finished. And the places where I explicitly call GC are exactly the possible places where "everything finishes". > > ::: src/lua-factory/grl-lua-library.c > @@ +1463,3 @@ > + os->cb.resolve (os->source, os->operation_id, NULL, os->user_data, > NULL); > + break; > + > > Why do you call the callback on GC? > Isn't it enough to call the callback on grl.callback() and there call the > lua_gc() to free things below? We call it if the finishing callback wasn't done by the plugin. When lua code finishes, and OperationSpec is marked for GC, we check whether the finishing callback was done, and if not, do it ourselves. > @@ +1511,3 @@ > + * Creates a C closure using the copy of the value on top of the > + * lua stack as a single upvalue. Pushes the original value that > + * was on top of the stack back there, making the C closure second. > > Comment is translating the code, not really helpful. > I would prefer having explicit that top of the stack is the userdata for the > function and also that this function does not change the stack afterwards > (meaning that the caller should remove it). Creates a C closure using the userdata, which should already be on top of the stack, and leaves that userdata back in its original place, which means that it should removed from there manually. (In reply to Victor Toso from comment #22) > Review of attachment 308774 [details] [review] [review]: > > > Fix the GrlNetWc options being ignored in > > some cases > > Which cases? Can you improve a bit the comment to explain the change? In cases where the net options are on top of the stack, which is pretty much every case.
Created attachment 309132 [details] [review] grilo_test_patch-1 Updated grl_appletrailers, the first one alphabetically. Tested OK in grl-test-ui.
Created attachment 310355 [details] [review] lua-factory: Stricter integer typization for Lua Lua 5.3 introduced a new integer type to Lua. Now it should be used instead of previous "number" whenever applicable.
Created attachment 310356 [details] [review] lua-factory: Move get_requested_keys and get_media_keys inside get_options Now accessible as grl.get_options("requested-keys") and grl.get_options("media-keys")
Created attachment 310357 [details] [review] lua-factory: Change the way OperationSpec is saved and accessed Operation spec is now saved as an upvalue for the C closures instead of the global table. Functions grl.get_options and grl.callback were removed from the global scope, and are now passed as a second and third parameters to the lua functions (first and second in case of resolve()). The check whether the operation was finished correctly (finishing callback was made) is now done by the OperationSpec finalizer which is called by the Lua GC when the user code is done (when OperationSpec leaves the Lua scope and hence is marked for GC). grl.fetch() and grl.unzip() can now be used from anywhere, but the plaintext verification is not working anymore.
Created attachment 310358 [details] [review] lua-factory: Change the way Lua callbacks are working. Lua callbacks are now functions, not strings. Functions that use callbacks (grl.fetch and grl.unzip) now accept an additional parameter - userdata, which behaves like the usual glib async callback userdata: it is passed to the callback of the said function.
Created attachment 310359 [details] [review] lua-factory: Fix the GrlNetWc options being ignored in some cases
Created attachment 310360 [details] [review] lua-factory: Fix the memory leak in GOA-related functions
Created attachment 310361 [details] [review] Update grl-appletrailers for the new infrastructure
Comment on attachment 310355 [details] [review] lua-factory: Stricter integer typization for Lua Marking as needs-work as per previous review
I updated all the patch statuses, and re-uploaded them using git-bz, so that the bugzilla patch descriptions are actually that, descriptive.
Created attachment 310362 [details] [review] lua-factory: Stricter integer typization for Lua Lua 5.3 introduced a new integer type to Lua. Now it should be used instead of previous "number" whenever applicable.
Comment on attachment 310360 [details] [review] lua-factory: Fix the memory leak in GOA-related functions Attachment 310360 [details] pushed as e88d6e5 - lua-factory: Fix the memory leak in GOA-related functions
Review of attachment 310356 [details] [review]: I don't understand the point of doing that. Although the requested_keys could be considered an option, media_keys really aren't. I don't like the name "get_media_keys" though and I think it would be better to pass in the original GrlMedia instead.
Comment on attachment 310362 [details] [review] lua-factory: Stricter integer typization for Lua Attachment 310362 [details] pushed as 8b18594 - lua-factory: Stricter integer typization for Lua
Review of attachment 310358 [details] [review]: > Subject: [PATCH] lua-factory: Change the way Lua callbacks are working. No period at the end of subjects. This is missing changes to all the lua plugins that use this, of which there are a few (either in the same patch, or all in a separate patch, no need for a patch per source in any case). ::: src/lua-factory/grl-lua-library.c @@ +44,3 @@ typedef struct { lua_State *L; + gint lua_ud; We don't pay per character, so I'd rather have this spelt out (eg. lua_userdata).
Review of attachment 310357 [details] [review]: > grl.fetch() and grl.unzip() can now be used from anywhere, but > the plaintext verification is not working anymore. We can't regress on that though...
Review of attachment 310361 [details] [review]: I'd rather all the sources were fixed, and if possible, were fixed as the API is changed, so that developers of sources can see the necessary changes be applied step-by-step.
(In reply to Bastien Nocera from comment #36) > Review of attachment 310356 [details] [review] [review]: > > I don't understand the point of doing that. Although the requested_keys > could be considered an option, media_keys really aren't. > > I don't like the name "get_media_keys" though and I think it would be better > to pass in the original GrlMedia instead. The point is to move all the functions that require access to OperationSpec into one. Since OperationSpec is not global anymore, every function that require access to it should be passed to the lua as a parameter. I made just the two of them: options() and callback(). So I just made the least changes possible in order to change the OperationSpec behavior. Removing get_media_keys completely in favor of passing the media itself is outside of this scope, I think. (In reply to Bastien Nocera from comment #39) > Review of attachment 310357 [details] [review] [review]: > > > grl.fetch() and grl.unzip() can now be used from anywhere, but > > the plaintext verification is not working anymore. > > We can't regress on that though... I already understood that. I have two ideas how to fix this: we can put the information about whether the plaintext fetch is allowed into the lua globals just like we put the GOA object, or we can parse the "source" structure and search for the tag every time the fetch is done. I have the working code for both solution, whichever you prefer. (In reply to Bastien Nocera from comment #40) > Review of attachment 310361 [details] [review] [review]: > > I'd rather all the sources were fixed, and if possible, were fixed as the > API is changed, so that developers of sources can see the necessary changes > be applied step-by-step. So... I didn't quite get what you want me to do :) Just in case: I changed this plugin so people can see that the new infra is working (and also that the amount of needed changes is not that big). I picked it only because it was the first one alphabetically.
Created attachment 310544 [details] [review] lua-factory: grl-video-title-parsing use integer Commit 8b18594e65fc1196c1d270ee3fcefe55195ea397 makes lua sources uses the new integer type. In the case of video-title-parsing, season and episode metadata-keys should use tonumber(). Test failure was: Grilo-WARNING **: [lua-library] grl-lua-library.c:357: 'no value' is not compatible for 'season'
So, I really want this changes as I think it could improve lua-factory and dev experience with the plugins... But we do need some tests or else we may not find bugs until it happen with users.
Can you point me to the code dealing with this test? It's not like it's possible to change tointeger() to tonumber() for just some metadata keys. tointeger() is used because the keys in question are of the int type. Also, I don't see how tonumber() will help, because lua_typename() doesn't say "number", it says "no value". IIRC, previously such things were converted into NaNs by tonumber(), hardly a desirable behavior.
(In reply to Morse from comment #44) > Can you point me to the code dealing with this test? For this test: https://git.gnome.org/browse/grilo-plugins/tree/tests/local-metadata/test_local_metadata.c#n150 But what I mean is test for lua-factory changes itself as well which could go here: https://git.gnome.org/browse/grilo-plugins/tree/tests/lua-factory > > It's not like it's possible to change tointeger() to tonumber() for just > some metadata keys. tointeger() is used because the keys in question are of > the int type. Also, I don't see how tonumber() will help, because > lua_typename() doesn't say "number", it says "no value". IIRC, previously > such things were converted into NaNs by tonumber(), hardly a desirable > behavior. I did not see any tointeger() so that's why I use tonumber() in the above patch. Do you think the fix should go in lua-library instead?
(In reply to Victor Toso from comment #45) > (In reply to Morse from comment #44) > > Can you point me to the code dealing with this test? > For this test: > https://git.gnome.org/browse/grilo-plugins/tree/tests/local-metadata/ > test_local_metadata.c#n150 > > But what I mean is test for lua-factory changes itself as well which could > go here: > https://git.gnome.org/browse/grilo-plugins/tree/tests/lua-factory Hm, I fail to see how the local-metadata is connected to the lua-factory. Also the code lua_typename (L, -1) is plain wrong. You should change it to lua_typename (L, lua_type(L, -1)) That way we'll at least see where the problem lies.
(In reply to Morse from comment #46) > Hm, I fail to see how the local-metadata is connected to the lua-factory. > The grl-vide-title-parsing was introduced to simplify local-metadata. Lua patterns is easier and probably faster then regexp. Both are connected in the same test to check if both are compatible. > Also the code > > lua_typename (L, -1) > > is plain wrong. You should change it to > > lua_typename (L, lua_type(L, -1)) That's true, I'll do another patch to fix this all over the code. Thanks. > > That way we'll at least see where the problem lies. Using the correct way, we have: Grilo-WARNING **: [lua-library] grl-lua-library.c:357: 'string' is not compatible for 'episode' This is due the fact that lua_isinteger does not convert string to integer so season = "3" and episode "4" will fail. I'm either thinking about keeping the patch or having (lua_isnumber || lua_isinteger) in lua-library.
Created attachment 310792 [details] [review] lua_factory: fix wrong types of 'season' and 'episode' fields in video title parsing Ok, so we have two approaches here. First one, we can require that the season and episode fields were integers, not strings. Which is easier, but will break stuff, and introduce inconsistency.
Created attachment 310794 [details] [review] lua_factory: accept the strings representing integers as integer type The second approach would be to check and convert strings to integers manually on our side, since lua won't do it automatically. I do like it better, actually. I didn't test the patches. I'm away from my build machine.
Review of attachment 310792 [details] [review]: Same as my previous patch
(In reply to Morse from comment #48) > Ok, so we have two approaches here. First one, we can require that the > season and episode fields were integers, not strings. Which is easier, but > will break stuff, and introduce inconsistency. I agree. I'm rejecting my patch as well to fix the source instead of lua-library... we should be able to use "3" as number/integer IMHO. (In reply to Morse from comment #49) > The second approach would be to check and convert strings to integers > manually on our side, since lua won't do it automatically. > > I do like it better, actually. > > I didn't test the patches. I'm away from my build machine. Yeah, looks better approach.
Review of attachment 310544 [details] [review]: Instead of fixing sources to convert numeric string such "3" we should fix this in lua-library and keep the compatibility
Hi Morse, Related to the tests I've started something here [0]. I'll try to address comment #46 and comment #49 tomorrow (hopefully!) [0] https://bugzilla.gnome.org/show_bug.cgi?id=755447
Created attachment 313830 [details] [review] lua-factory: Move get_requested_keys inside get_options
Created attachment 313831 [details] [review] lua-factory: remove OperationSpec from global scope I updated my patches. I decided not to merge grl.get_media_keys into get_options. After I studied what exactly does it do, and where it's called, I instead changed it into grl_lua_library_push_grl_media (). This new function is called when resolve or browse operation are prepared, and provides the media as a first parameter for lua functions, the same way as search text and query are the first parameters for search and query operations.
Created attachment 314511 [details] [review] lua-factory: Change the way the options are accessed Change options into the table. callback() is the only function now. options contain the following: count skip operation_id requested_keys (table) filters (table) There are three types of filters: range_filters, which are the tables containing two key-value pairs: "min" and "max", key_filters, containing only one pair: "value", and one filter called "types" which contains three key-value pairs, with keys being "audio", "video" and "image", and boolean values.
Review of attachment 310358 [details] [review]: Patch is obsolete
Review of attachment 310356 [details] [review]: Patch is obsolete
Review of attachment 310357 [details] [review]: Patch is obsolete
Review of attachment 313830 [details] [review]: This looks good. We should push this only after the changes on all plugins.
Review of attachment 313831 [details] [review]: grl-lua-common and grl-lua-factory part seems okay. grl-lua-library.c is a bit messy; - you moved code around besides including new code. - you introduce options here with functions but change it in the next patch ::: src/lua-factory/grl-lua-library.c @@ +250,1 @@ Fix this in a separated patch. @@ -1023,3 @@ - return 1; -} - Huge code block being moved * @@ +1648,3 @@ + } else { + GRL_DEBUG ("Key %s has unhandled G_TYPE. Lua source will miss this data", + key_name); Please don't move entirely functions around?
Review of attachment 314511 [details] [review]: ::: src/lua-factory/grl-lua-factory.c @@ +1479,3 @@ text = (ss->text == NULL) ? "" : ss->text; + os = g_slice_new (OperationSpec); why this change? @@ +1494,3 @@ grl_lua_library_prepare_operation_parameters (L, os); if (lua_pcall (L, 3, 0, 0)) { 4 arguments now @@ +1512,3 @@ GRL_DEBUG ("grl_lua_factory_source_browse"); + os = g_slice_new (OperationSpec); ditto @@ +1527,3 @@ grl_lua_library_prepare_operation_parameters (L, os); if (lua_pcall (L, 3, 0, 0)) { ditto @@ +1548,3 @@ query = (qs->query == NULL) ? "" : qs->query; + os = g_slice_new (OperationSpec); ditto @@ +1580,1 @@ ditto @@ +1584,1 @@ os->source = rs->source; ditto @@ +1596,3 @@ grl_lua_library_prepare_operation_parameters (L, os); if (lua_pcall (L, 3, 0, 0)) { ditto ::: src/lua-factory/grl-lua-library.c @@ -904,3 @@ - return 0; -} - definitely belongs to the patch that introduce the options argument. @@ +1788,3 @@ + lua_settable (L, -3); +} + This function definitely need helpers. (It seems that some functions were removed to make this big one...)
(In reply to Victor Toso from comment #61) > Review of attachment 313831 [details] [review] [review]: > > grl-lua-common and grl-lua-factory part seems okay. > grl-lua-library.c is a bit messy; > - you moved code around besides including new code. > - you introduce options here with functions but change it in the next patch > > ::: src/lua-factory/grl-lua-library.c > @@ +250,1 @@ > > > Fix this in a separated patch. It's not a fix per se. Both methods are viable, the change is needed to remove the dependence on GrlSource, since it's not globally accessible anymore. I can do it in a separate patch, but the change would hardly make sense by itself, without the context of the globals removal. > > @@ -1023,3 @@ > - return 1; > -} > - > > Huge code block being moved * > > @@ +1648,3 @@ > + } else { > + GRL_DEBUG ("Key %s has unhandled G_TYPE. Lua source will miss this > data", > + key_name); > > Please don't move entirely functions around? While the code inside the function remained pretty much the same, the main idea behind it changed. Previously, it was a realization of the internal API method grl.get_media_keys, and hence was located in the "Lua-Library methods" section. Now it's a preparatory routine, which is called automatically and not by user. I moved it to the "Lua-Library and Lua-Factory utilities" section (and also renamed it) because now it fills the similar role as previous grl_lua_library_save/load/set/remove_operation_data (alongside with the new functions placed in the same sections and named in the same manner). I can leave the function in the previous place, but I think it will worsen the readability of the code: the name and the place of the function would suggest that it does something else. > you introduce options here with functions but change it in the next patch Well, it was your suggestion to replace the options() function with the static table. I did it in a separate patch, that's all. (In reply to Victor Toso from comment #62) > Review of attachment 314511 [details] [review] [review]: > > ::: src/lua-factory/grl-lua-factory.c > @@ +1479,3 @@ > text = (ss->text == NULL) ? "" : ss->text; > > + os = g_slice_new (OperationSpec); > > why this change? Since now all the fields of the OperationSpec are explicitly initialized, I thought that g_slice_new0 is redundant now. Just optimization, I can leave it as it was. > > @@ +1494,3 @@ > grl_lua_library_prepare_operation_parameters (L, os); > > if (lua_pcall (L, 3, 0, 0)) { > > 4 arguments now search term/browse item/query/whatnot options callback What else? grl_lua_library_prepare_operation_parameters() now pushes only the callback, since options are prepared in a separate function. > ::: src/lua-factory/grl-lua-library.c > @@ -904,3 @@ > - return 0; > -} > - > > definitely belongs to the patch that introduce the options argument. Well, this IS this patch. The previous introduced options() as a function. > @@ +1788,3 @@ > + lua_settable (L, -3); > +} > + > > This function definitely need helpers. (It seems that some functions were > removed to make this big one...) Can separate it in three.
(In reply to Morse from comment #63) > (In reply to Victor Toso from comment #61) > > Review of attachment 313831 [details] [review] [review] [review]: > > > > grl-lua-common and grl-lua-factory part seems okay. > > grl-lua-library.c is a bit messy; > > - you moved code around besides including new code. > > - you introduce options here with functions but change it in the next patch > > > > ::: src/lua-factory/grl-lua-library.c > > @@ +250,1 @@ > > > > > > Fix this in a separated patch. > > It's not a fix per se. Both methods are viable, the change is needed to > remove the dependence on GrlSource, since it's not globally accessible > anymore. I can do it in a separate patch, but the change would hardly make > sense by itself, without the context of the globals removal. > > > > > @@ -1023,3 @@ > > - return 1; > > -} > > - > > > > Huge code block being moved * > > > > @@ +1648,3 @@ > > + } else { > > + GRL_DEBUG ("Key %s has unhandled G_TYPE. Lua source will miss this > > data", > > + key_name); > > > > Please don't move entirely functions around? > > While the code inside the function remained pretty much the same, the main > idea behind it changed. Previously, it was a realization of the internal API > method grl.get_media_keys, and hence was located in the "Lua-Library > methods" section. Now it's a preparatory routine, which is called > automatically and not by user. I moved it to the "Lua-Library and > Lua-Factory utilities" section (and also renamed it) because now it fills > the similar role as previous > grl_lua_library_save/load/set/remove_operation_data (alongside with the new > functions placed in the same sections and named in the same manner). > > I can leave the function in the previous place, but I think it will worsen > the readability of the code: the name and the place of the function would > suggest that it does something else. Point is that this will change behavior, break api. If I'm expecting to look at this patch in the future to look for a bug I would prefer not have to parse moving code around. So, if it is necessary to change function name, its parameters that's fine. Moving the code around should be done separately but only when it really matters. > > > you introduce options here with functions but change it in the next patch > > Well, it was your suggestion to replace the options() function with the > static table. I did it in a separate patch, that's all. Yes but does it make sense for you to have two patches in arrow, one introducing options as table of functions and the other as options as static table? Please, don't :) > > (In reply to Victor Toso from comment #62) > > Review of attachment 314511 [details] [review] [review] [review]: > > > > ::: src/lua-factory/grl-lua-factory.c > > @@ +1479,3 @@ > > text = (ss->text == NULL) ? "" : ss->text; > > > > + os = g_slice_new (OperationSpec); > > > > why this change? > > Since now all the fields of the OperationSpec are explicitly initialized, I > thought that g_slice_new0 is redundant now. Just optimization, I can leave > it as it was. Yes, please. > > > > > @@ +1494,3 @@ > > grl_lua_library_prepare_operation_parameters (L, os); > > > > if (lua_pcall (L, 3, 0, 0)) { > > > > 4 arguments now > > search term/browse item/query/whatnot > options > callback > > What else? grl_lua_library_prepare_operation_parameters() now pushes only > the callback, since options are prepared in a separate function. Right, I was confused with the change of grl_lua_library_prepare_operation_parameters ... > > > ::: src/lua-factory/grl-lua-library.c > > @@ -904,3 @@ > > - return 0; > > -} > > - > > > > definitely belongs to the patch that introduce the options argument. > > Well, this IS this patch. The previous introduced options() as a function. Yes I did not express myself well enough but you got it. > > > @@ +1788,3 @@ > > + lua_settable (L, -3); > > +} > > + > > > > This function definitely need helpers. (It seems that some functions were > > removed to make this big one...) > > Can separate it in three. Thanks.
> Yes but does it make sense for you to have two patches in arrow, one introducing options as table of functions and the other as options as static table? >Please, don't :) So, you want me to rearrange the patches including all the changes in one patch? That'll be a lot of changes for one patch.
Created attachment 316393 [details] [review] lua-factory: remove dependency on GrlSource from plaintext verification
Created attachment 316394 [details] [review] lua-factory: change grl.get_options() into the static table
Created attachment 316395 [details] [review] lua-factory: change grl.get_media_keys() into the static table
Created attachment 316398 [details] [review] lua-factory: change grl.callback(), grl.fetch() and grl.unzip() Here's the bunch of rearranged patches. I took the liberty of changing the order of parameters to grl.fetch and grl.unzip. Previously it was like this: grl.fetch ("url", "callback", {network = "parameters"}) grl.unzip ("url", {"list", "of", "files"}, "callback", {network = "parameters"}) The last parameter is optional. And with this commit the new userdata field is added. Which is also optional. So if we just add it to the end, we'll have two optional parameters in a row, which is not that good. Doubly so, as the network parameters are rarely used, while the userdata is expected to be used quite often (for the grilo callback). So I decided to change the order, and to place network parameters right before callback, but still to keep it optional. Since we know that the callback should be of a lua_function type, it won't be a problem to discern what parameters are provided. So now it looks like this: grl.fetch ("url", {network = "parameters"}, callback, userdata) or grl.fetch ("url", callback, userdata) or just grl.fetch ("url", callback) All the combinations are acceptable. Same with unzip. Are you ok with this, or do you prefer some other order?
(In reply to Morse from comment #69) > Created attachment 316398 [details] [review] [review] > lua-factory: change grl.callback(), grl.fetch() and grl.unzip() > > Here's the bunch of rearranged patches. > > I took the liberty of changing the order of parameters to grl.fetch and > grl.unzip. Oh but you love breaking stuff :) > > Previously it was like this: > > grl.fetch ("url", "callback", {network = "parameters"}) > grl.unzip ("url", {"list", "of", "files"}, "callback", {network = > "parameters"}) > > The last parameter is optional. And with this commit the new userdata field > is added. Which is also optional. So if we just add it to the end, we'll > have two optional parameters in a row, which is not that good. Doubly so, as > the network parameters are rarely used, while the userdata is expected to be > used quite often (for the grilo callback). The network parameters is useful mostly when the web service require specifics of how or from whom is fetching data. It may not be widely used but it is nice to have. > > So I decided to change the order, and to place network parameters right > before callback, but still to keep it optional. Since we know that the > callback should be of a lua_function type, it won't be a problem to discern > what parameters are provided. > > So now it looks like this: > > grl.fetch ("url", {network = "parameters"}, callback, userdata) > or > grl.fetch ("url", callback, userdata) > or just > grl.fetch ("url", callback) > All the combinations are acceptable. Same with unzip. > > Are you ok with this, or do you prefer some other order? Yeah, I'm okay with it! I hope to be reviewing your patches shortly.
Review of attachment 316394 [details] [review]: Great job with the help functions here. Please, include in the commit log that the functions that you removed (name it) were supersed by the functions that you have created in order to help following the changes. ::: src/lua-factory/grl-lua-library.c @@ +1588,3 @@ + + lua_newtable (L); + i = 1; i is initialized twice @@ +1589,3 @@ + lua_newtable (L); + i = 1; + for (it = keys; it; it = g_list_next (it)) { it != NULL is preferred and it is okay to use it = it->next @@ +1598,3 @@ + + key_name = grl_registry_lookup_metadata_key_name (registry, key_id); + lua_pushinteger (L, i++); do the i++ in a new line @@ +1625,3 @@ + if (!lua_istable (L, -1)) { + GRL_WARNING ("push_operation_range_filters was called \n\ + with no lua table on top of the stack"); Is this check necessary? this would likely show a bug on grl_lua_library_push_grl_options. If you keep the check, don't include new line to GRL_WARNING. Using two strings "" is preferred as well, like: GRL_WARNING ("push_operation_range_filters was called " "with no lua table on top of the stack"); @@ +1626,3 @@ + GRL_WARNING ("push_operation_range_filters was called \n\ + with no lua table on top of the stack"); + return; and you are leaking range_filters @@ +1694,3 @@ + /* since the value would be used for comparison, and since os.time() + * returns unix time in lua, it'd be a good idea to pass g_date_time + * as unix time here */ We don't enable os library in our sandbox https://git.gnome.org/browse/grilo-plugins/commit/?id=2e25300444cc0b799defa054e53b3 iso 8601 seems better in this case @@ +1701,3 @@ + lua_pushinteger (L, g_date_time_to_unix (date)); + } else + lua_pushnil (L); use braces in else if `if` does. also, 2 spaces identation @@ +1708,3 @@ + lua_pushinteger (L, g_date_time_to_unix (date)); + } else + lua_pushnil (L); ditto. @@ +1713,3 @@ + + /* ignore G_TYPE_BYTE_ARRAY since I can't see how it + * can possibly be used for filtering */ I think it is safe to remove this comment @@ +1752,3 @@ + if (!lua_istable (L, -1)) { + GRL_WARNING ("push_operation_filters was called \n\ + with no lua table on top of the stack"); ditto. @@ +1753,3 @@ + GRL_WARNING ("push_operation_filters was called \n\ + with no lua table on top of the stack"); + return; leaking filters @@ +1756,3 @@ + } + + for (it = filters; it; it = g_list_next (it)) { ditto. @@ +1779,3 @@ + + /* Although a table isn't really necessary for non-range filters, for the sake of + * uniformity it's best to make all the filters of the same type */ "all filters should be kept in table" is enough @@ +1809,3 @@ + + /* ignore G_TYPE_BYTE_ARRAY since I can't see how it + * can possibly be used for filtering */ I think it is safe to remove this two comments as well. @@ +1817,3 @@ + if (success) { + lua_settable (L, -3); + lua_settable (L, -3); this looks wrong? first settable is for the table you have created. second settable is for the one outside this function. If this operation is correct, it does not belong here. @@ +1868,3 @@ + lua_pushstring (L, "types"); + lua_newtable (L); + type_filter = grl_operation_options_get_type_filter (options); create push_operation_type_filter (L, options); @@ +1881,3 @@ + + push_operation_range_filters (L, options); + push_operation_filters (L, options); So, these three functions would behave exactly the same, waiting for the filter table in top of the stack.
Review of attachment 316393 [details] [review]: looks good otherwise ::: src/lua-factory/grl-lua-library.c @@ +231,3 @@ + lua_getglobal (L, LUA_SOURCE_TABLE); + if (!lua_istable (L, -1)) + return FALSE; you must remove nil from top of stack (due lua_getglobal) @@ +234,3 @@ + lua_getfield (L, -1, LUA_SOURCE_TAGS); + if (!lua_istable (L, -1)) + return FALSE; you must remove table + nil from top of stack
Review of attachment 316395 [details] [review]: Looks good!
Review of attachment 316398 [details] [review]: ::: src/lua-factory/grl-lua-library.c @@ +879,1 @@ * @netopts: (table) Options to set the GrlNetWc object. change the order here. urls, netopts, callback, userdata @@ +901,3 @@ + luaL_argcheck (L, (lua_isfunction (L, 2) || + (lua_istable (L, 2) && lua_isfunction (L, 3))), 3, + "expecting callback function after network parameters"); if 2nd is function, you don't chack (if @@ +903,3 @@ + "expecting callback function after network parameters"); + + /* net option parameter is omitted */ Overall, I prefer comments that explain the reason to the code. /* keep the arguments in the order */ @@ +911,3 @@ + if (lua_gettop (L) > 4) { + GRL_WARNING ("too many arguments to 'fetch' function, ignored"); + } I think it is safe to luaL_error in case of lua_gettop > 4. In this case, move it below the luaL_argcheck @@ +955,3 @@ } + wc = net_wc_new_with_options(L, 2); space between function and () @@ +997,3 @@ + g_return_val_if_fail (lua_isuserdata(L, lua_upvalueindex(1)), 0); + os = *((OperationSpec **)lua_touserdata (L, lua_upvalueindex(1))); space between function and () Also, I preferred how you did below, something like p = lua_touserdata (L, lua_upvalueindex (1)); os = *p; @@ +1169,2 @@ * @netopts: (table) Options to set the GrlNetWc object. + * @userdata: User data to be passed to the @callback. maybe it would be better to have netopt after url to match grl.fetch and any other in the future. @@ +1192,3 @@ + "expecting callback function after network parameters"); + + /* net option parameter is omitted */ ditto. @@ +1200,3 @@ + if (lua_gettop (L) > 5) { + GRL_WARNING ("too many arguments to 'unzip' function, ignored"); + } ditto. @@ +1450,2 @@ { + OperationSpec **userdata = lua_touserdata( L, 1 ); lua_touserdata (L, 1); @@ +1504,2 @@ { + OperationSpec **userdata = lua_newuserdata (L, sizeof(OperationSpec *) ); sizeof (OperationSpec *)); @@ +1515,3 @@ lua_settable (L, -3); + /* set table as the metatable of the userdata */ + lua_setmetatable (L, -2); btw, this is really cool.
Created attachment 316720 [details] [review] [PATCH 1/4] lua-factory: remove dependency on GrlSource from plaintext verification
Created attachment 316721 [details] [review] [PATCH 2/4] lua-factory: change grl.get_options() into the static table
Created attachment 316722 [details] [review] [PATCH 3/4] lua-factory: change grl.get_media_keys() into the static table
Created attachment 316723 [details] [review] [PATCH 4/4] lua-factory: change grl.callback(), grl.fetch() and grl.unzip() (In reply to Victor Toso from comment #71) > Review of attachment 316394 [details] [review] [review]: > @@ +1625,3 @@ > + if (!lua_istable (L, -1)) { > + GRL_WARNING ("push_operation_range_filters was called \n\ > + with no lua table on top of the stack"); > > Is this check necessary? this would likely show a bug on > grl_lua_library_push_grl_options. I forgot to change it to g_assert. Changed now. > @@ +1694,3 @@ > + /* since the value would be used for comparison, and since os.time() > + * returns unix time in lua, it'd be a good idea to pass g_date_time > + * as unix time here */ > > We don't enable os library in our sandbox > https://git.gnome.org/browse/grilo-plugins/commit/ > ?id=2e25300444cc0b799defa054e53b3 > > iso 8601 seems better in this case This one is tricky. The values in filters are not intended for comparison with current time (os.time) but rather with the time of items that plugin will return. I said about os.time because I thought that it means that the use of the unix time is a common practice in lua. I did a quick sweep of various online APIs (VK, FB, Google...) and found out that both epoch and ISO are used, so whatever we choose we'll leave someone very sad. I myself would rather use epoch, because of two reasons: one, I think it's easier to convert epoch into ISO than vise versa, and two, it's impossible to just compare two ISO strings to find out which date is earlier (timezone info will mess things up), so some conversions would be needed anyways. Also, because VK uses epoch :) > @@ +1817,3 @@ > + if (success) { > + lua_settable (L, -3); > + lua_settable (L, -3); > > this looks wrong? > first settable is for the table you have created. > second settable is for the one outside this function. If this operation is > correct, it does not belong here. No, that's correct. The filters table which this function expects is set in the end of grl_lua_library_push_grl_options, these two are the filter itself, and the value inside said filter (options.filters.filter_itself.value) (In reply to Victor Toso from comment #74) > Review of attachment 316398 [details] [review] [review]: > @@ +911,3 @@ > + if (lua_gettop (L) > 4) { > + GRL_WARNING ("too many arguments to 'fetch' function, ignored"); > + } > > I think it is safe to luaL_error in case of lua_gettop > 4. > In this case, move it below the luaL_argcheck Makes sense. I changed it to error, but we shouldn't move this since we can only tell whether the number of parameters is exceeded after we aligned the netopts > @@ +1169,2 @@ > * @netopts: (table) Options to set the GrlNetWc object. > + * @userdata: User data to be passed to the @callback. > > maybe it would be better to have netopt after url to match grl.fetch and any > other in the future. Yes, sure. I changed both functions at the same time, just forgot about comments. Everything else is fixed.
Created attachment 317509 [details] [review] [PATCH 01/10] lua-factory: port grl-appletrailers.lua to the new lua
Created attachment 317510 [details] [review] [PATCH 02/10] lua-factory: port grl-euronews.lua to the new lua
Created attachment 317511 [details] [review] [PATCH 03/10] lua-factory: port grl-guardianvideos.lua to the new lua
Created attachment 317512 [details] [review] [PATCH 04/10] lua-factory: port grl-lastfm-cover.lua to the new lua
Created attachment 317513 [details] [review] [PATCH 05/10] lua-factory: port grl-metrolyrics.lua to the new lua
Created attachment 317514 [details] [review] [PATCH 06/10] lua-factory: port grl-musicbrainz.lua to the new lua
Created attachment 317515 [details] [review] [PATCH 07/10] lua-factory: port grl-pocket.lua to the new lua system
Created attachment 317516 [details] [review] [PATCH 08/10] lua-factory: port grl-radiofrance.lua to the new lua
Created attachment 317517 [details] [review] [PATCH 09/10] lua-factory: port grl-spotify-cover.lua to the new lua
Created attachment 317518 [details] [review] [PATCH 10/10] lua-factory: port grl-video-title-parsing.lua to the
Hi, (In reply to Morse from comment #78) > > We don't enable os library in our sandbox > > https://git.gnome.org/browse/grilo-plugins/commit/ > > ?id=2e25300444cc0b799defa054e53b3 > > > > iso 8601 seems better in this case > > This one is tricky. The values in filters are not intended for comparison > with current time (os.time) but rather with the time of items that plugin > will return. I said about os.time because I thought that it means that the > use of the unix time is a common practice in lua. I did a quick sweep of > various online APIs (VK, FB, Google...) and found out that both epoch and > ISO are used, so whatever we choose we'll leave someone very sad. I myself > would rather use epoch, because of two reasons: one, I think it's easier to > convert epoch into ISO than vise versa, and two, it's impossible to just > compare two ISO strings to find out which date is earlier (timezone info > will mess things up), so some conversions would be needed anyways. > > Also, because VK uses epoch :) right. we will definitely need a new function in lua-library for handling time... it isn't blocker for this series anyway. Keeping epoch for now seems fine. > > > @@ +1817,3 @@ > > + if (success) { > > + lua_settable (L, -3); > > + lua_settable (L, -3); > > > > this looks wrong? > > first settable is for the table you have created. > > second settable is for the one outside this function. If this operation is > > correct, it does not belong here. > > No, that's correct. The filters table which this function expects is set in > the end of grl_lua_library_push_grl_options, these two are the filter > itself, and the value inside said filter > (options.filters.filter_itself.value) I'll take a better look again > > > (In reply to Victor Toso from comment #74) > > Review of attachment 316398 [details] [review] [review] [review]: > > > @@ +911,3 @@ > > + if (lua_gettop (L) > 4) { > > + GRL_WARNING ("too many arguments to 'fetch' function, ignored"); > > + } > > > > I think it is safe to luaL_error in case of lua_gettop > 4. > > In this case, move it below the luaL_argcheck > > Makes sense. I changed it to error, but we shouldn't move this since we can > only tell whether the number of parameters is exceeded after we aligned the > netopts Sure > > > @@ +1169,2 @@ > > * @netopts: (table) Options to set the GrlNetWc object. > > + * @userdata: User data to be passed to the @callback. > > > > maybe it would be better to have netopt after url to match grl.fetch and any > > other in the future. > > Yes, sure. I changed both functions at the same time, just forgot about > comments. > > > Everything else is fixed. Awesome. I'll be reviewing and testing this shortly :)
Review of attachment 317515 [details] [review]: make test fails ::: src/lua-factory/sources/grl-pocket.lua @@ +59,3 @@ grl.debug ("Fetching URL: " .. url .. " (count: " .. count .. " skip: " .. skip .. ")") + local userdata = {callback = callback, count = count) (test_local_metadata:17235): Grilo-WARNING **: [lua-factory] grl-lua-factory.c:944: [/home/vtosodec/jhbuild/src/grilo-plugins/src/lua-factory/sources/grl-pocket.lua] failed to load: /home/vtosodec/jhbuild/src/grilo-plugins/src/lua-factory/sources/grl-pocket.lua:61: '}' expected near ')' Fixed locally...
Review of attachment 317518 [details] [review]: ::: src/lua-factory/sources/grl-video-title-parsing.lua @@ +140,3 @@ -- related to episode and season number if parse_as_episode(media) then grl.debug(req.title .. " is an EPISODE") (test_local_metadata:1539): Grilo-WARNING **: [lua-factory] grl-lua-factory.c:1606: calling resolve function fail: '/home/vtosodec/jhbuild/src/grilo-plugins/src/lua-factory/sources/grl-video-title-parsing.lua:142: attempt to index a nil value (global 'req')' FAIL req was removed but still being used
so, make test is failing on local-metadata test (which does test grl-video-title-parsing) I could verify that grl-video-title-parsing is not broken (after fixup in comment #92) and that lua-library append the right metadata to GrlMedia. I still need to track why the test receive NULL metadata. I'll try to take another look tonight as I wasn't exactly able to do a proper review yet. I'll be pushing the rebased patches + small fixes
Created attachment 318575 [details] [review] lua-factory: change grl.get_media_keys() into the static table It's part of the work aimed at removing the OperationSpec from the global scope. grl_l_media_get_keys was replaced by grl_lua_library_push_grl_media -- changes: fixes due the latest changes in Grilo related to GrlMedias type
Created attachment 318576 [details] [review] lua-factory: port grl-pocket.lua to the new lua system -- changes: fixed typo on table creation
Created attachment 318577 [details] [review] lua-factory: port grl-video-title-parsing.lua to the new lua system -- changes: removed table was still in use
Created attachment 318578 [details] [review] lua-factory: port grl-metrolyrics.lua to the new lua system -- changes: conflict on rebase
Review of attachment 317513 [details] [review]: I forgot to mark this as obsolete anyway, anothr small nitpick: ::: src/lua-factory/sources/grl-metrolyrics.lua @@ +87,2 @@ end + userdta.callback(userdata.media, 0) hmm, i did not see this typo when fixing the rebase conflict. make test did not reach this metrolyrics test yet. @@ +112,3 @@ feed = feed:gsub("^[%s%W]*(.-)[%s%W]*$", "%1") + return feed I agree that 'metrolyrics_get_lyrics' should return lyrics and not a media... but this could be two patches ;) - to correctly use the function - to change api
Review of attachment 317517 [details] [review]: ::: src/lua-factory/sources/grl-spotify-cover.lua @@ +62,1 @@ album = grl.encode(req.album) req does not exist anymore ~ fixed locally
Created attachment 318613 [details] [review] lua-factory: remove dependency on GrlSource from plaintext verification It's part of the work aimed at removing the OperationSpec from the global scope.
Created attachment 318614 [details] [review] lua-factory: change grl.get_options() into the static table It's part of the work aimed at removing the OperationSpec from the global scope. grl_l_operation_get_keys was replced by push_operation_requested_keys grl_l_operation_get_options was split and replaced by push_operation_type_filter push_operation_range_filters push_operation_filters grl_lua_library_push_grl_options
Created attachment 318615 [details] [review] lua-factory: change grl.get_media_keys() into the static table It's part of the work aimed at removing the OperationSpec from the global scope. grl_l_media_get_keys was replaced by grl_lua_library_push_grl_media
Created attachment 318616 [details] [review] lua-factory: change grl.callback(), grl.fetch() and grl.unzip() This commit removes grl.callback() and changes the behavior of the grl.fetch() and grl.unzip(). The grl.callback function is now provided as a parameter to all the operations, and grl.fetch and grl.unzip now require callback as a lua function, not a string. Also, they now accept userdata. This commit finishes the work of removing the OperationSpec from the global scope. functions grl_lua_library_save/load/remove_operation_data and functions grl_lua_library_set/get_current_operation were removed functions grl_util_operation_spec_gc push_operation_spec_userdata and grl_lua_library_push_grl_callback were added
Created attachment 318617 [details] [review] lua-factory: port grl-appletrailers.lua to the new lua system
Created attachment 318618 [details] [review] lua-factory: port grl-euronews.lua to the new lua system
Created attachment 318619 [details] [review] lua-factory: port grl-guardianvideos.lua to the new lua system
Created attachment 318620 [details] [review] lua-factory: port grl-lastfm-cover.lua to the new lua system
Created attachment 318621 [details] [review] lua-factory: port grl-radiofrance.lua to the new lua system
Created attachment 318622 [details] [review] lua-factory: port grl-spotify-cover.lua to the new lua system
Created attachment 318623 [details] [review] lua-factory: port grl-pocket.lua to the new lua system
Created attachment 318624 [details] [review] lua-factory: port grl-video-title-parsing.lua to the new lua system
Created attachment 318625 [details] [review] lua-factory: port grl-metrolyrics.lua to the new lua system
Created attachment 318626 [details] [review] tests: port lua-factory fake sources to new API
So far I've rebased your patches due some changes in Grilo and fixed small problems pointed by make test and also the not so small one: https://bugzilla.gnome.org/show_bug.cgi?id=760382 _now_ I hope to be able to proper review the patches :-) So far, I'm happy to see make test working and the small work to move sources from one api to another. About the time used as metadata from lua-factory -> lua sources, it seems that we are not using epoch in another part of grl-lua-library. We should stick with one and provide a util function for lua-sources to get the time and convert it. As I said earlier, this isn't a blocker for this IMO. Let's see if I can finish this review this week.
So, biggest issue so far. We should return the metadata in the same GrlMedia object. the callback() handler creates a new GrlMedia and return it. This is not expected from Grilo (and some CRITICAL/WARNINGS should be placed) and it isn't expected from Application either. 1-) media table to lua source is great! 2-) we should merge the resulting media table with the orginal GrlMedia
>We should return the metadata in the same GrlMedia object. I guess you are referring to resolve(). I didn't know that. Nothing in the grilo docs suggested it. I suppose such an important note should be stressed out in "Writing plugins for Grilo" tutorial. I'll return this behavior back then. I removed it for the sake of uniformity of code (in industry, I was taught that uniformity is the most important thing, maybe I'm pushing it too far :)
(In reply to Morse from comment #116) > >We should return the metadata in the same GrlMedia object. > I guess you are referring to resolve(). I didn't know that. Me neither :-) I always used the same GrlMedia and never thought about it. Now that the behavior was changed that I could see it. > Nothing in the > grilo docs suggested it. I suppose such an important note should be stressed > out in "Writing plugins for Grilo" tutorial. I agree, this was discussed today on IRC. Patches are welcome for docs as well o/ > > I'll return this behavior back then. I removed it for the sake of uniformity > of code (in industry, I was taught that uniformity is the most important > thing, maybe I'm pushing it too far :) Many thanks for your work.
Created attachment 319144 [details] [review] [PATCH 5/4] lua-factory: return back the behaviour of the resolve() with this the behavior of the resolve() should be the same as before.
(In reply to Morse from comment #118) > Created attachment 319144 [details] [review] [review] > [PATCH 5/4] lua-factory: return back the behaviour of the resolve() > > with this the behavior of the resolve() should be the same as before. Thanks! I'll be testing and reviewing everything today/tomorrow.
(In reply to Victor Toso from comment #119) > Thanks! I'll be testing and reviewing everything today/tomorrow. ... in 2016-01-15 :-( (In reply to Morse from comment #118) > Created attachment 319144 [details] [review] [review] > [PATCH 5/4] lua-factory: return back the behaviour of the resolve() > > with this the behavior of the resolve() should be the same as before. Looks good. It should be squashed with 'lua-factory: change grl.get_media_keys() into the static table' .. I did it locally so, no big deal... I'm reviewing everything now, finally.
Well, just a reminder: the error-reporting functionality that I included in the ported sources relies on a patch from here: https://bugzilla.gnome.org/show_bug.cgi?id=759013 Not that there are tests that check the correctness of the error reporting, but still.
Review of attachment 318613 [details] [review]: Looks good apart from this ::: src/lua-factory/grl-lua-library.c @@ +241,3 @@ + + lua_pushnil (L); + lua_getfield (L, -1, LUA_SOURCE_TAGS); space after while @@ +253,1 @@ + lua_pop (L, 3); Isn't it lua_pop (L, 2); ? There is table (source) and table (tags), nothing else (lua_next pushes nothing after returning 0)
(In reply to Victor Toso from comment #122) > Isn't it lua_pop (L, 2); ? > > There is table (source) and table (tags), nothing else (lua_next pushes > nothing after returning 0) Hm, it looks like you're right. But somehow the pattern while (lua_next ()) { ... lua_pop (1); } lua_pop (something + 1 for the key); seems oddly familiar to me. Unfortunately, I'm away from my code right now, but I'm afraid the similar misuse of lua_next() may also be in some other places.
Review of attachment 318614 [details] [review]: Looks good to me
(In reply to Morse from comment #121) > Well, just a reminder: the error-reporting functionality that I included in > the ported sources relies on a patch from here: > https://bugzilla.gnome.org/show_bug.cgi?id=759013 Sure, I'll look into it after I finish this series > Not that there are tests that check the correctness of the error reporting, > but still. Well, I'd love some help to have more tests, really ;) (In reply to Morse from comment #123) > (In reply to Victor Toso from comment #122) > > Isn't it lua_pop (L, 2); ? > > > > There is table (source) and table (tags), nothing else (lua_next pushes > > nothing after returning 0) > > Hm, it looks like you're right. But somehow the pattern > > while (lua_next ()) { > ... > lua_pop (1); > } > lua_pop (something + 1 for the key); > > seems oddly familiar to me. Unfortunately, I'm away from my code right now, > but I'm afraid the similar misuse of lua_next() may also be in some other > places. No problem, I'll be careful on review o/
Created attachment 319589 [details] [review] lua-factory: change grl.get_media_keys() into the static table It's part of the work aimed at removing the OperationSpec from the global scope. grl_l_media_get_keys was replaced by grl_lua_library_push_grl_media -- Squashed the patch that returns behavior of GrlMedia in resolve obsolete both patches in bz!
Review of attachment 319589 [details] [review]: commit message could be improved by mentioning Grilo expects the same GrilMedia container to be returned on resolve operation (some logic changed just because of that!) Looks good otherwise PS: In order to apply patches with git-bz, this one is the 3rd patch :) ::: src/lua-factory/grl-lua-library.c @@ +996,2 @@ if (nparam > 0) { + media = grl_util_build_media (L, os->media); no need to change the parameter to os->media now (probably my mistake due squashing both patches)
Review of attachment 318616 [details] [review]: Only minor things, looks good otherwise ::: src/lua-factory/grl-lua-common.h @@ -76,3 @@ gpointer user_data; guint error_code; - guint pending_ops; You forgot to remote it from the doc above ::: src/lua-factory/grl-lua-library.c @@ +882,3 @@ +* @netopts: [optional] (table) Options to set the GrlNetWc object. +* @callback: (function) The function to be called after fetch is complete. +* @userdata: [optional] User data to be passed to the @callback. I wonder if userdata is really optional. As callback is now a parameter, it should be the best practice to pass that as userdata here... ... I can't see how useful it is to have > grl.fetch(urls, fetch_cb) (The code seems right, the only possible tweak here is in the documentation [optional] for userdata) @@ +1451,3 @@ **/ +static int +grl_util_operation_spec_gc (lua_State* L) ... (lua_State *L) @@ +1478,2 @@ + GRL_WARNING ("Source '%s' is broken, as the finishing " + "callback was not called for %s operation", grl_source_get_id (os->source), type); GRL_ERROR seems better here! @@ +1491,1 @@ + g_slice_free(OperationSpec, os); space between function and (
Review of attachment 318617 [details] [review]: check the sources with grilo-test-ui, I think the logs will point this kind of issue ::: src/lua-factory/sources/grl-appletrailers.lua @@ +73,3 @@ end grl.debug('Fetching URL: ' .. url .. ' (count: ' .. count .. ' skip: ' .. skip .. ')') s/count/userdata.count s/skip/userdata.skip @@ +131,1 @@ if count == 0 then s/count/userdata.count @@ +135,3 @@ end if count ~= 0 then s/count/userdata.count
Review of attachment 318618 [details] [review]: yep
Review of attachment 318619 [details] [review]: looks good
Review of attachment 318620 [details] [review]: Looks good otherwise ::: src/lua-factory/sources/grl-lastfm-cover.lua @@ +75,2 @@ if not result then + userdata.callback('error', 'Failed to connect to Last.fm') So, this one depends on https://bugzilla.gnome.org/show_bug.cgi?id=759013 I would prefer not to set error _now_ and let this series in asap. Then when #759013 gets in, we can improve errors of all sources
Review of attachment 318621 [details] [review]: yep
Review of attachment 318622 [details] [review]: ::: src/lua-factory/sources/grl-spotify-cover.lua @@ +76,2 @@ if not result then + userdata.callback("error", "Failed to connect to spotify") once again, let's do it after https://bugzilla.gnome.org/show_bug.cgi?id=759013 get in
Review of attachment 318623 [details] [review]: looks good otherwise ::: src/lua-factory/sources/grl-pocket.lua @@ +83,2 @@ if not results then + userdata.callback("error", "Failed to connect to pocket") yep, after https://bugzilla.gnome.org/show_bug.cgi?id=759013 get in!
Review of attachment 318624 [details] [review]: looks good
Review of attachment 318625 [details] [review]: go go go ::: src/lua-factory/sources/grl-metrolyrics.lua @@ +82,3 @@ + if not lyrics then + userdata.callback("error","This Lyrics do not match our parser! Please file a bug!") + return https://bugzilla.gnome.org/show_bug.cgi?id=759013 this series is all about the current api changes that you are introducing, let this other improvement for another patch
Overall series are good and probably ready to be pushed after this round of fixes.
(In reply to Victor Toso from comment #128) > Review of attachment 318616 [details] [review] [review]: > I wonder if userdata is really optional. As callback is now a parameter, it > should be the best practice to pass that as userdata here... Well, I could invent some weird code that doesn't need userdata, like grl.fetch (url, function (data) callback (build_media (data)) end) but when I wrote [optional], I didn't mean "rarely used parameter", I meant that it is theoretically possible to call the function without it. And of course you can change the docs to whatever you like. > @@ +1478,2 @@ > + GRL_WARNING ("Source '%s' is broken, as the finishing " > + "callback was not called for %s operation", > grl_source_get_id (os->source), type); > > GRL_ERROR seems better here! Is it? My impression was that lua errors translate into grilo warnings. (In reply to Victor Toso from comment #129) > Review of attachment 318617 [details] [review] [review]: > > check the sources with grilo-test-ui, I think the logs will point this kind > of issue > > ::: src/lua-factory/sources/grl-appletrailers.lua > @@ +73,3 @@ > end > > grl.debug('Fetching URL: ' .. url .. ' (count: ' .. count .. ' skip: ' > .. skip .. ')') > > s/count/userdata.count > s/skip/userdata.skip Actually, this should work. We still have access to local count and skip > @@ +131,1 @@ > if count == 0 then > > s/count/userdata.count > > @@ +135,3 @@ > end > > if count ~= 0 then > > s/count/userdata.count Here, yes, that is true. And strange, I was positively sure that I made an extra pass over the code looking exactly for all "count" and "skip" usages. (In reply to Victor Toso from comment #137) > Review of attachment 318625 [details] [review] [review]: > > go go go > > ::: src/lua-factory/sources/grl-metrolyrics.lua > @@ +82,3 @@ > + if not lyrics then > + userdata.callback("error","This Lyrics do not match our parser! > Please file a bug!") > + return > > https://bugzilla.gnome.org/show_bug.cgi?id=759013 > > this series is all about the current api changes that you are introducing, > let this other improvement for another patch Yes, maybe it wasn't correct to put it all at once. So, all the changes left are pretty trivial. I'm afraid it will take me longer to prepare and submit all these patches than to actually introduce all the fixes. Do you maybe have some built-in tool to introduce little changes on the fly? If not, I'll do the stuff this evening (i.e. in 8-10 hours).
(In reply to Morse from comment #139) > (In reply to Victor Toso from comment #128) > > Review of attachment 318616 [details] [review] [review] [review]: > > > I wonder if userdata is really optional. As callback is now a parameter, it > > should be the best practice to pass that as userdata here... > > Well, I could invent some weird code that doesn't need userdata, like > > grl.fetch (url, function (data) callback (build_media (data)) end) > > but when I wrote [optional], I didn't mean "rarely used parameter", I meant > that it is theoretically possible to call the function without it. And of > course you can change the docs to whatever you like. Yeah, I don't mean we should change the code. It was more thought about weird code possibilities. No need to change the docs as well. > > > @@ +1478,2 @@ > > + GRL_WARNING ("Source '%s' is broken, as the finishing " > > + "callback was not called for %s operation", > > grl_source_get_id (os->source), type); > > > > GRL_ERROR seems better here! > > Is it? My impression was that lua errors translate into grilo warnings. Yes, we used GRL_WARNING all over the place, but I felt that this one was different. All the GRL_WARNING was on wrong API calls but this one is doing the callback call for the broken source. I don't have strong feelings about either way. > > (In reply to Victor Toso from comment #129) > > Review of attachment 318617 [details] [review] [review] [review]: > > > > check the sources with grilo-test-ui, I think the logs will point this kind > > of issue > > > > ::: src/lua-factory/sources/grl-appletrailers.lua > > @@ +73,3 @@ > > end > > > > grl.debug('Fetching URL: ' .. url .. ' (count: ' .. count .. ' skip: ' > > .. skip .. ')') > > > > s/count/userdata.count > > s/skip/userdata.skip > > Actually, this should work. We still have access to local count and skip True, thanks > > > @@ +131,1 @@ > > if count == 0 then > > > > s/count/userdata.count > > > > @@ +135,3 @@ > > end > > > > if count ~= 0 then > > > > s/count/userdata.count > > Here, yes, that is true. And strange, I was positively sure that I made an > extra pass over the code looking exactly for all "count" and "skip" usages. > > (In reply to Victor Toso from comment #137) > > Review of attachment 318625 [details] [review] [review] [review]: > > > > go go go > > > > ::: src/lua-factory/sources/grl-metrolyrics.lua > > @@ +82,3 @@ > > + if not lyrics then > > + userdata.callback("error","This Lyrics do not match our parser! > > Please file a bug!") > > + return > > > > https://bugzilla.gnome.org/show_bug.cgi?id=759013 > > > > this series is all about the current api changes that you are introducing, > > let this other improvement for another patch > > Yes, maybe it wasn't correct to put it all at once. > > So, all the changes left are pretty trivial. I'm afraid it will take me > longer to prepare and submit all these patches than to actually introduce > all the fixes. Do you maybe have some built-in tool to introduce little > changes on the fly? If not, I'll do the stuff this evening (i.e. in 8-10 > hours). Yes, it is all trivial. I don't mind doing the fixes locally later today or tomorrow. Are you okay for a better commit message as suggested in comment #127 ?
> Yes, it is all trivial. I don't mind doing the fixes locally later today or > tomorrow. Are you okay for a better commit message as suggested in comment > #127 ? Yes, sure, I'm ok with anything. It's your repo after all, you'll be the one dealing with all this code after I'd be long gone :)
Created attachment 320054 [details] [review] lua-factory: remove dependency on GrlSource from plaintext verification It's part of the work aimed at removing the OperationSpec from the global scope. https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320055 [details] [review] lua-factory: change grl.get_options() into the static table It's part of the work aimed at removing the OperationSpec from the global scope. grl_l_operation_get_keys was replced by push_operation_requested_keys grl_l_operation_get_options was split and replaced by push_operation_type_filter push_operation_range_filters push_operation_filters grl_lua_library_push_grl_options https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320056 [details] [review] lua-factory: change grl.get_media_keys() into the static table It's part of the work aimed at removing the OperationSpec from the global scope. grl_l_media_get_keys was replaced by grl_lua_library_push_grl_media Although the GrlMedia is now a parameter for the lua source, Grilo expects that the same GrlMedia object will be returned in the callback for the Resolve operation [0]; For that reason we still keep track of GrlMedia and merge it with the resulting GrlMedia from Lua source. [0] see https://bugzilla.gnome.org/show_bug.cgi?id=760382#c3 https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320057 [details] [review] lua-factory: change grl.callback(), grl.fetch() and grl.unzip() This commit removes grl.callback() and changes the behavior of the grl.fetch() and grl.unzip(). The grl.callback function is now provided as a parameter to all the operations, and grl.fetch and grl.unzip now require callback as a lua function, not a string. Also, they now accept userdata. This commit finishes the work of removing the OperationSpec from the global scope. functions grl_lua_library_save/load/remove_operation_data and functions grl_lua_library_set/get_current_operation were removed functions grl_util_operation_spec_gc push_operation_spec_userdata and grl_lua_library_push_grl_callback were added https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320058 [details] [review] lua-factory: port grl-appletrailers.lua to the new lua system https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320059 [details] [review] lua-factory: port grl-euronews.lua to the new lua system https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320060 [details] [review] lua-factory: port grl-guardianvideos.lua to the new lua system https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320061 [details] [review] lua-factory: port grl-lastfm-cover.lua to the new lua system https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320062 [details] [review] lua-factory: port grl-radiofrance.lua to the new lua system https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320063 [details] [review] lua-factory: port grl-spotify-cover.lua to the new lua system https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320064 [details] [review] lua-factory: port grl-pocket.lua to the new lua system https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320065 [details] [review] lua-factory: port grl-video-title-parsing.lua to the new lua system https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320066 [details] [review] lua-factory: port grl-metrolyrics.lua to the new lua system https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com>
Created attachment 320067 [details] [review] lua-factory: port grl-appletrailers.lua to the new lua system https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com> -- edit forgot to remove error parameter on callback (wip from https://bugzilla.gnome.org/show_bug.cgi?id=759013)
Created attachment 320068 [details] [review] lua-factory: port grl-euronews.lua to the new lua system https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com> -- edit forgot to remove error parameter on callback (wip from https://bugzilla.gnome.org/show_bug.cgi?id=759013)
Created attachment 320069 [details] [review] lua-factory: port grl-guardianvideos.lua to the new lua system https://bugzilla.gnome.org/show_bug.cgi?id=753141 Acked-by: Victor Toso <me@victortoso.com> -- edit forgot to remove error parameter on callback (wip from https://bugzilla.gnome.org/show_bug.cgi?id=759013)
Attachment 318626 [details] pushed as d322e89 - tests: port lua-factory fake sources to new API Attachment 320054 [details] pushed as 66abb77 - lua-factory: remove dependency on GrlSource from plaintext verification Attachment 320055 [details] pushed as 2bfcff9 - lua-factory: change grl.get_options() into the static table Attachment 320056 [details] pushed as 3e409b7 - lua-factory: change grl.get_media_keys() into the static table Attachment 320057 [details] pushed as d809be3 - lua-factory: change grl.callback(), grl.fetch() and grl.unzip() Attachment 320061 [details] pushed as fbb244e - lua-factory: port grl-lastfm-cover.lua to the new lua system Attachment 320062 [details] pushed as 93547ac - lua-factory: port grl-radiofrance.lua to the new lua system Attachment 320063 [details] pushed as 9b329f3 - lua-factory: port grl-spotify-cover.lua to the new lua system Attachment 320064 [details] pushed as 368693b - lua-factory: port grl-pocket.lua to the new lua system Attachment 320065 [details] pushed as 46b127a - lua-factory: port grl-video-title-parsing.lua to the new lua system Attachment 320066 [details] pushed as 0e71278 - lua-factory: port grl-metrolyrics.lua to the new lua system Attachment 320067 [details] pushed as bea8e08 - lua-factory: port grl-appletrailers.lua to the new lua system Attachment 320068 [details] pushed as 592bb1c - lua-factory: port grl-euronews.lua to the new lua system Attachment 320069 [details] pushed as ccfa70c - lua-factory: port grl-guardianvideos.lua to the new lua system