Re: Operation options (filtering, sorting and more)

[Iago Toral Quiroga <itoral igalia com>]

El vie, 11-03-2011 a las 20:28 +0100, Guillaume Emont escribió:
(...)
> =================================
> How to handle source capabilities
> =================================
> 
> I think that's the part where the API decisions seem the least obvious
> and need
> decision making. These parameters would strongly influence the APIs we
> have on
> GrlOperationOptions, and whether we have an additional GrlOperationCaps
> (type
> names are up for discussion as well).
> 
> Static
> ------
> 
> The options supported by the source are properties (not necessarily GObjcet
> properties), similar to  grl_metadata_source_supported_keys(). There
> could be
> one per operation.
> 
> :Pros: Simple to implement for sources
> :Cons: Less flexible
> 
> Dynamic
> -------
> 
> We give a set of options, the operation to achieve (and maybe the other
> options
> to be passed to the operation function), and the source says whether it can
> honour the options, optionally telling what it can honour in the passed
> options.
> 
> :Pros: More flexible
> :Cons: Slightly more to implement for sources
> 
> 
> One instance for options negotiation
> ------------------------------------
> 
> :Pros: less instances to create
> :Cons: double usage for the instance, semantics lose a bit of clarity
> 
> 
> Two instances for options negotiation
> -------------------------------------
> 
> :Pros: simpler structure(s), lower memory footprint of instance
> :Cons: caller needs to handle 2 instances
> 
> 
> Examples
> --------
> 
> Some examples of what various choices would result in code for someone using
> the API.
> 
> one instance, dynamic::
> 
>   options = grl_operation_options_new ();
>   /* frobuz and gharble1 matter so much to us that we don't want to do the
>    * operation if they aren't available. gharble2 would be cool, but we
> can do
>    * without it. */
>   grl_operation_options_set_frobuz (options, frobuz);
>   grl_operation_options_add_gharbles (options, gharble1, gharble2, NULL);
>   grl_media_source_fixate_options (source, GRL_OP_BROWSE, options);
>   if (grl_operation_options_get_frobuz (options) == frobuz
>       && grl_operation_options_has_gharble (options, gharble1))
>     grl_media_source_browse (source, container, keys, skip, count, flags,
>                              options, my_callback, some_data);

>From the client point of view, I would like more an approach like this:

grl_operation_options_new_for_source (source) and then the setters
return TRUE (if the option is supported) or FALSE otherwise.

the code would look like this:

options = grl_operation_options_new_for_source (source);
if (grl_operation_options_set_frobuz (options, frobuz) && 
    grl_operation_options_add_gharbles (options, gharble1)) {
    grl_operation_options_add_gharbles (options, gharble2);
    grl_media_source_browse (source, container, keys, skip, count,
                             flags, options, callback, data);
}

On the plugin side this would mean implementing a virtual function with
this signature:

gboolean grl_my_source_add_option (options, option_name, option_value);

a GrlOperation object would always call this method on the target source
to return TRUE or FALSE on any of its setters.

I am not sure if the operation options should also consider the
operation type (browse, query, etc), right now I don't see a good reason
for this, but maybe there is one.

> one instance, static::
> 
>   options = grl_media_source_get_capabilities (source, GRL_OP_BROWSE);
>   /* frobuz and gharble1 matter so much to us that we don't want to do the
>    * operation if they aren't available. gharble2 would be cool, but we
> can do
>    * without it. */
>   if (grl_operation_options_can_set_frobuz (options)
>       && grl_operation_options_can_have_gharble (options, gharble1)) {
>     grl_operation_options_set_frobuz (options, frobuz);
>     grl_operation_options_add_gharble (options, gharble1);
>     if (grl_operation_options_can_have_gharble (options, gharble2))
>       grl_operation_options_add_gharble (options, gharble2);
>     grl_media_source_browse (source, container, keys, skip, count, flags,
>                              options, my_callback, some_data);
>   }

to be honest, I don't have a strong preference between dynamic or
static.



> 
> two instances, static::
> two instances, dynamic::
> two instances, static (variant)::
(...)

I don't see a good reason to separate caps from options, but I don't
have a strong opinions here either.

> =================
> Proposed Solution
> =================
> 
>  - Opaque structure, with setters/getters, so that we can easily add new
>    options while preserving the API
>  - Padding in the structure, to be able to add new options while
> preserving the
>    ABI. Alternative: GType private stuff

GType private looks like a better alternative than structure padding to
preserve ABI. However I wonder if using GObject to model GrlOptions
instead of simple structure makes sense.

>  - still need a choice: static? dynamic? one instance? two instances?
> one type?
>    two types (options + caps)?

As I said, I would go with dynamic, one instance, one type, but I don't
have a strong preference, so if you guys feel confident that other
options are better, go for them.

> 
> ======================
> Object and API changes
> ======================
> 
> New types
> ---------
> 
> Types I think we will need in any case::
> 
>   typedef struct _GrlOperationOptions GrlOperationOptions;
> 
>   typedef enum {
>     GRL_DIRECTION_ASCENDING,
>     GRL_DIRECTION_DESCENDING
>   } GrlDirection;

I think GrlDirection is too generic, I would rather use something more
descriptive, like this:

typedef enum {
  GRL_SORT_TYPE_ASCENDING,
  GRL_SORT_TYPE_DESCENDING
} GrlSortType;

> 
>   typedef enum {
>     GRL_FILTER_TYPE_NONE = 0,
>     GRL_FILTER_TYPE_AUDIO = (1 << 0),
>     GRL_FILTER_TYPE_VIDEO = (1 << 1),
>     GRL_FILTER_TYPE_IMAGE = (1 << 2),
>   } GrlTypeFilter;

Ok.

> API additions for media source
> ------------------------------
> 
> At least::
> 
>   guint grl_media_source_browse (GrlMediaSource *source,
>                                  GrlMedia *container,
>                                  const GList *keys,
>                                  guint skip,
>                                  guint count,
>                                  GrlMetadataResolutionFlags flags,
>                                  GrlOperationOptions *options,
>                                  GrlMediaSourceResultCb callback,
>                                  gpointer user_data);

Seeing the API my feeling is that we should probably merge flags and
options together, they are both operation modifiers in the end. The only
difference being that in the case of flags, they are mostly handled by
core, but that is something the user does not really care about...

It would remove one of the parameters from the API (which is getting
pretty big) and would merge together two concepts in one, which I guess
could make this also easier to understand.

Does it make sense?

> And the same thing for the other operations. Should we do this for Metadata
> Sources operations as well?

Right now I don't see any specific options for metadata operations that
we may want to add, however, of we go with merging flags and options we
would have everything prepared if we need this in the future.

> Then we need some methods to determine the capabilities of a source,
> 
> First idea to help the user adjust his options to what the source supports::
> 
>   gboolean
>   grl_media_source_supported_options (GrlMediaSource *source,
>                                       GrlSupportedOps operation,
>                                       GrlOperationOptions *options,
>                                       GrlOperationOptions
> **effective_options);

As I said,  my preference for this is
grl_operation_options_new_for_source and have a virtual method in
GrlMediaSource returning TRUE/FALSE for setting individual operation
options.

I think this would fit very well with typical use cases, and makes the
plugin side implementation straight forward as well.

The only issue with this approach is the use case in which we want a
declarative list of all the options supported by the source. For this
case, the user would need to test the source with all the options one by
one, but oh well, it does not look like the typical use case and it is
not such a pain in the ass to implement either.

Iago