Re: RFC: Operation Options API proposal

[Iago Toral Quiroga <itoral igalia com>]

El jue, 24-03-2011 a las 12:45 +0100, Guillaume Emont escribió:
> Hi!
> 
> Thanks for your answers, I replied in-line.
> 
> Guij
> 
> On 24/03/2011 10:33, Iago Toral wrote:
> > 
> > On Wed, 23 Mar 2011 14:50:32 +0100, Guillaume Emont <gemont igalia com>
> > wrote:
> > (...)
> >> Capabilities
> >> ------------
> >>
> >> The consensus seems to be reached on the necessity of a separate GrlCaps
> >> type,
> >> that would handle the capabilities of a Source.
> >>
> >> I would see an API similar to GrlData, but without having GrlCaps
> >> being an
> >> object.
> >>
> >> ::
> >>
> >>   /* setters _new()/_free are for sources to use, getters are for
> >>    * applications */
> >>
> >>   GrlCaps *grl_caps_new (void);
> >>   void grl_caps_free (GrlCaps *caps);
> >>
> >>   /* Using GValue here for consistency with GrlData, but I wonder
> >> whether a
> >>    * simple gpointer wouldn't be enough */
> >>   void grl_caps_set_value (GrlCaps *caps, const gchar *name, GValue
> >> *value);
> >>   GValue *grl_caps_get_value (GrlCaps *caps, const gchar *name);
> > 
> > If we have get/set_* then I think we do not really need a get/set_value
> > and could go with a get/set_data that would receive and return a
> > pointer. BTW, out of curiosity, do we have an example caps that could
> > need this GValue/pointer API?
> 
> I don't think that any specific cap would need that. My (vague)
> understanding is that using GValues may ease the work of binding
> writers, but not too sure. Is there a binding writer in the room?

Well, Simón worked on the GObject introspection support in Grilo, so
maybe he can shed some light here, Simón?

> Also, to ensure it's clear, I think the idea of a get/set_value or
> get/set_data is complementary with custom setters/getters, as these
> would be implemented using the generic getter/setter.

Sure.

> Also, the generic getter/setter could be useful if people want to add
> some custom caps that only their own plugin and application would need
> to know (without having to modify core)
> > 
> >>   /* nice helpers for the main types */
> >>   void grl_caps_set_string (GrlCaps *caps, const gchar *name, gchar
> >> *value);
> >>   gchar *grl_caps_get_string (GrlCaps *caps, const gchar *name);
> >>
> >>   void grl_caps_set_boolean (GrlCaps *caps, const gchar *name, gboolean
> >> value);
> >>   gboolean grl_caps_get_boolean (GrlCaps *caps, const gchar *name);
> >>
> >>   void grl_caps_set_int (GrlCaps *caps, const gchar *name, gint value);
> >>   gint grl_caps_get_int (GrlCaps *caps, const gchar *name);
> >>
> >>   void grl_caps_set_float (GrlCaps *caps, const gchar *name, gfloat
> >> value);
> >>   gfloat grl_caps_get_float (GrlCaps *caps, const gchar *name);
> > 
> > The caps I have on mind right now are things like filter types, sorting
> > options, operation flags, ... I wonder how the API would be used, for
> > example, to inform the user that a certain filter is supported.
> 
> In this model, that's up to the people who add new caps to define and
> document their exact meaning (e.g. in the documentation of the
> setter/getter they would add).
> e.g. for filter types you would have
> typedef enum {
>   GRL_FILTER_TYPE_NONE = 0,
>   GRL_FILTER_TYPE_BOX = 1 << 0,
>   GRL_FILTER_TYPE_AUDIO = 1 << 1,
>   GRL_FILTER_TYPE_VIDEO = 1 << 2,
>   GRL_FILTER_TYPE_PICTURE = 1 << 3,
> } GrlFilterType
> 
>   /* Types on which one can filter */
>   GrlFilterType grl_caps_get_filter_type (GrlCaps *caps);
> 
> That could allow a user to know whether this type is supported.
> 
> The implementer of a new caps would also need to add some code in
> GrlOptions to add the corresponding options. In that code, he would
> check that the caps agree with it, so that the user could do:
>   options = grl_operation_options_new (caps);
>   if (grl_operation_options_filter_on_type (options,
> GRL_FILTER_TYPE_VIDEO)) {
>    // ....
>   }

What about a more complex filter like the ones I put as examples in some
other e-mail I sent to the list? Like these:

A filter with an argument, like:
Artist=X

A chained filter like:
Artist=X AND Album=Y

A ranged filter, like:
X < Date < Y

(...)
> >> :Comments: We might want reference counting on GrlOperationOptions, so
> >> that
> >>            the application can choose between dropping its reference
> >>            immediately (and not have to free the options when the
> >> operation has
> >>            terminated) or keep it to use the same GrlOperationOptions
> >>            instance for several operations.
> > 
> > That's a good point, but in that case should we not just go with
> > GObject? If we actually decide that GrlCaps and GrlOptions should have a
> > common ancestor as well, then that sounds like another reason advising
> > the use of objects.
> We could. I don't know if we care about the memory overhead. If we're OK
> with a GObject, I think that we could inherit from GrlData that already
> provides a good part of what we need.

I think the memory overhead caused by GrlOptions and GrlCaps objects is
insignificant, there will not be many instances of these anyway.

> >> ====================
> >> Existing API changes
> >> ====================
> >>
> >>
> >> API modifications for media source
> >> ----------------------------------
> >>
> >> Caps::
> >>
> >>   GrlCaps *grl_media_source_get_caps (GrlMediaSource *source,
> >>                                       GrlSupportedOps operation);
> >>
> >> Use::
> >>
> >>   guint grl_media_source_browse (GrlMediaSource *source,
> >>                                  GrlMedia *container,
> >>                                  const GList *keys,
> >>                                  GrlOperationOptions *options,
> >>                                  GrlMediaSourceResultCb callback,
> >>                                  gpointer user_data);
> >>
> >>   /* And the same thing for the other operations. */
> >>
> >> First idea to help the user adjust their options to what the source
> >> supports::
> >>
> >>   gboolean
> >>   grl_media_source_supported_options (GrlMediaSource *source,
> >>                                       GrlSupportedOps operation,
> >>                                       GrlOperationOptions *options,
> >>                                       GrlOperationOptions
> >> **effective_options);
> > 
> > I like this better.
> 
> Actually, that's a remain from the old document that wasn't supposed to
> be there ;). With caps, I guess it would be more logical to have:
>   gboolean
>   grl_caps_fixate_options (GrlCaps *caps,
>                            static GrlOperationOptions *options,
>                            GrlOperationOptions **new_options);
>   With a way to create options without caps (grl_operation_options_new
> (GrlCaps *caps) would accept NULL as a parameter)

I understand new_options are the options accepted by the source. I'd
have it named supported_options and have another parameter called
unsupported_options as well, since clients may be interested in having
that too (both could be NULL and ignored).

(...)

> >>
> >> ===================
> >> Remaining Questions
> >> ===================
> >>
> >> Supposing we all agree on the above (I doubt we will without at least
> >> some
> >> slight modifications), some questions are still to be answered:
> >>
> >>  - Should we use options for metadata as well, or wait until we rework
> >> the class hierarchy?
> > 
> > I think we need options for metadata, since we have operation flags
> > there already IIRC.
> 
> Ok. The reason I ask that question is because that could mean a lot of
> code duplication.
> An alternative would be to implement most of the stuff only in
> GrlMetadataSource, since GrlMediaSource currently inherits from it.

Yes, that is what we have been doing in all cases like this.

> > 
> >>  - Should we have an externally visible common ancestor to
> >> GrlOperationOptions
> >>    and GrlCaps? (one that would implement the generic setters and
> >> getters)
> > 
> > They are both semantically similar, so I think from an implementation
> > POV having a common ancestor makes sense. Then again, from the user's
> > point of view they are still different things in the end, so I am not so
> > sure if making that externally visible makes sense.
> > 
> Well, if we go with GObject, I think we need to have that visible.

Would users have to use the ancestor's API to interact with the caps or
the option objects?

> >>  - Should we use GValue or gpointer?
> >> Please tell me if there are other aspects that I forgot.
> > 
> > GValue would make things more complicated for the user with no gain, I'd
> > go with gpointer.
> 
> As I said above, GValue might be useful for bindings, but I'm not too
> sure, I'd love to have some comments from somebody who understands that
> better than I do (maybe you?). Also, if we inherit from GrlData, it's
> already there

Juan prefers GValue and I don't have a strong opinion because in the end
I don't see people using this, they will use all the other _set_*
functions or the specific wrappers we provide 99% of the time. So let's
go with GValue.

> > I think the proposal is good. I would start the implementation and then,
> > if we find issues we may need to adjust some bits.
> Before starting, I think we need to ensure we agree on a few more points:
>  - inherit from GrlData? (I'd say yes)

I would not like to inherit from GrlData, same reason I always give for
that. GrlData was designed to support very different semantics (those of
the content hierarchy: GrlMedia, etc) and here we would be using it for
a concept that is semantically different (the operation caps and
options). If we want to reuse GrlData I'd rather use composition than
inheritance, but even in that case I'd probably just use a GHashTable
and leave GrlData alone.

>  - put all the stuff in GrlMetadataSource (and use grl_metadata_source_*
> methods on media sources as well)? (I'd say yes as well, and we need to
> plan quickly when to fix that class hierarchy)

Yes, that is what we should do. And yes also to fixing the hierarchy as
a high priority thing.

Iago