Re: lua-factory: the analysis of the changes proposed in #753141

[Victor Toso <lists victortoso com>]

Hi,

On Fri, Oct 30, 2015 at 01:35:53AM +0300, George Sedov wrote:
> > 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
>
> Each operation spec stays with its own operation-id as a key, yes, but
> ALSO one is stored as "current" as you can see in
> grl_lua_library_set_current_operation(). Here's the piece of code
> responsible for it:
>
> 1686  lua_getglobal (L, LUA_ENV_TABLE);
> 1687  lua_pushstring (L, GRILO_LUA_OPERATION_INDEX);
> 1688  lua_pushlightuserdata (L, os);
> 1689  lua_settable (L, -3);
>
> You can see that GRILO_LUA_OPERATION_INDEX is quite global.
>
> As for the test, it's not that easy. We are talking race conditions
> here, so an automated test would be a challenge. And a pointless one at
> that, since you agree that moving away from global table is ultimately
> a good idea anyways :)

I just fail to see how as this is a single process under glib mainloop
so that's why I asked you how. It might be the case of multiple async
calls from lua source, like, grl.fetch() grl.fetch() - but that
would be a broken plugin IMO.

But yes, I agree that it is a great improvement getting the globals
out. The globals were a workaround and this is perfect time to fix it.

> > 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.
>
> > aside of `callback` which is the only one I think it makes sense to
> > keep as an argument here.
>
> Why is callback so different from options in your opinion? They are
> both context-dependent in the very same way. They both do not make any
> sense outside of their respective scopes for which they were created.
> And they both do not belong to the globally accessible library, IMO.

Callback makes more sense to me to have as parameters then the options
as one could simply say that callback per se is not part of Grl library
- Plugin developer should keep reference to the callback (as userdata
  on async functions) in order to finish a operation later on

As functions, get_media_keys or get_requested_keys should always be
reacheable when a operation is ongoing IMHO.
After three or four fetch() the user might want to get those keys to
only write the keys that were requested from application or only
write the keys that has no value yet. Passing a table of utility
functions around for that does not seem right to me.

I might prefer to have an actual GrlMedia as parameter and maybe the
operations options as argument (as table) as well. I think this would
be enough to remove context-dependend functions -- aside of callback.

<snip>
grl_source_search (query, media, options, callback)
  local userdata = {}
  if options.count < 5 then
    grl.callback("fail!")
    callback()
  end
  -- save what you want here
  userdata.media = media
  userdata.options = options.req_keys
  userdata.callback = callback
  grl.fetch("something", fetch_done, userdata)
</snip>

What do you think?

This looks nicer to me as I'm used to the C side of grilo API, so this
way is closer to it -- does not mean it is the right way.

Regards,
  Victor Toso