Re: Operation options (filtering, sorting and more)

[Iago Toral Quiroga <itoral igalia com>]

Hi,

El mar, 15-03-2011 a las 15:04 +0100, Guillaume Emont escribió:
> Hi Iago,
> 
> Thanks for your comments, please find inline my comments on your
> comments, on which you can comment ;)

Sure, I comment below on your comments on my comments on your
comments :P

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

Sure, I think that's totally fine, and in that case I think we should
forget about the add_option vmethod.
> > 
> > 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.

Ok, fair enough :)

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

And this sounds good to me :)

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

Since I have not looked at the code this explanation is a bit too
abstract for me to align with one option or the other, can you or juan
drop a couple of significant examples here?

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

Sounds good to me.

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

Sure, when you have a more specific proposal for this I'd love to look
at it.

> >>
> >> ======================
> >> 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_* ?

Sure, that's fine too.

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

Yes, I think Juan made a good point about this in another e-mail. Let's
merge flags, skip and count in the options then.

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

Yes, you mentioned that above and I agree.

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

Sounds good to me. 

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

Yes, we could expose the static API to the user as well, they are
complementary things in the end.

Also, to see what this API would be good for, I am dropping a few random
examples of filters here and I would like you to tell which ones would
be achievable with this API and which ones would not (and example of how
the API would be used to define the filter if it is supported would be
cool too!)

a) Artist = X
b) Duration > X
c) Date > X
d) Artist = X AND Date > Y
e) Artist = X AND Album = Y AND Genre = Z
f) Artist = X AND Duration > Y AND Date < Z
g) Artist = X AND Genre = Y AND Type = Audio
h) Artist = X AND Genre = Y AND (Type = Audio OR Type = Video)
i) Artist = X OR Artist = Y
j) (Artist = X OR Artist = Y) AND (Album = Z OR Album = Q)

Iago