[vlc-devel] [PATCH] GSoC: lua services discovery, take two

[Fabio Ritrovato]

>> You put an item inside as a child to the service discovery, like
>> services_discovery_AddItem, but what you pass is a string that has to
>> be elaborated somehow by the SD
>
> Ok. What about frontEndRequestsAddItem(input_item_t*) and
> frontEndRequestsAddItemAsSearchString(const char *)?
>
> We should probably iterate a bit on that, what would you expect from
> that string being? If that's a path, then it seems right to use an
> input_item instead. Else, I am not sure.
>
> And the FrontEnd prefix is because we are indeed using the API from
> two side. I don't have a better proposal for now.

It depends on the module, in the SD one, it needs a file path, but the
resulting item, points to the file path on the device, not the
original path...
So using an input item just to pass a path seems kind of a waste to me...


>> This happens when you click "save" on an SD item, what happens is what
>> the writer of the modules wants to...
>> For example, in the MTP SD, it's used to transfer files from the
>> device to you hard disk...
>
> Ok. What about frontEndRequestsSaveItem()? Save is probably vague, and
> the interface would expect an unified experience among service
> discovery. Is SetItemPersistence() more appropriate?

I think it can be hard to have an unified experience, as one can write
the module to do anything...
Also, I'm not sure i get what you mean with SetItemPersistence()...

>> It's about what should happen when you delete the item from the
>> playlist; usually, nothing is enough...
>> In the MTP SD, when you delete an item , it gets deleted from the device too...
>
> Given that this can fail, what do you think about:
> services_discovery_FrontEndRequestsDeleteItem();

Ok...
But why this one with the services_discovery prefix? Just a typo?

>> It's a list, so you are appending the new parameter to that list, you
>> are not setting a value...
>
> Ok. Should we support remove then. Would the user want to do that?

No, the user does not interact with it, it's read-only for him...

>> The format is there to be more generic, it could just be a string, but
>> J-Peg agreed with me to leave format, just in case someone needs it,
>> but it's not a problem to remove it...
>
> This could probably simplify the implementation. It's easy to do
> format with lua. (To be honest I don't think that we need
> "key=\"value\"" as this is just serializing { key, value } to a
> string, more below).

It's easy in lua, but this can be used in C too...

>> I can agree that maybe naming can be rethought, but I'll try to
>> explain better what they do...
>> What you build using that function is a list, or better, a tree, of
>> all the available parameters and all of their possible values for the
>> SD, I'll give you an example:
>> - parameter: "Categories"
>> -- name: "Gaming"     value: "category=\"Games\""
>> -- name: "Music"         value: "category=\"Music\""
>> ....
>> - parameter: "Category order"
>> -- name: "Top rated"         value: "category_order=\"top_rated\""
>> -- name: "Top favorites"   value: "category_order=\"top_favorites\""
>> ....
>
> Thanks, that's helpful :)
>
> So this is a tree with a depth of 1. So far I see:
> - category type (category_order) and its pretty name ("Category order")
> - category (top_rated) and its pretty name ("Top rated")
> - It seems that from category we could infer, category type and the
> pretty names.
>
> Is it more complex that this?

Yes, that was just a coincidence...
For example, from the jamendo script:

{parameter="Albums",name="Most populars",value="album=\"rating_desc\""}
{parameter="Radio",name="Rock",value="radio=\"9\""}

Also category_type and its pretty name can be different, I'm working
on a module that uses them that way...

Maybe it can be made so you can derivate one from the other, but that
will require a lot of work on the script side, to derivate again, an
useful value...
Right now, the value is something that needs no work, it's the value
that, for example, the youtube api needs for a query...

> Here, I think we'd better want the bridge pretty-name <-> category and
> category <-> category type to be actually implemented by
> services_discovery, if those are part of the services_discovery API.
>
> So far this would make this function:
> frontEndRequestsSetCategoryVisible(const char * category, bool visible);

I don't get why one would want an invisible category, if it's needed,
it has to be visible, if it's not needed, no point in adding it and
make it invisible...

> Well, it seems that we'll have also to iterate on this again with your feedback.
>
> I am also not sure if this is module specific. It seems that this is
> completely module specific, because some module may support the query
> category_order="top_rated", some may not. So how do you actually know
> if the module supports that kind of things? It could probably be
> valuable to have that kind of functionality. How is the user supposed
> to interact with those services?

Yes, very module is different and can support anything the coder wants to...
Even query that does basically the same thing, can be radically
different, since it's based on how the service API works...
For example, the jamendo equivalend of top_rated is
album=\"starred_desc\", or order=\"rated\" in dailymotion...

>> An accessor to what?
>> parameter_t *services_discovery_GetParameter(...)?
>> Because the interface needs the whole tree, not just a single value...
>
> This deserves an accessor even though that's a tree. At least so that
> we could ensure that client properly locks this structure. So instead
> of directly accessing the structure, the SD should just do
> GetVisibleCategoriesCount(), GetCategoryAtIndex(), and
> CategoryGetPrettyName(), GetCategoryType(),
> CategoryTypeGetPrettyName(). (This is if
> frontEndRequestsSetCategoryVisible() is enough).
>
> I hope this makes sense.

Actually, there was a lock, but courmisch told me to remove it, as it
was not needed...
But if it is, I can put it in again...
Also, those function would have the services_discovery_ prefix, right?

Also, don't worry, this is not a pain, as we are really getting something...
Thanks for your time to...