GNOME Bugzilla – Bug 711243
Lua-Factory Plugin
Last modified: 2014-02-24 23:36:35 UTC
Created attachment 258707 [details] [review] Search, Load and Initialize Lua sources This is a series of patches to include the Lua-Factory plugin with Lua sources and documentation to the grilo-plugins. Those first three patches are the the Lua-factory code. The first patch search and load the lua source; The second patch is the glue-code between Grilo operations and the Lua code; The third patch is the library code, a lua module to help the lua code. One could test it with the lua-flickr [0] using the grilo-test-ui api-key [1] to make a public browse or including the personal tokens provides by Flickr to make a personal browse in your sets (grilo-test-ui generate those too) [0] https://github.com/victortoso/grilo-plugins/blob/master/src/lua-factory/sources/grl-flickr.lua [1] https://git.gnome.org/browse/grilo/tree/tools/grilo-test-ui/main.c#n41
Created attachment 258708 [details] [review] Grilo operations and the Lua code
Created attachment 258709 [details] [review] the library code
Created attachment 259236 [details] [review] Grilo operations and the Lua code
Created attachment 259237 [details] [review] the library code
Review of attachment 258707 [details] [review]: ::: src/Makefile.am @@ +56,3 @@ +if LUA_FACTORY_PLUGIN +SUBDIRS += lua-factory You need to add also this directory in DIST_SUBDIRS, at the end of the file ::: src/lua-factory/grl-lua-common.h @@ +54,3 @@ + GrlSource *source; + guint operation_id; + LuaOperationType lot; could you use a more descriptive name, like op_type? @@ +64,3 @@ + GrlMedia *media; + GrlMedia *container; + } content; Why this union, if both elements are of the same type? ::: src/lua-factory/grl-lua-factory.c @@ +26,3 @@ +#include "lua.h" +#include "lauxlib.h" +#include "lualib.h" If they are system headers, use <>, not quotes @@ +42,3 @@ +#define LUA_FACTORY_PLUGIN_ID "grl-lua-factory" +#define LUA_FACTORY_PLUGIN_NAME "Lua-Factory" +#define LUA_FACTORY_PLUGIN_DESC _("A plugin to load lua based plugins") NAME and DESC definitions are not used anywhere @@ +80,3 @@ +static GList *get_lua_sources(void); + +static GrlLuaFactorySource *grl_lua_factory_source_new(gchar *, GList *); Please, add the variable names in all these declarations. @@ +122,3 @@ + source = grl_lua_factory_source_new(it->data, configs); + + /* Get the source and check its init function before registering */ Is this comment outdated? it is not checking any "init" function @@ +172,3 @@ + goto source_new_error; + + ret = lua_pcall(L, 0, 0, 0); Interesting.... which function is invoked here? And why we need to invoke it? @@ +227,3 @@ +source_new_end: + lua_pop(L, 1); + lua_close(L); is needed to do a pop() if you are going to close it? @@ +228,3 @@ + lua_pop(L, 1); + lua_close(L); + return NULL; You can reach this point with source as not NULL. I think you must free it here, or delay the source creation until everything is OK @@ +318,3 @@ + } + + g_free(L); shouldn't be a lua close()? @@ +340,3 @@ + /* Environment-only plugins */ + GRL_INFO("'%s' %s", ENV_LUA_SOURCES_PATH, + _("is setted - Getting lua-sources only from there.")); No need to log this as INFO. Let's use GRL_DEBUG and avoid the localized string @@ +341,3 @@ + GRL_INFO("'%s' %s", ENV_LUA_SOURCES_PATH, + _("is setted - Getting lua-sources only from there.")); + l_locations = g_list_prepend(l_locations, g_strdup(envvar)); Can you have several paths in the envvar, separated by ':'? If so, you should split the paths and put them in the list. @@ +401,3 @@ + && g_strcmp0(value, "true") + && grl_config_get_string(merged_configs, key) == NULL) { + return FALSE; Can you explain what is doing this conditional? @@ +438,3 @@ + GList *available_configs) +{ + GList *list_all = NULL; /* Lists of GrlConfigs */ Comments go before the code, not in the side @@ +519,3 @@ + /* Source ID */ + lua_getfield(L, -1, LUA_SOURCE_ID); + plugin_id = lua_tolstring(L, -1, NULL); Documentation says: "Because Lua has garbage collection, there is no guarantee that the pointer returned by lua_tolstring will be valid after the corresponding value is removed from the stack." Isn't it a risk of plugin_id becoming invalid after performing the pop()? @@ +543,3 @@ + *source_name = g_strdup(plugin_name); + *source_desc = g_strdup(plugin_desc); + See my comment above: Itthink this dup() should be done at each step, before doing the pop() @@ +585,3 @@ + lua_getglobal(L, LUA_SOURCE_TABLE); + if (!lua_istable(L, -1)) { + GRL_WARNING("'%s' %s", LUA_SOURCE_TABLE, _("table is not defined")); This message is most expected for developers. So far we are not translating such messages. Let's follow that rule for now. @@ +592,3 @@ + + /* Check if lua modules dependencies are installed */ + table_list = table_array_to_list(L, LUA_SOURCE_MODULES_DEPS); The function is about getting the list of defined keys. This piece of code is doing other task, checking if dependencies are available. Let's move it to a different function and call it before getting the list of defined keys. @@ +603,3 @@ + module_fail = TRUE; + GRL_INFO("%s %s", lua_module, + _("lua module is not installed")); Remind, let's not translate GRL_FOO messages. @@ +644,3 @@ + + /* Resolve keys - type, required and optional fields */ + lua_pushstring(L, LUA_SOURCE_RESOLVE_KEYS); what are these resolve keys? @@ +735,3 @@ + GrlLuaFactorySource *lua_source = GRL_LUA_FACTORY_SOURCE(source); + + return caps; Are you returning always 0? Is this because this patch doesn't implement any operation? If so, let's add a comment to clarify ::: src/lua-factory/grl-lua-factory.xml @@ +3,3 @@ + <name>Lua Source Factory</name> + <module>libgrlluafactory</module> + <description>A plugin that creates sources from lua plugins</description> Suggestion: capitalize Lua
Review of attachment 259236 [details] [review]: Some comments left. I'm really considering if it is worth at this point trying to push write operations, like store() or remove(). I'm feeling that maybe it could be better to just hold them on, and just go for now with the read-only operations: search(), browse(), resolve() and query(). ::: src/lua-factory/grl-lua-factory.c @@ +143,2 @@ /* Get the source and check its init function before registering */ + if (source && lua_plugin_source_init(source)) { What if lua_plugin_source_init() fails? Shouldn't the source be unreffed, as it seems we won't use it? @@ +820,3 @@ + caps |= (lua_source->priv->fn[LUA_QUERY]) ? GRL_OP_QUERY : caps; + caps |= (lua_source->priv->fn[LUA_RESOLVE]) ? GRL_OP_RESOLVE : caps; + caps |= (lua_source->priv->fn[LUA_STORE]) ? GRL_OP_STORE : caps; Actually, we have two capabilities for store: GRL_OP_STORE and GRL_OP_STORE_PARENT. Though the operation is the same (grl_source_store()), the difference is that with the former you can specify where to store. It would be a good idea for a source to declare exactly which kind of store supports. Maybe a property or similar. @@ +846,3 @@ + os->lot = LUA_SEARCH; + + text = (ss->text == NULL) ? "" : ss->text; In Grilo, a NULL search means "search everything". If the source doesn't support it, it can send back a GRL_CORE_ERROR_SEARCH_NULL_UNSUPPORTED. I understand that the case is still valid, though in Lua it would get an empty string rather than a NULL. @@ +867,3 @@ + lua_pushliteral(L, "operation_id"); + lua_pushinteger(L, os->operation_id); + lua_settable(L, -3); Why extracting these values from the options? Can't the Lua source do it? There could be other parameters interested for Lua source, like flags, or future options.
Review of attachment 259237 [details] [review]: ::: src/lua-factory/grl-lua-library.c @@ +162,3 @@ +* +* @media: (table) The media content to be returned. +* @count: (number) Number of media left to the application. Let's rename it to "remaining", like Grilo
Can you attach a functional Lua source for testing? Bonus point if it also uses the Lua library utility.
Hi Juan, Thanks for the review! (In reply to comment #5) > Review of attachment 258707 [details] [review]: > > ::: src/Makefile.am > @@ +56,3 @@ > > +if LUA_FACTORY_PLUGIN > +SUBDIRS += lua-factory > > You need to add also this directory in DIST_SUBDIRS, at the end of the file Fixed! > > ::: src/lua-factory/grl-lua-common.h > @@ +54,3 @@ > + GrlSource *source; > + guint operation_id; > + LuaOperationType lot; > > could you use a more descriptive name, like op_type? Yes, no problem. Fixed. > > @@ +64,3 @@ > + GrlMedia *media; > + GrlMedia *container; > + } content; > > Why this union, if both elements are of the same type? True, does not make sense. Fixed (using only media) > > ::: src/lua-factory/grl-lua-factory.c > @@ +26,3 @@ > +#include "lua.h" > +#include "lauxlib.h" > +#include "lualib.h" > > If they are system headers, use <>, not quotes Fixed. > > @@ +42,3 @@ > +#define LUA_FACTORY_PLUGIN_ID "grl-lua-factory" > +#define LUA_FACTORY_PLUGIN_NAME "Lua-Factory" > +#define LUA_FACTORY_PLUGIN_DESC _("A plugin to load lua based plugins") > > NAME and DESC definitions are not used anywhere Fixed (removed). > > @@ +80,3 @@ > +static GList *get_lua_sources(void); > + > +static GrlLuaFactorySource *grl_lua_factory_source_new(gchar *, GList *); > > Please, add the variable names in all these declarations. Sure, no problem. Fixed. > > @@ +122,3 @@ > + source = grl_lua_factory_source_new(it->data, configs); > + > + /* Get the source and check its init function before registering */ > > Is this comment outdated? it is not checking any "init" function Yes, It was a rebase problem. Fixed. > > @@ +172,3 @@ > + goto source_new_error; > + > + ret = lua_pcall(L, 0, 0, 0); > > Interesting.... which function is invoked here? And why we need to invoke it? I've done this way because of plenty examples I've passed by in the web and the info gathered in the Programming in Lua book (3th version), which says: "After creating a state and populating it with the standard libraries, it is time to interpret the user input. For each line the user enters, the program first calls luaL_loadstring to compile the code. If there are no errors, the call returns zero and pushes the resulting function on the stack" So lua_pcall in this particular case, don't acctually call a specific function but runs the piece of code loaded (from a file with luaL_loadfile rather than a string as it was in the example). We are only able to properly call the lua functions after running it. The example is a simple lua stand-alone interpreter: http://www.lua.org/pil/24.1.html > > @@ +227,3 @@ > +source_new_end: > + lua_pop(L, 1); > + lua_close(L); > > is needed to do a pop() if you are going to close it? There is no need at all. Fixed. > > @@ +228,3 @@ > + lua_pop(L, 1); > + lua_close(L); > + return NULL; > > You can reach this point with source as not NULL. I think you must free it > here, or delay the source creation until everything is OK True, thanks! Fixed. > > @@ +318,3 @@ > + } > + > + g_free(L); > > shouldn't be a lua close()? Yes. Fixed. > > @@ +340,3 @@ > + /* Environment-only plugins */ > + GRL_INFO("'%s' %s", ENV_LUA_SOURCES_PATH, > + _("is setted - Getting lua-sources only from there.")); > > No need to log this as INFO. Let's use GRL_DEBUG and avoid the localized string > Ok. Fixed. > @@ +341,3 @@ > + GRL_INFO("'%s' %s", ENV_LUA_SOURCES_PATH, > + _("is setted - Getting lua-sources only from there.")); > + l_locations = g_list_prepend(l_locations, g_strdup(envvar)); > > Can you have several paths in the envvar, separated by ':'? If so, you should > split the paths and put them in the list. > I didn't thought about it. It is working now with multiple paths separated by ':'. Thanks. > @@ +401,3 @@ > + && g_strcmp0(value, "true") > + && grl_config_get_string(merged_configs, key) == NULL) { > + return FALSE; > > Can you explain what is doing this conditional? > R: Checks If a *necessary* config from source is settled. key <- a source key (defined in the .lua) value <- (if it is mandatory to the source it is "true" otherwise it is "false") So, if the (key is not "type") and (is a mandatory config) and (is its value in *merged_configs* is NULL), return FALSE because we not all mandatory options has value. The logic is not good? Any suggestions? I've included a comment on it and also changed 'value' to 'is_mandatory' > @@ +438,3 @@ > + GList *available_configs) > +{ > + GList *list_all = NULL; /* Lists of GrlConfigs */ > > Comments go before the code, not in the side > Fixed. > @@ +519,3 @@ > + /* Source ID */ > + lua_getfield(L, -1, LUA_SOURCE_ID); > + plugin_id = lua_tolstring(L, -1, NULL); > > Documentation says: "Because Lua has garbage collection, there is no guarantee > that the pointer returned by lua_tolstring will be valid after the > corresponding value is removed from the stack." Isn't it a risk of plugin_id > becoming invalid after performing the pop()? > Yes, you are completly right. Fixed, only doing the pop after saving the fields. > @@ +543,3 @@ > + *source_name = g_strdup(plugin_name); > + *source_desc = g_strdup(plugin_desc); > + > > See my comment above: Itthink this dup() should be done at each step, before > doing the pop() > > @@ +585,3 @@ > + lua_getglobal(L, LUA_SOURCE_TABLE); > + if (!lua_istable(L, -1)) { > + GRL_WARNING("'%s' %s", LUA_SOURCE_TABLE, _("table is not defined")); > > This message is most expected for developers. So far we are not translating > such messages. Let's follow that rule for now. Fixed. Changed from _warning to _debug as it is for developers only message. > > @@ +592,3 @@ > + > + /* Check if lua modules dependencies are installed */ > + table_list = table_array_to_list(L, LUA_SOURCE_MODULES_DEPS); > > The function is about getting the list of defined keys. This piece of code is > doing other task, checking if dependencies are available. Let's move it to a > different function and call it before getting the list of defined keys. Fixed. Looks better, thanks! > > @@ +603,3 @@ > + module_fail = TRUE; > + GRL_INFO("%s %s", lua_module, > + _("lua module is not installed")); > > Remind, let's not translate GRL_FOO messages. > Are you sure about this one? The user must know that a lua-based source was not loadable due a lack of dependencies. Shouldn't this one be translated? > @@ +644,3 @@ > + > + /* Resolve keys - type, required and optional fields */ > + lua_pushstring(L, LUA_SOURCE_RESOLVE_KEYS); > > what are these resolve keys? The source may need keys to resolve an operation. The mandatory keys are the *necessary* keys to perform such operation. In the case of Metrolyrics, for instance, we need the Author and Title of the Music. Its possible to have 'optional' keys to better filter results. In the case of Metrolyrics, could be 'album' as a lyric may vary from album to album. The type, as we discussed before, would be the media type. For instance, Metrolyrics source.resolve_keys.type is 'audio'. lua-factory would only call lua-source resolve function if GRL_IS_MEDIA_AUDIO(media) is true > > @@ +735,3 @@ > + GrlLuaFactorySource *lua_source = GRL_LUA_FACTORY_SOURCE(source); > + > + return caps; > > Are you returning always 0? Is this because this patch doesn't implement any > operation? If so, let's add a comment to clarify Ok, Fixed. > > ::: src/lua-factory/grl-lua-factory.xml > @@ +3,3 @@ > + <name>Lua Source Factory</name> > + <module>libgrlluafactory</module> > + <description>A plugin that creates sources from lua plugins</description> > > Suggestion: capitalize Lua Accepted!
(In reply to comment #6) > Review of attachment 259236 [details] [review]: > > Some comments left. > > I'm really considering if it is worth at this point trying to push write > operations, like store() or remove(). I'm feeling that maybe it could be better > to just hold them on, and just go for now with the read-only operations: > search(), browse(), resolve() and query(). > It is also my preference... Mostly because I don't have it properly tested. I could improve the grl-flickr-lua source with a lua-library to make http posts and then testing the store and remove with web content. Anyhow, I've removed those operations from the patches and keeping them in a branch to be included in the future. > ::: src/lua-factory/grl-lua-factory.c > @@ +143,2 @@ > /* Get the source and check its init function before registering */ > + if (source && lua_plugin_source_init(source)) { > > What if lua_plugin_source_init() fails? Shouldn't the source be unreffed, as it > seems we won't use it? > Yes, it should. Fixed. > @@ +820,3 @@ > + caps |= (lua_source->priv->fn[LUA_QUERY]) ? GRL_OP_QUERY : caps; > + caps |= (lua_source->priv->fn[LUA_RESOLVE]) ? GRL_OP_RESOLVE : caps; > + caps |= (lua_source->priv->fn[LUA_STORE]) ? GRL_OP_STORE : caps; > > Actually, we have two capabilities for store: GRL_OP_STORE and > GRL_OP_STORE_PARENT. Though the operation is the same (grl_source_store()), the > difference is that with the former you can specify where to store. > > It would be a good idea for a source to declare exactly which kind of store > supports. Maybe a property or similar. Oh... Didn't know that. I see now the 'GrlMediaBox *parent' in the GrlSourceStoreSpec. Shouldn't we pass it to the source when it is available? > > @@ +846,3 @@ > + os->lot = LUA_SEARCH; > + > + text = (ss->text == NULL) ? "" : ss->text; > > In Grilo, a NULL search means "search everything". If the source doesn't > support it, it can send back a GRL_CORE_ERROR_SEARCH_NULL_UNSUPPORTED. Interesting I didn't know about this specific error when source does not support empty text. I see it with vimeo and shoutcast now. > > I understand that the case is still valid, though in Lua it would get an empty > string rather than a NULL. Yes, but in Lua nil is a default value. If you check parameter.text in the lua source, I've intentionaly put an empty string just to keep the lua type of it. Currently, the source does not deal with this error and would call the callback with no elements (grl.callback(nil, 0, userdata)) > > @@ +867,3 @@ > + lua_pushliteral(L, "operation_id"); > + lua_pushinteger(L, os->operation_id); > + lua_settable(L, -3); > > Why extracting these values from the options? Can't the Lua source do it? There Operation_id came from grilo, what do you mean about the Lua-source doing it? The operation id is not used in the source so I've removed them as parameter to the lua-function. The operation_id is only used when calling the callback > could be other parameters interested for Lua source, like flags, or future > options. Do you want any other parameters to be passed to Lua source?
The follow patches follow the fixes from Juan's review. I've also added two lua sources. Metrolyrics: From http://api.metrolyrics.com/v1/ to get lyrics. Flickr-lua: We can public search/browse and private search/browse. I've created two samples to teste both sources above [0] [0] github.com/victortoso/grl-lua-factory-tests
Created attachment 263783 [details] [review] Search, Load and Initialize Lua sources
Created attachment 263784 [details] [review] Grilo operations and the Lua code
Created attachment 263785 [details] [review] the library code
Created attachment 263786 [details] [review] grl-metrolyrics lua-source
Created attachment 263787 [details] [review] grl-flickr-lua lua source
(In reply to comment #9) > So, if the (key is not "type") and (is a mandatory config) and (is its value in > *merged_configs* is NULL), return FALSE because we not all mandatory options > has value. > > The logic is not good? Any suggestions? > I've included a comment on it and also changed 'value' to 'is_mandatory' It's fine enough. Thanks! > > > > @@ +603,3 @@ > > + module_fail = TRUE; > > + GRL_INFO("%s %s", lua_module, > > + _("lua module is not installed")); > > > > Remind, let's not translate GRL_FOO messages. > > > > Are you sure about this one? The user must know that a lua-based source was not > loadable due a lack of dependencies. Shouldn't this one be translated? > Well, probably that can be applied to any warning or critical messages too. The point is that we didn't do it in the other sources. And checking other programs, like glib or gtk+, seems this sort of debug messages are not translated (check for calls to g_warning() or g_critical()). Hence my proposal to keep such messages untranslated. > Its possible to have 'optional' keys to better filter results. In the case of > Metrolyrics, could be 'album' as a lyric may vary from album to album. > Notice that programmer never invoke the Lua plugin directly, but it invokes the resolve()operation in the source api, which reaches the Lua plugin. And with resolve(), programmer can ask which keys are needed in order to resolve a specific key. But there is no concept of "optional" keys, that is, keys that if you provide the value, the better, else, you can still solve the key. Such concept is only in the Lua plugin, but won't never reach the user and thus never be used at all. So it seems for me more a "documentation" stuff than real information to be used.
(In reply to comment #10) > (In reply to comment #6) > Oh... Didn't know that. I see now the 'GrlMediaBox *parent' in the > GrlSourceStoreSpec. > > Shouldn't we pass it to the source when it is available? > > Operation_id came from grilo, what do you mean about the Lua-source doing it? I mean that instead of passing the GrlOperationOption directly to the source and make it to get the information it needs from there, we are extracting part of the information and passing it to the source. So there is information that doesn't reach the source, like the flags. Is this information interesting for the source? Well, it depends on the source, of course. But if it is in the options passed to the source, we can say that in general could be interesting for the source. The other drawback is that if we extend it with new options in the future, we will need to extract them and pass it to the Lua source too.
BTW, check you are not adding trailing whitespaces in the code.
Another picky comment: can you add a space between the function name and parentheses in the function invokation? That is, instead of "foo(a,b)" use "foo (a, b)"
Review of attachment 263784 [details] [review]: ::: src/lua-factory/grl-lua-factory.c @@ +151,3 @@ + /* Get the source and check its init function before registering */ + if (source && lua_plugin_source_init(source)) { Uhm... I think lua_plugin_source_init() should be called inside source_new(). So grl_lua_factory_source_new() would return a source if its correctly initialized.
Review of attachment 263787 [details] [review]: ::: src/lua-factory/sources/grl-flickr-lua.lua @@ +230,3 @@ + return media +end + So this function creates a plain GrlMedia, not a GrlMediaAudio or GrlMediaVideo. I think in the case of Flickr the medias created should be of GrlMediaImage.
> Well, probably that can be applied to any warning or critical messages too. The > point is that we didn't do it in the other sources. And checking other > programs, like glib or gtk+, seems this sort of debug messages are not > translated (check for calls to g_warning() or g_critical()). Hence my proposal > to keep such messages untranslated. Okay then, no problem :) Fixed. > Such concept is only in the Lua plugin, but won't never reach the user and thus > never be used at all. > > So it seems for me more a "documentation" stuff than real information to be > used. Indeed. I believe that it could be removed without harm.
> I mean that instead of passing the GrlOperationOption directly to the source > and make it to get the information it needs from there, we are extracting part > of the information and passing it to the source. So there is information that > doesn't reach the source, like the flags. > > Is this information interesting for the source? Well, it depends on the source, > of course. But if it is in the options passed to the source, we can say that in > general could be interesting for the source. > > The other drawback is that if we extend it with new options in the future, we > will need to extract them and pass it to the Lua source too. I think I've understood now. So, the lua-source would get information from operation spec (like GrlSourceBrowseSpec) through the lua-library API, correct? Do you think that would be interesting to do the same to GrlMedia*? Instead of extracting author and title in the Metrolyrics example, we could let the lua-source do it. If it needs something else it would also be achievable.
(In reply to comment #19) > BTW, check you are not adding trailing whitespaces in the code. (In reply to comment #20) > Another picky comment: can you add a space between the function name and > parentheses in the function invokation? > > That is, instead of "foo(a,b)" use "foo (a, b)" No problem. With the help of indent I've added the space between the functions and it hopefully helped with the trailling whitespaces. I've also removed some of extra-spaces in #defines. (In reply to comment #21) > Uhm... I think lua_plugin_source_init() should be called inside source_new(). > So grl_lua_factory_source_new() would return a source if its correctly > initialized. Fixed. (In reply to comment #22) > So this function creates a plain GrlMedia, not a GrlMediaAudio or > GrlMediaVideo. I think in the case of Flickr the medias created should be of > GrlMediaImage. You are right. Actually I forgot to handle such types. I'm using the media.type = 'image' in lua to be translated as GrlMediaImage... the same apply to audio, video and box from now on.
(In reply to comment #19) > BTW, check you are not adding trailing whitespaces in the code. My Vim gets 'red' with spaces in the end of a line. I've removed extra-spaces in #define declarations. I did not find any after using indent... (In reply to comment #20) > Another picky comment: can you add a space between the function name and > parentheses in the function invokation? > > That is, instead of "foo(a,b)" use "foo (a, b)" No problem. (In reply to comment #21) > Uhm... I think lua_plugin_source_init() should be called inside source_new(). > So grl_lua_factory_source_new() would return a source if its correctly > initialized. Fixed. (In reply to comment #22) > So this function creates a plain GrlMedia, not a GrlMediaAudio or > GrlMediaVideo. I think in the case of Flickr the medias created should be of > GrlMediaImage. You are right. Actually I forgot to handle such types. I'm using the media.type = 'image' in lua to be translated as GrlMediaImage... the same apply to audio, video and box from now on.
* The metrolyrics api-key is currently invalid making it unsuitable for tests; * The flickr-lua source is working for both, public and personal browse; * Until the weekend I'll have a few more sources to test the plugin; > I mean that instead of passing the GrlOperationOption directly to the source > and make it to get the information it needs from there, we are extracting part > of the information and passing it to the source. So there is information that > doesn't reach the source, like the flags. > > Is this information interesting for the source? Well, it depends on the source, > of course. But if it is in the options passed to the source, we can say that in > general could be interesting for the source. > > The other drawback is that if we extend it with new options in the future, we > will need to extract them and pass it to the Lua source too. I've created the following functions in order to get data from lua-source: # Get count/skip/flags... grl.get_options() # All the GrlKeyID from OperationSpec grl.get_requested_keys() # All GrlKeyId with its value (currently, only for resolve operation) grl.get_media_keys() Am I getting closer to what you mean? Any thoughts about it? BTW, big thanks for all the feedback!
Created attachment 268755 [details] [review] Search, Load and Initialize Lua sources
Created attachment 268757 [details] [review] Grilo operations and the Lua code
Created attachment 268758 [details] [review] the library code
Created attachment 268759 [details] [review] grl-flickr-lua lua source
Created attachment 268873 [details] [review] grl-metrolyrics lua-source The metrolyrics source again. I've removed the api-request. Now I'm just fetching directly the URL which is always (...)/title-name-lyrics-artist-name.html
This is looking better. I still don't like the "userdata" being passed around though. Looking at grl_source_resolve(), it's actually an OperationSpec opaque object. Maybe passing "operation" around is better than userdata (which isn't actually userdata, it's implementation data). I'm not certain whether the lua_State is shared amongst function calls, but wouldn't it be possible to attach the OperationSpec to a specific call, and pop it back out in the lua-library calls? That way, the operation spec being passed around doesn't depend on the writer of the script. At least renaming it would be useful.
Review of attachment 268873 [details] [review]: ::: src/lua-factory/sources/grl-metrolyrics.lua @@ +31,3 @@ + supported_keys = { "lyrics" }, + config_keys = { + required = { "api-key" }, You said you don't need the API key anymore, but it's still marked as required here. @@ +59,3 @@ + + artist = string.gsub(req.artist, " ", "-") + title = string.gsub(req.title, " ", "-") That's not going to be enough for non-ascii names. For example: http://www.metrolyrics.com/hit-em-with-the-bernie-lyrics-mattmoney.html is for the "artist" called "$$MattMoney$$"
Review of attachment 268758 [details] [review]: ::: src/lua-factory/grl-lua-library.c @@ +249,3 @@ + +/** +* grl.get_media_keys The function says "operation_get_keys" below. @@ +289,3 @@ +*/ +static gint +grl_l_media_get_keys (lua_State *L) get_media_keys() or media_get_keys()? @@ +342,3 @@ +*/ +static gint +grl_l_fetch (lua_State *L) This needs a way to pass a user-agent, or set a nice default one if none is passed. Very few websites will like the default empty user-agent. ::: src/lua-factory/lua-library/lua-oauth.c @@ +25,3 @@ +#include <glib.h> +#include <stdlib.h> +#include <oauth.h> I'd personally rather we didn't do oauth inside lua sources. I think it would be better to have goa bindings, but I'll leave that up to Juan to say.
I tried to implement a metadata scraper for imdb and had a few remarks. One cannot set: media.external-url as '-' is a reserved character. media['external-url'] works though. It would be nice if media.external_url worked as well. We'll need a few HTML escaping and unescaping functions to do searches. quvi has some taken from lua tutorials. We'll need something to marshal keys that take GDateTime (I'd use the iso8601 format, passed through grl_date_time_from_iso8601()). And it's unclear how to set GRL_METADATA_KEY_PERFORMER or GRL_METADATA_KEY_KEYWORD that expect lists, or check whether the incoming media is a video/audio/image or a box (my lookup script tried to look up boxes). But pretty awesome overall!
Review of attachment 268758 [details] [review]: ::: src/lua-factory/grl-lua-library.c @@ +57,3 @@ + const gchar *media_type = lua_tostring (L, -1); + + if (g_strcmp0 (media_type, "box")) == 0!!!
(In reply to comment #33) > This is looking better. I still don't like the "userdata" being passed around > though. Looking at grl_source_resolve(), it's actually an OperationSpec opaque > object. Maybe passing "operation" around is better than userdata (which isn't > actually userdata, it's implementation data). Me neither. I was worried about the glue stuff and forgot to fix this. Now I'm using the lua_State environment table to hold this pointer. It's easy to access from anywhere with lua_State *. (In reply to comment #34) > Review of attachment 268873 [details] [review]: > > ::: src/lua-factory/sources/grl-metrolyrics.lua > @@ +31,3 @@ > + supported_keys = { "lyrics" }, > + config_keys = { > + required = { "api-key" }, > > You said you don't need the API key anymore, but it's still marked as required > here. Yes, my mistake. Thanks! > > @@ +59,3 @@ > + > + artist = string.gsub(req.artist, " ", "-") > + title = string.gsub(req.title, " ", "-") > > That's not going to be enough for non-ascii names. For example: > http://www.metrolyrics.com/hit-em-with-the-bernie-lyrics-mattmoney.html > is for the "artist" called "$$MattMoney$$" Fixed, thanks! (In reply to comment #35) > Review of attachment 268758 [details] [review]: > > ::: src/lua-factory/grl-lua-library.c > @@ +249,3 @@ > + > +/** > +* grl.get_media_keys > > The function says "operation_get_keys" below. > > @@ +289,3 @@ > +*/ > +static gint > +grl_l_media_get_keys (lua_State *L) > > get_media_keys() or media_get_keys()? media_get_keys is better, indeed. I'm not good at all with names... are those function names for lua-api good enough? * operation_get_options -> grl.get_options * operation_get_keys -> grl.get_requested_keys * media_get_keys -> grl.get_media_keys > > @@ +342,3 @@ > +*/ > +static gint > +grl_l_fetch (lua_State *L) > > This needs a way to pass a user-agent, or set a nice default one if none is > passed. Done! Now you can pass a table (optinal) in the fetch function. Metrolyrics have an example. > I'd personally rather we didn't do oauth inside lua sources. I think it would > be better to have goa bindings, but I'll leave that up to Juan to say. I'm using it to flickr-lua only, which is a proof-case.. so, its not really necessary. It is also possible to use others lua-libraries and lua-factory will check in run-time if that library is installed and accessible to the source... (In reply to comment #36) > I tried to implement a metadata scraper for imdb and had a few remarks. > > One cannot set: > media.external-url > as '-' is a reserved character. > media['external-url'] works though. It would be nice if media.external_url > worked as well. Fixed! the same applies to the config-table for GrlNetWc (e.g config.user_agent = '..') > > We'll need a few HTML escaping and unescaping functions to do searches. quvi > has some taken from lua tutorials. I still have to check that. > > We'll need something to marshal keys that take GDateTime (I'd use the iso8601 > format, passed through grl_date_time_from_iso8601()). Yes, I have to work on that too. GDateTime is a Boxed type and I'm not yet dealing with that. > > And it's unclear how to set GRL_METADATA_KEY_PERFORMER or > GRL_METADATA_KEY_KEYWORD that expect lists, or check whether the incoming media > is a video/audio/image or a box (my lookup script tried to look up boxes). I didn't understand. media.performer and media.keyword don't work? Both expect a string and should be set by generic build_media() > > But pretty awesome overall! Thanks!!! (In reply to comment #37) > Review of attachment 268758 [details] [review]: > > ::: src/lua-factory/grl-lua-library.c > @@ +57,3 @@ > + const gchar *media_type = lua_tostring (L, -1); > + > + if (g_strcmp0 (media_type, "box")) > > == 0!!! Ooops :) Fixed!
* Few bugs fixed; * Included GError message in log for json parser; * Removed the lightuserdata from lua (the OperationSpec pointer) * Now we are able to set GrlNetWc properties; * Can use '_' to represent '-' in keys with multiple words
Created attachment 269495 [details] [review] Search, Load and Initialize Lua sources
Created attachment 269496 [details] [review] Grilo operations and the Lua code
Created attachment 269497 [details] [review] the library code
Created attachment 269498 [details] [review] grl-flickr-lua lua source
Created attachment 269499 [details] [review] grl-metrolyrics lua-source
(I've forgot to mention that grl.callback() == grl.callback(nil, 0))
> (In reply to comment #35) > > Review of attachment 268758 [details] [review] [details]: > > > > ::: src/lua-factory/grl-lua-library.c > > @@ +249,3 @@ > > + > > +/** > > +* grl.get_media_keys > > > > The function says "operation_get_keys" below. > > > > @@ +289,3 @@ > > +*/ > > +static gint > > +grl_l_media_get_keys (lua_State *L) > > > > get_media_keys() or media_get_keys()? > > media_get_keys is better, indeed. > > I'm not good at all with names... are those function names for lua-api good > enough? > * operation_get_options -> grl.get_options > * operation_get_keys -> grl.get_requested_keys > * media_get_keys -> grl.get_media_keys Those look good. I was more concerned about the API comments not matching the function names. > > @@ +342,3 @@ > > +*/ > > +static gint > > +grl_l_fetch (lua_State *L) > > > > This needs a way to pass a user-agent, or set a nice default one if none is > > passed. > > Done! Now you can pass a table (optinal) in the fetch function. Metrolyrics > have an example. That's good, though "Grilo Source - Metrolyrics" isn't a valid user-agent. "Grilo Source Metrolyrics/0.2.8" would be. > > I'd personally rather we didn't do oauth inside lua sources. I think it would > > be better to have goa bindings, but I'll leave that up to Juan to say. > > I'm using it to flickr-lua only, which is a proof-case.. so, its not really > necessary. It is also possible to use others lua-libraries and lua-factory will > check in run-time if that library is installed and accessible to the source... I'd prefer if we didn't allow random libraries to be loaded. One of the interesting things about using lua is that it's sandboxed and allows us to use untrusted sources. So allowing local IO (for example) could be considered a potential privacy breach. > (In reply to comment #36) > > I tried to implement a metadata scraper for imdb and had a few remarks. > > > > One cannot set: > > media.external-url > > as '-' is a reserved character. > > media['external-url'] works though. It would be nice if media.external_url > > worked as well. > > Fixed! the same applies to the config-table for GrlNetWc (e.g config.user_agent > = '..') Great. > > We'll need a few HTML escaping and unescaping functions to do searches. quvi > > has some taken from lua tutorials. > > I still have to check that. http://repo.or.cz/w/libquvi-scripts.git/blob/refs/heads/next:/share/common/quvi/util.lua#l95 for example. I guess we can add those to grilo when we need them. > > We'll need something to marshal keys that take GDateTime (I'd use the iso8601 > > format, passed through grl_date_time_from_iso8601()). > > Yes, I have to work on that too. GDateTime is a Boxed type and I'm not yet > dealing with that. I think it should be passed from the source as a string, and you'd call grl_date_time_from_iso8601() to create the real GDateTime. Eg. no GDateTime in the lua source itself. > > And it's unclear how to set GRL_METADATA_KEY_PERFORMER or > > GRL_METADATA_KEY_KEYWORD that expect lists, or check whether the incoming media > > is a video/audio/image or a box (my lookup script tried to look up boxes). > > I didn't understand. media.performer and media.keyword don't work? Both expect > a string and should be set by generic build_media() You can set multiple keywords and performers. Grilo uses: grl_media_video_add_director() grl_media_video_add_performer() grl_media_video_add_producer() etc. Which don't overwrite the original value, but add to it. I think those should return tables/arrays/lists.
(In reply to comment #45) > (I've forgot to mention that grl.callback() == grl.callback(nil, 0)) That doesn't quite work to finish a list. Try the radiofrance lua source, in bug 722820, and you'll see that it adds one last empty item at the end of the browse list.
*** Bug 667557 has been marked as a duplicate of this bug. ***
(In reply to comment #47) > (In reply to comment #45) > > (I've forgot to mention that grl.callback() == grl.callback(nil, 0)) > > That doesn't quite work to finish a list. Try the radiofrance lua source, in > bug 722820, and you'll see that it adds one last empty item at the end of the > browse list. Thanks, fixed! > That's good, though "Grilo Source - Metrolyrics" isn't a valid user-agent. > "Grilo Source Metrolyrics/0.2.8" would be. Fixed. > http://repo.or.cz/w/libquvi-scripts.git/blob/refs/heads/next:/share/common/quvi/util.lua#l95 > for example. > > I guess we can add those to grilo when we need them. Okay. > > > We'll need something to marshal keys that take GDateTime (I'd use the iso8601 > > > format, passed through grl_date_time_from_iso8601()). > > > > Yes, I have to work on that too. GDateTime is a Boxed type and I'm not yet > > dealing with that. > > I think it should be passed from the source as a string, and you'd call > grl_date_time_from_iso8601() to create the real GDateTime. Eg. no GDateTime in > the lua source itself. That was included. Any G_TYPE_DATE_TIME will be accepted as string and a GDateTime will be created with grl_date_time_from_iso8601. The same apply to grl.get_media_keys(), the GDateTime key will be a string in lua created with g_date_time_format (date, "%F %T") which is iso8601 standard. > You can set multiple keywords and performers. Grilo uses: > grl_media_video_add_director() > grl_media_video_add_performer() > grl_media_video_add_producer() > etc. > > Which don't overwrite the original value, but add to it. I think those should > return tables/arrays/lists. That is really interesting in Grilo which made me have a few doubts about GrlRelatedKeys. Anyway, lua-library accepts arrays/list as table to *add* value to the same key.. which, I think, it does cover most of its use in search/browse/query operations. For instance: media.director = { "Joel Coen", "Ethan Coen" } But for resolve operation it's not complete because grl.get_media_keys() is not returning tables to lua-source yet. I'm having some troubles with grl_data_get* API.
* Fixed bug in grl.callback() * Handling G_TYPE_DATE_TIME and G_TYPE_BYTE_ARRAY values * grl.get_media_keys() replace '-' by '_' in key's name by default. ** Handling 'supported_media' and 'source-icon' of Lua-sources. * Improved comments in commits * Handling multi-value (array) per key from lua. For future documentation: Currently, We are always 'setting' the media-table content in GrlMedia which removes any previous content. In a resolve operation, the source that wants to *add* content to an existing key, should get original value(s) using grl.get_media_keys and include its values to them before calling grl.callback()
Forgot to mention: I removed the lua-oauth library from patches. It is easy to include if necessary, but no source is using it right now.
Created attachment 270028 [details] [review] Search, Load and Initialize Lua sources
Created attachment 270029 [details] [review] Grilo operations and the Lua code
Created attachment 270030 [details] [review] the library code
Created attachment 270031 [details] [review] grl-metrolyrics lua-source
Bastien, Victor, amazing work you have done so far!
About restricting the libraries allowed in Lua sources, I think this should be an option to be added in the future. The point is that we do not restrict the kind of sources we allow in Grilo by default. The Lua plugin really simplifies creating new sources. If one can write a native C plugin that does local IO, why not allowing the same for the Lua plugin? But, I also see that restricting and sandboxing what a source can do is really an interesting feature. In this sense, I envision such restriction by the manufacturer only allowing the Lua Factory plugin, no other native plugin, and configuring the factory to restrict the allowed libraries to a predefined set. So anyone creating plugins for such device will do using Lua as the single language, and also knowing what are the supported libraries. Manufacturer could extend this set in future if they think it is interesting and safe. So wrapping up: restricting the libraries is a feature we should allow, but not make mandatory by default.
Bastien, Victor, what about merging this issue in upstream? Do we have anything urgent to do before merging it? We can file new bugs for anything missing (like the documentation)
I can do a pass at a full review tomorrow. (In reply to comment #57) > About restricting the libraries allowed in Lua sources, I think this should be > an option to be added in the future. I'd rather do the reverse. I've coded a few sources already (as you've seen), and I've not needed any external libraries, bar one unescaping one-liner. If we don't restrict loading, we run the risk of introducing security problems (PyGTK plugins had one like that, if you managed to make somebody download a broken python file into their home directory, the plugin would load the "library" from the home dir instead of the system-wide paths). I think it's also easier for us in terms of controlling the API, avoiding problems after installation when a random lua library is required (especially if it's a third-party one that's not packaged). We can certainly implement that after merging those patches though.
(In reply to comment #59) > I can do a pass at a full review tomorrow. > Great. > (In reply to comment #57) > > About restricting the libraries allowed in Lua sources, I think this should be > > an option to be added in the future. > > I'd rather do the reverse. I've coded a few sources already (as you've seen), > and I've not needed any external libraries, bar one unescaping one-liner. > > If we don't restrict loading, we run the risk of introducing security problems > (PyGTK plugins had one like that, if you managed to make somebody download a > broken python file into their home directory, the plugin would load the > "library" from the home dir instead of the system-wide paths). > My main point is that we are not doing such restriction on the native plugins, so why we want to do it by default in Lua? Anyone could write the plugin natively to skip those restrictions in Lua. For me, the reason to allow Lua is not for sandboxing/restricting (at least, not the main reason) but to have an easy way of creating new plugins. Maybe we can do some sanity-checks to avoid problems without adding restrictions, dunno. And it doesn't mean at all I'm against such restrictions. Just that we should have a way to activate/deactivate those restrictions. > I think it's also easier for us in terms of controlling the API, avoiding > problems after installation when a random lua library is required (especially > if it's a third-party one that's not packaged). > I don't think it will be a problem. That's precisely the reason why the init() function was introduced: to verify that everything you need is there and available. This init() function can check that your system contains the expected libraries, and even they work as they should (if you provide the proper tests there). > We can certainly implement that after merging those patches though. Definitely. Either default is restricting or not we can do it later.
(In reply to comment #60) > My main point is that we are not doing such restriction on the native plugins, > so why we want to do it by default in Lua? Because we want to control the API. Restricting it later means breaking those sources. <snip> > > I think it's also easier for us in terms of controlling the API, avoiding > > problems after installation when a random lua library is required (especially > > if it's a third-party one that's not packaged). > > > > I don't think it will be a problem. That's precisely the reason why the init() > function was introduced: to verify that everything you need is there and > available. This init() function can check that your system contains the > expected libraries, and even they work as they should (if you provide the > proper tests there). That would mean broken scripts get installed, and failure only happens at runtime. If we restrict the usable API, we can be sure that either the plugin is self-contained, or relies on lua libraries that we do support, and that it will all work correctly after the installation.
Review of attachment 270028 [details] [review]: Prefix your commit messages with "lua-factory: " ::: src/Makefile.am @@ +52,3 @@ +if LUA_FACTORY_PLUGIN +SUBDIRS += lua-factory Trailing white-space here @@ +114,3 @@ apple-trailers bliptv bookmarks dmap filesystem flickr freebox gravatar guardian-videos jamendo \ lastfm-albumart local-metadata magnatune metadata-store optical-media \ + pocket podcasts raitv shoutcast tmdb tracker upnp vimeo youtube lua-factory This is supposed to be in alphabetical order. ::: src/lua-factory/Makefile.am @@ +10,3 @@ +ext_LTLIBRARIES = libgrlluafactory.la + +libgrlluafactory_la_CFLAGS = \ Do the backslashes need to be this far? @@ +12,3 @@ +libgrlluafactory_la_CFLAGS = \ + $(DEPS_LUA_FACTORY_CFLAGS) \ + -DLUA_FACTORY_SOURCE_LOCATION=\"@LUA_FACTORY_SOURCE_LOCATION@\" Indentation. @@ +14,3 @@ + -DLUA_FACTORY_SOURCE_LOCATION=\"@LUA_FACTORY_SOURCE_LOCATION@\" + +libgrlluafactory_la_LIBADD = \ One line is fine here. @@ +23,3 @@ + +libgrlluafactory_la_SOURCES = \ + grl-lua-factory.c \ Indentation. @@ +30,3 @@ + lua-library/lua-libraries.h + +extdir = $(GRL_PLUGINS_DIR) Indentation again. @@ +34,3 @@ +luafactoryxml_DATA = $(LUA_FACTORY_PLUGIN_ID).xml + +EXTRA_DIST = \ One line. @@ +39,3 @@ +DIST_SUBDIRS = sources + +MAINTAINERCLEANFILES = \ Ditto. ::: src/lua-factory/grl-lua-factory.c @@ +75,3 @@ +}; + +static GList *get_lua_sources (void); Could we avoid all those forward declarations by moving functions around? @@ +124,3 @@ + GList *it = NULL; + GList *lua_sources = NULL; + GrlLuaFactorySource *source = NULL; Move source inside the loop block where it's used. @@ +132,3 @@ + GRL_DEBUG ("grl_lua_factory_plugin_init"); + + lua_sources = get_lua_sources (); I'd do a "if lua_sources == NULL" return TRUE; It's not an error to not have any lua sources installed. @@ +198,3 @@ + if (ret != LUA_OK) { + GRL_DEBUG ("First call failed: '%s'", lua_tolstring (L, -1, NULL)); + goto source_new_error; This statement is only used twice, we can probably avoid using a goto if we don't have to. @@ +299,3 @@ + +static void +lua_factory_source_free (GrlLuaFactorySource *lua_source) Merge it into finalize. @@ +301,3 @@ +lua_factory_source_free (GrlLuaFactorySource *lua_source) +{ + if (lua_source == NULL) That wouldn't be needed any more in finalize. @@ +376,3 @@ + GDir *dir = NULL; + gint i = 0; + gchar **local_dir = NULL; Again, variable should be inside the block where they're used. I would also make it plural. @@ +377,3 @@ + gint i = 0; + gchar **local_dir = NULL; + const gchar *const *system_dir = NULL; Ditto. @@ +389,3 @@ + GRL_DEBUG ("'%s' %s", ENV_LUA_SOURCES_PATH, + "is setted - Getting lua-sources only from there."); + local_dir = g_strsplit (envvar, ":", -1); Not ":" but G_SEARCHPATH_SEPARATOR_S. @@ +423,3 @@ + it_file; + it_file = g_dir_read_name (dir)) { + if (g_str_has_suffix (it_file, ".lua")) { For later, load ".luac" files as well. @@ +505,3 @@ + GList *options = NULL; + GrlConfig *merged_config = NULL; + gchar *config_source_id = NULL; Into the block. @@ +703,3 @@ + table_list = table_array_to_list (L, LUA_SOURCE_SUPPORTED_KEYS); + if (table_list != NULL) { + for (it = table_list; it; it = g_list_next (it)) { This section, iterating over all the keys should be moved in a separate function. ::: src/lua-factory/grl-lua-factory.h @@ +26,3 @@ +#include <grilo.h> + +#define GRL_LUA_FACTORY_SOURCE_TYPE \ All one line is fine for those.
Review of attachment 270029 [details] [review]: ::: src/lua-factory/grl-lua-factory.c @@ +373,3 @@ + + if (lua_pcall (L, 1, 1, 0)) { + GRL_WARNING ("calling grl_source_init function fail: %s", failed. @@ +1082,3 @@ + if (it_key_id != GRL_METADATA_KEY_INVALID + && grl_data_has_key (GRL_DATA (media), it_key_id) == FALSE) { + missing = g_list_prepend (missing, GRLKEYID_TO_POINTER (it_key_id)); This looks like similar code to the one I asked to factor out into a function.
Review of attachment 270030 [details] [review]: ::: configure.ac @@ +847,3 @@ + AC_MSG_ERROR([json-glib-1.0 not found, install it or use --disable-lua-factory]) + fi + if test "x$HAVE_OAUTH" = "xno"; then That should be gone, as it's not in the lua libraries anymore. ::: src/lua-factory/grl-lua-library.c @@ +135,3 @@ + if (lua_isnumber (L, -1)) { + grl_data_set_int (GRL_DATA (media), key_id, lua_tointeger (L, -1)); + line feeds aren't necessary. @@ +466,3 @@ + const gchar *key = lua_tostring (L, -2); + if (g_strcmp0 (key, "user-agent") == 0 + || g_strcmp0 (key, "user_agent") == 0) { We usually put the operators on the preceding line. @@ +497,3 @@ + fo->lua_cb = g_strdup (lua_callback); + + grl_net_wc_request_async (wc, url, NULL, grl_util_fetch_done, fo); You'll be leaking the wc here, so unref it, it will stay alive until the end of the callback function.
Review of attachment 270031 [details] [review]: ::: src/lua-factory/sources/grl-metrolyrics.lua @@ +58,3 @@ + if not req or not req.artist or not req.title then + grl.callback() + end if req.lyrics then return end?
Created attachment 270127 [details] [review] lua-factory: Load and initialise Lua sources - Find lua sources in the install path or in the directories specified by envvars - Run source a first time to get the source metadata provided by the script's 'source' table - Check if necessary API keys were provided by application, if applicable
Created attachment 270128 [details] [review] lua-factory: Grilo operations to Lua sources This is the glue code between Grilo and the Lua sources. The Lua stack (lua_State) is used to pass the variables to the lua code. Besides the Grilo operations, the lua source can use the grl_source_init function to check if it can be successfully loaded or not.
Created attachment 270129 [details] [review] lua-factory: Lua libraries for sources Include libraries for lua sources to use. Core library: - grl.callback: operation to return the content for user application; - grl.fetch: A fetch operation that uses GrlNet to fetch web content; - grl.get_options: Get options provided by application; - grl.get_requested_keys: List of all requested keys; - grl.get_media_keys: Current values of media, mainly for resolve op; Json library: - grl.json.string_to_table: parser using json-glib that gets a json object as string and return its equivalent in a table;
Created attachment 270130 [details] [review] metrolyrics: Looks up lyrics with metrolyrics.com If media has Artist and Title keys, look up lyrics on metrolyrics.com.
Created attachment 270151 [details] [review] lua-factory: Lua libraries for sources Include libraries for lua sources to use. Core library: - grl.callback: operation to return the content for user application; - grl.fetch: A fetch operation that uses GrlNet to fetch web content; - grl.get_options: Get options provided by application; - grl.get_requested_keys: List of all requested keys; - grl.get_media_keys: Current values of media, mainly for resolve op; Json library: - grl.json.string_to_table: parser using json-glib that gets a json object as string and return its equivalent in a table;
Created attachment 270181 [details] [review] lua-factory: Fix crasher on startup When the front-end deletes the source during registration, we need to make sure that we don't consider the source to be registered successfully. See also commit 5973947051d6ca789965c92c5c33cf66cadd3f25 which did this for the dmap, and podcasts plugins.
(In reply to comment #61) > Because we want to control the API. Restricting it later means breaking those > sources. Well, by default I don't want to control the API, except the API related with Grilo. And as I don't know what kind of restrictions we want to add in the future (I think it really depends on the manufacturer and who is deploying Grilo), I want to leave to them what that decision. > That would mean broken scripts get installed, and failure only happens at > runtime. If we restrict the usable API, we can be sure that either the plugin > is self-contained, or relies on lua libraries that we do support, and that it > will all work correctly after the installation. The purpose of the init() function is precisely to detect if the script is broken or not, and if so, do not use it. In any case, I can also see the potential benefits of restricting. Could be we can follow a mixed approach: define a set of allowed (and supported) libraries that plugins can use, but allow them to declare if they want to use other libraries. If so, *and* our application configures the plugin to accept such kind of sources, then we can accept them. This mixed approach allows, on one side, to use plugins that for some reason need some weird library that we do not officially support, giving the application accept such kind of "unrestricted" sources. And on the other side, we encourage people to attach to our predefined set if they want to have plugins that can be used in any application. But if this is not possible, at least there's a way of using them, under those conditions (applications accepting such plugins and the required libraries installed in the system). Also, whatever approach we follow, the set of accepted/restricted libraries shouldn't be hardcoded, but be configurable (not sure if by applications or in a global manner). Because depending on the final system, the supported options can be different (maybe in some specific case they allow to write in disk through a provided restricted Lua library, while in others it doesn't). Of course, that means that plugins that works in a system could not work in another, but that's acceptable, IMHO. I think if we define a good set of libraries, likely other systems will also use this set. But at least we have in account those that not.
Created attachment 270200 [details] [review] lua-factory: Load and initialise Lua sources - Find lua sources in the install path or in the directories specified by envvars - Run source a first time to get the source metadata provided by the script's 'source' table - Check if necessary API keys were provided by application, if applicable
Attachment 270128 [details] pushed as f5f335c - lua-factory: Grilo operations to Lua sources Attachment 270130 [details] pushed as 65d5f17 - metrolyrics: Looks up lyrics with metrolyrics.com Attachment 270151 [details] pushed as 20c60d1 - lua-factory: Lua libraries for sources Attachment 270181 [details] pushed as b7b59ff - lua-factory: Fix crasher on startup Attachment 270200 [details] pushed as b06acd2 - lua-factory: Load and initialise Lua sources