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

[Pierre d'Herbemont]

On Sat, Jul 25, 2009 at 4:32 PM, Fabio Ritrovato<exsephiroth87 at gmail.com> wrote:
>> So if it's only used here, inline it.
>
> That's not possible, it need to access the pl_priv part of the
> playlist, and it can0t be done outside of the playlist API

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.

+services_discovery_t * playlist_ServiceDiscoveryGetById( playlist_t
*p_playlist,
+                                                       int i_id )
+{
...
+        if( i_id == pl_priv(p_playlist)->pp_sds[i]->p_cat->i_id ||
+            i_id == pl_priv(p_playlist)->pp_sds[i]->p_one->i_id )
+        {
+            PL_UNLOCK;
+            return pl_priv(p_playlist)->pp_sds[i]->p_sd;

You are the playlist outside the playlist lock. This is wrong.

> I can't understand what it is that you want, but I think you probably
> didn't fully undestrood what that's for...
> It's not something realted to searching, the string can be anything,
> and, actually, the SD never access or use the parameter list...
> It is only used by the interface to create a nice dialog to make the
> user choice what he wants to add, and that can be anything...
> For example, in the youtube SD, you can create a string like :
> category="Games" category_order="most_viewed"  time="all_time"...
> The parameter list is just a list of all the things an user can choose
> to display, nothing obscure or critical...

These are search terms. I understand that I may miss something, but
obviously, the string you create is a search query.

>>ItemPut() name is also too vague, and seems to be unrelated to string
>>passing to the SD.
>
> It has to be vague, that's the point, every SD act in a different way,
> one could accept a string to parse, one could need an url or a file
> path...

This is pure speculation, and not an actual need. No need to bear that
burden yet.

And, this can be vague and actually get a meaningful name.

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

+    int (*pf_item_get) ( services_discovery_t *, const char *,
input_item_t * );

What do you actually get? get from what?

+    int (*pf_item_delete) ( services_discovery_t *, input_item_t * );

This one is ok. But where do you delete the input item from?

> The function is to add something to the SD, regardless of what that is...
> And, for example, input_item_New, name, is unrelated to the fact you
> give it a string to create it...

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.

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

Also:
--- a/include/vlc_services_discovery.h
+++ b/include/vlc_services_discovery.h
@@ -40,6 +40,19 @@ extern "C" {
 #include <vlc_input.h>
 #include <vlc_events.h>

+typedef struct parameter_value_t
+{
+    char *psz_name;            /**< The name of the value */
+    char *psz_value;           /**< The actual name */
+} parameter_value_t;
+
+typedef struct parameter_t
+{
+    char   *psz_name;      /**< Name of the parameter */
+    int    i_values;        /**< Number of values in the parameter */
+    struct parameter_value_t **pp_values;     /**< Pointer to an
array of values */
+} parameter_t;
+

Why is this not internal? Please use accessors here if needed.

>>Please don't implement them yet then.
>
> My task was to create an API to let everyone code their own script,
> those functions are not used by me at the moment, but they can be
> useful for someone else (actually, i use them too, but not on these
> scripts...)

ok. If this is an API, that could be used from lua script.

Pierre.