Re: RFC: Operation Options API proposal

[Iago Toral <itoral igalia com>]

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?

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

>   /* nice wrappers like: */
>   void grl_caps_set_frobuz (GrlCaps *caps, GrlFrobuz *frobuz);
>   GrlFrobuz *grl_caps_get_frobuz (GrlCaps *caps);
>
>   /* wrappers we already want */
>
>   /* whether source supports pagination (skip & count)
>   void grl_caps_set_pagination (GrlCaps *caps, gboolean pagination);
>   gboolean grl_caps_get_pagination (GrlCaps *caps);
>   /* flags the source can honour. Only GRL_RESOLVE_FAST_ONLY makes
> sense, since
>    * the other flags are entirely handled by core */
>   void grl_caps_set_flags (GrlCaps *caps, GrlMetadataResolutionFlags 
> flags);
>   GrlMetadataResolutionFlags grl_caps_get_falgs (GrlCaps *caps);
>
>
> Operation Options
> -----------------
>
>
> ::
>
>   /* new/free and setters is for programs, getters are for sources.
>    */
>
>   GrlOperationOptions *grl_operation_options_new (GrlCaps *caps);
>   void grl_operation_options_free (GrlOperationOptions *options);
>
>   /* Return TRUE if options could be set, FALSE if that was not 
> allowed
> by caps
>    */
>   gboolean grl_operation_options_set_value (GrlOperationOptions 
> *options,
>                                             const gchar *name,
>                                             GValue *value);
>   GValue grl_operation_options_get_value (GrlOperationOptions 
> *options,
>                                           const gchar *name);
>
>   /* nice helpers for the main types */
>   gboolean grl_operation_options_set_string (GrlOperationOptions 
> *options,
>                                              const gchar *name,
>                                              gchar *value);
>   gchar *grl_operation_options_get_string (GrlOperationOptions 
> *options,
>                                            const gchar *name);
>
>   gboolean grl_operation_options_set_boolean (GrlOperationOptions 
> *options,
>                                               const gchar *name,
>                                               gboolean value);
>   gboolean grl_operation_options_get_boolean (GrlOperationOptions 
> *options,
>                                               const gchar *name);
>
>   gboolean grl_operation_options_set_int (GrlOperationOptions 
> *options,
>                                           const gchar *name,
>                                           gint value);
>   gint grl_operation_options_get_int (GrlOperationOptions *options,
>                                       const gchar *name);
>
>   gboolean grl_operation_options_set_float (GrlOperationOptions 
> *options,
>                                             const gchar *name,
>                                             gfloat value);
>   gfloat grl_operation_options_get_float (GrlOperationOptions 
> *options,
>                                           const gchar *name);
>
>   /* nice wrappers like: */
>   gboolean grl_operation_options_set_frobuz (GrlOperationOptions 
> *options,
>                                              GrlFrobuz *frobuz);
>   GrlFrobuz *grl_operation_options_get_frobuz (GrlOperationOptions
> *options);
>
>   /* wrappers we already want */
>
>   /* Note that count is signed, so that it can cleanly be set to -1 
> to
> mean "no
>    * limit" */
>   gboolean grl_operation_options_set_pagination (GrlOperationOptions
> *options,
>                                                  guint skip, gint 
> count);
>   void grl_operation_options_get_pagination (GrlOperationOptions 
> *options,
>                                              guint *skip, gint 
> *count);
>
>   void grl_operation_options_set_flags (GrlOperationOptions *options,
>                                         GrlMetadataResolutionFlags 
> flags);
>   GrlMetadataResolutionFlags
>   grl_operation_options_get_falgs (GrlOperationOptions *options);
>
> :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.

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

> Alternative idea, similar to GstCaps in gstreamer, simpler to 
> implement for
> plugins, slightly less flexible::
>
>   GrlOperationOptions *
>   grl_media_source_supported_options (GrlMediaSource *source,
>                                       GrlSupportedOps operation);
>
>   GrlOperationOptions *
>   grl_operation_options_intersect (GrlOperationOptions *options1,
>                                    GrlOperationOptions *options2);
>
>
> ===================
> 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.

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

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

I think the proposal is good. I would start the implementation and 
then, if we find issues we may need to adjust some bits.

Iago