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

[Fabio Ritrovato]

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

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

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

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

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

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

> 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...
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...
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\""
....
In the interface, a first drop down box is filled with parameters
("Categories, Category order", etc...)...
When you select an item from there, a second drop down is filled with
that parameter possible values ("Gaming", "Music", etc...)
By selecting those, you can build a string of what you want, and then
pass it to the SD...

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

> Parameter and Value in the function you describe is misleading because
> it could be anything. "Parameter" is generally used for function
> parameters.
>
> Moreover, if we were to keep Parameter, you wanted
> SetValueForParameter() or even SetValueForParameterAsString(),
> SetValueForParameterAsNumber()...

If that name is better, no problem in renaming...
I think there's little point in having a numeric value, as values are
a "name=\"something\"", so they can be parsed easily...

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