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

[Pierre d'Herbemont]

On Mon, Jul 27, 2009 at 3:29 AM, Fabio Ritrovato<exsephiroth87 at gmail.com> wrote:
>> So, just remove the id part, and pass a playlist_item_t pointer. The
>> idea is that you should not use id, especially in a API. This should
>> not be hard to do.
>
> It can be done, but to do it, I'll have to use playlist_ItemGetById()...
> If that's ok, then there's no problem...

It's ok to use this one, you have no choice ;)

>> This is pure speculation, and not an actual need. No need to bear that
>> burden yet.
>
> No, it's not, the MTP SD I wrote need a file path...

Ok.

>> And, this can be vague and actually get a meaningful name.
>
> Btw, those name were not decided by me, they were suggested last year
> when I worked on the SD api...

Too bad I missed it, sorry if it's a pain, but we are getting
something from that discussion. Thanks for your time.

>> +    int (*pf_item_put) ( services_discovery_t *, const char * );
>> This is non sense. First you have the item prefix, and no item is
>> being used at all.
>>
>> What do you actually put? put where?
>
> 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.

>> +    int (*pf_item_get) ( services_discovery_t *, const char *,
>> input_item_t * );
>>
>> What do you actually get? get from what?
>
> 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?

>> +    int (*pf_item_delete) ( services_discovery_t *, input_item_t * );
>>
>> This one is ok. But where do you delete the input item from?
>
> 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();

>> You don't get the point: New is a generic term that describe a
>> constructor, it's widely accepted.
>>
>> +VLC_EXPORT( int, services_discovery_AddParameterValue,
>> (services_discovery_t *, const char *, const char *, const char *, ...
>> ) LIBVLC_FORMAT( 4, 5 ) );
>> (The three strings refers to const char *psz_parameter, const char
>> *psz_name, const char *psz_format)
>>
>> - Why is this an Add instead of a Set?
>> - Why is there a format?
>> - psz_parameter and psz_name: I don't like this naming, and/or I don't
>> understand why you need both of them.
>
> 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?

> 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).

> 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?

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);

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?

> They are, actually, identical to input_items info_category_t, just
> with different meanings...

I hope we could build something better than this :-)

>> Why is this not internal? Please use accessors here if needed.
>
> 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.

Pierre.