Re: Operation options (filtering, sorting and more)

[Guillaume Emont <gemont igalia com>]

Hi Iago,

Thanks for your comments, please find inline my comments on your
comments, on which you can comment ;)

Guj

On 14/03/2011 09:45, Iago Toral Quiroga wrote:
> 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.

It sounds like a lot of work, and I have a feeling that most sources
will have a fixed set of capabilities per operation, or even a fixed set
overall.

How about we have that grl_my_source_add_option() (which is dynamic
negociation of capabilitites) as something optional, with a default
implementation in core that would use a get_supported_options() vmethod?
Actually, depending on whether we need the dynamic negociation at the
plugin level with present plugins, we might not have to add the
_add_option() vmethod now.

> 
> 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.
You could always imagine some back-end that behaves differently for
different operations, would it be for sorting, filtering, or some option
we have no idea of for now but might want to add in the future. Also,
some options might make sense only for some operations (if we include
skip and count, they won't make sense for metadata()), and it might be
good to have core check for that.

> 
>> 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.
> 
As said above, I'm now leaning towards a dynamic API exposed outside,
and, for now, a static API at the plugin level. That way, we keep the
option to add a dynamic API at the plugin level in the future if we need it.
> 
> 
>>
>> 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.
Juan's branch have them merged, but I feel this makes some structure
members, and some APIs have names that aren't as simple and obvious as
they could.
> 
>> =================
>> 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.
Yeah, I'm not convinced we need GObject either. And I believe we can
have a simple struct have a GType and use the GType private stuff.
> 
>>  - 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.
I think I'm now leaning towards dynamic (with a static API for plugins,
or maybe both static and dynamic for plugins), but still towards 2
types, but maybe the "Caps" type could be only internal to grilo and
plugins, I need to think a bit more about that.
> 
>>
>> ======================
>> 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;
> 
Yeah, or how about GrlSortOrder and GRL_SORT_ORDER_* ?
>>
>>   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?
It does. And maybe we could merge skip and count as well.
> 
>> 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.
Not as straight forward as if the plugin only have to implement a
_get_supported_options().

Also, for the outside API, how about:
options = grl_operation_options_new ()
grl_source_set_operation_options (source, options, option_name,
option_value);

> 
> 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.
Yeah, not sure it's fundamentally needed, but we could have both APIs
available to the outside world. For metadata keys, we have
_supported_keys() (static) and _may_resolve() (dynamic) don't we?
> 
> Iago
> 
> _______________________________________________
> grilo-list mailing list
> grilo-list gnome org
> http://mail.gnome.org/mailman/listinfo/grilo-list