Hi, First of all, thanks for your time and work on this. Comments bellow.
On Wed, Oct 21, 2015 at 06:26:19PM +0300, George Sedov wrote: > Hi, > > So there was some confusion regarding the bug #753141, and what was the > ideas behind the proposed changes. Let me explain the situation. > > Right now, the lua plugins are working this way: > > 1. When the plugin is asked to perform the operation, lua-factory > constructs the structure named OperationSpec (OS), in where it puts all > the relevant information about the operation, most of it taken directly > from the GrlSourceSearchSpec, BrowseSpec and such. > > 2. The constructed OS is then put into the lua context, into the global > table "_G" with the unique key, constructed from the operation_id (see > https://git.gnome.org/browse/grilo-plugins/tree/src/lua-factory/grl-lua > -library.c#n1584) > > 3. After putting the OS, lua-factory also puts the same data into the > same global table, only this time with the key, which is the same for > all the operations, and with that, that operation is considered > "current" (see https://git.gnome.org/browse/grilo-plugins/tree/src/lua- > factory/grl-lua-library.c#n1658). This function also does the check for > the "unfinalized operations" which I will explain later. > > 3. As part of it's lua API, lua-factory provides several functions that > use the OS data. Most of them are synchronous and just take the OS > which is "current", but there are two of them that require callbacks, > namely grl.fetch and grl.unzip. They save the operation_id of the > "current" operation internally, and use it later in the callback to > retrieve the relevant OS from the global table. They retrieve it by > making their operation "current" again, and after they are done, they > invalidate the "current" operation completely. These function also > increase the special counter of unfinished async calls, to keep the > track of when every particular operation is really "done". > > 4. Now about this "unfinalized operation" check. The idea behind it is, > that by the time the next operation starts, all the synchronous calls > from the previous operation have already been finished. So every OS > that has the counter of unfinished async calls = 0 should be > "finalized", i.e. the finalizing grilo callback must be called. If it's > not, it is called forcibly with the warning of "broken plugin" being > made. > > Apart from the obvious over-complication, this approach has several > flaws. > > 1. The foremost: this approach will totally not work in case of several > operations being run simultaneously. There can be only one "current" > operation, and so if the previous operation didn't finish it's > synchronous part by the time the next operation started, it is > finalized forcibly anyway. Another screw-up will happen if several lua > async callback would be run simultaneously. They will rewrite the > "current" operation and reset it when the first of them would be done. > All in all, the design is absolutely async-unfriendly. All in all, I agree with you. It is over-complicated but I don't see how it could break with several operations from grilo (like, several async search/browse) as each OperationSpec stays in _G related to its operation-id. If you *can* break it easily, tell me how or even better, create a test under lua-factory [1] and attach to the BZ :-) [1] https://git.gnome.org/browse/grilo-plugins/tree/tests/lua-factory Also, what I think is "async-unfriendly" is the "string" as callback without any argument as user_data. We workaround this with a global table in the plugin which make it ugly IMHO. Bad design. > 2. The async methods like grl.fetch can't work without OS, even though > they don't require them directly. So you can't fetch anything during > e.g. grl_source_init() Agreed. Bad design. > 3. The methods that do require OS on the other hand, like > grl.get_options, can be called from everywhere, even though it does > nothing and doesn't make any sense outside of the scope of the > respective operation. > So, the idea is provide all functions under 'grl' table. This is not strange at all and keep everything very simple to the developer. If a function does not make sense to be called then it should not be called - It does not mean that we must control when developer use it. grl.get_options was indeed created thinking that would be called inside an operation handler - thus it would fail otherwise as it would not be able to get OperationSpec. /* broken plugin */ > And now, after we refreshed the current situation, the changes proposed > in the patches from #753141 > > 1. Remove the OS from the global tables, as well as the concept of the > "current" operation. I'm okay with this. > 2. Similarly, remove all the functions that require OS directly from > the global scope. There are actually only four of them: grl.get_options > grl.get_requested_keys grl.get_media_keys and grl.callback. Right. > 3. Now comes the tricky part. To be able to access the OS from inside > these functions, the C closures are constructed for every such > functions with OS as an upvalue, and these closures are then passed to > the lua functions as parameters. From the lua code this will look like > this: > > was: > > function grl_source_search (query) > local count = grl.get_options("count") > grl.callback() > end > > become: > > function grl_source_search (query, options, callback) > local count = options("count") > callback() > end > So, this changes a lot. What I like least is that you are splitting functions provided by grl-lua-library. I do like grl as a library - globally accessible. I might need some studying to closures to realize why this would be a bad idea but why not having the whole grl table as closure then? Also: would it be worse if we set `grl.get_options = options` before calling the operation handler? even if get_options and friends are not accessible afterwards due gc cleanup, that might be better as we keep functions centralized. ** aside of `callback` which is the only one I think it makes sense to keep as an argument here. > 4. Since all the methods that require the access to OS should be passed > to the lua functions as parameters, it is best to make the number of > these methods as few as possible. That's why the methods > grl.get_options grl.get_requested_keys and grl.get_media_keys were > merged into one. otherwise the function would look something like > > function grl_source_search (query, options, requested_keys, media_keys, > callback) > > 5. As a final step, the way the lua async functions are working was > changed. > > was: > > function async_callback (data) > end > > function grl_source_search (query) > grl.fetch("something","async_callback") > end > > become: > > function async_callback (data) > end > > function grl_source_search (query) > > grl.fetch("something",async_callback) > end > > And if it doesn't strike you as a big difference, then consider these > possibilities: > Yes, this is much better indeed. > grl.fetch("something",function(data) print data end) > > or > > local userdata = {} > grl.fetch("something",function(data) async_callback(data, userdata) end) > > There is also provided an option of passing the userdata directly > > function async_callback (data, userdata) > end > > function grl_source_search (query) > grl.fetch("something",async_callback, {}, userdata) > end > > The extra {} is required as an option for grl.fetch > It is optional, not really required. > So, these are the changes that are proposed in bug #753141. As you can > see, they are quite API breaking, no way around it, but I think the > benefits are worth it. > > Cheers, > George I have looked into the patches and vk source while reading/replying this email. I hope to do a proper review soon. toso _______________________________________________ grilo-list mailing list grilo-list@gnome.org https://mail.gnome.org/mailman/listinfo/grilo-list
