Re: [PATCH 0/9] Multi-valued proposal (WIP)

[Iago Toral <itoral igalia com>]

Hi Juan,

I'll be reviewing this patches, but give me some time :)

One thing that I don't like is GrlDataMulti: I don't see a good reason 
to have this, it does not solve any problem and introduces a new 
object/concept making things slightly more difficult for no good reason.

When we discussed this topic in the past we always talked about adding 
additional API to GrlData and making it handle single and multi valued 
data transparently, not about subclassing it, and that is still the 
approach I think we should follow. Let's keep things simple.

The other thing I don't particularly like is the approach to correlated 
properties. The relations between the properties are not explicit (we do 
not have explicit data types mapping them), so when I get a GrlProperty 
I might find thing a set of keys or another depending on the key I 
requested and the relationships I established at run-time. I am not sure 
if I like this better than having explicit types for each set of 
correlated properties, although I admit this approach is more dynamic 
since the correlations can be established at run-time. Somehow it feels 
like we forced the solution to reuse code and I don't like that feeling.

I guess the question here is if the extra flexibility we get from this 
approach and the code reuse is worth the additional slight obscurity.

The alternative to this, as I explained in an e-mail before would be to 
have explicit, fixed relations defined at compile time. Things like:

GrlDataVideoURI, GrlDataThumbnail, etc

These would be structs:

typedef struct {
  gchar *uri;
  gchar *mime;
  guint width;
  guint height;
  ...
} GrlVideoURI;

And then we would have API to set and get these structs from a GrlData 
object:

GrlMediaVideo *video;
GrlVideoUri *uri_data = grl_video_uri_new (uri, mime, width, height);
grl_media_video_set_uri (video, uri_data);
...
GrlVideoUri *uri_data = grl_media_video_get_uri (video);
gchar *uri = grl_video_uri_get_uri (uri_data);
gchar *mime = grl_video_uri_get_mime (uri_data);
...

As you can see, the correlations here are fixed, they are defined by 
structures, so they are explicit and we provide the users with specific 
APIs to handle each correlation,, which I think is less obscure than 
using a generic API. The drawback is of course that they are fixed, and 
that you have to create structures and specific API for each correlation 
(more code) and you cannot define new relations at run-time.

As I said, I still have not made up my mind on what approach I like 
better. I guess Juan likes what he proposed, so what do other think?

Iago

On Mon, 21 Feb 2011 10:36:26 +0100, "Juan A. Suarez Romero" 
<jasuarez igalia com> wrote:
> Hello.
>
> Some time ago, Guillaume sent an proposal about how to handle 
> multivalued
> elements in Grilo. It got some interesting issues about some
> drawbacks it had.
>
> So here there is a revamped version based on Guillaume proposal, that 
> also
> tries to fix the issues mentioned in that thread. Note that it
> probably needs a
> polishment, like choosing better names, and maybe adding some new 
> functions.
>
> First of all, some important points to take in mind to understand the
> proposal.
>
> - Being able to handle multi-valued keys should not complicate the 
> life of
>   people just interested in single-valued properties.
> - Applications handling multi-valued properties should not be worry 
> about
>   plugins providing only single-valued properties. That is, an
> application can
>   be able to handle multi-valued properties, while the plugin it uses
> can only
>   handle single-valued properties.
> - The other way around: applications handling single-valued 
> properties should
>   be able to work with plugins providing multi-valued properties in 
> the same
>   way as working with single-valued plugins.
> - Very likely, most of applications are just interesting in 
> single-valued
>   properties. Do not complicate their life.
>
> Based on above assumptions, the proposal here extend GrlData, which
> is able to
> handle single-valued elements only, with a new API specifically 
> designed to
> handle multivalued elements. This new API is provided in a new class,
> GrlDataMulti, that extends GrlData.
>
> GrlMedia and children now inherits from GrlDataMulti, instead of
> GrlData. It is
> important to note that I keep GrlMedia API as it is, that is, it only 
> handles
> single-valued elements. When an application/plugin wants to use 
> multi-valued
> API, it just casts the media to GrlMediaMulti and uses the right API 
> (I will
> expose an example later).
>
> One of the issues mentioned in Guillaume's proposal was the fact that
> some keys
> are somewhat correlated. That is, depending on the keys, there must 
> be a
> mathematical function between values of those two keys. For instance, 
> having
> several values for "url" key and several values for "mime" key means
> that each
> mime value corresponds to a value in url key.
>
> For this reason, I have introduced a new function in plugin registry,
> grl_plugin_register_register_metadata_relation(), that basically 
> creates a
> relation between keys. In the core, right now keys "url", 
> "mime-type",
> "bitrate", "framerate", "height" and "width" are correlated. It means
> that for
> a specific url value, it has a specific mime-type, bitrate, and so
> forth (or no
> values for some of them).
>
> To make easier the things, whenever a new relation between 2 keys is 
> created,
> all the keys that were related with those 2 become part of the 
> relationship
> too. We can say we create a relation-ring. In example above, url and
> the other
> keys are in the same relation.
>
> It is important to mention that this relationship concept only makes
> sense when
> handling multi-valued keys: if application just use single-valued 
> keys, that
> relation is of no interest.
>
> So, how it works the multi-valued stuff? GrlDataMulti provides api to 
> add,
> remove, update and get the value of a key from a specific position. 
> So we can
> ask for 2nd value of mime-type. Instead of returning only the asked 
> value, it
> returns also the values of correlated keys. Therefore, it will return
> also the
> appropriate value for uri, bitrate, and so on. Note that maybe some 
> of that
> values are actually NULL, but they are still returned.
>
> These correlated keys are returned in a GrlProperty, which is 
> basically a
> GrlData containing the keys with this values. It doesn't provide any 
> specific
> API, as GrlData API is enough to get it.
>
> Therefore, as an example, for a plugin providing several values for
> "url" key,
> to get the 3rd value and the corresponding mime-type we can write:
>
> GrlProperty *prop = grl_data_multi_get (GRL_DATA_MULTI (media),
> GRL_METADATA_KEY_URL, 2); /* Starts to count at 0 */
> const gchar *url = grl_data_get_string (GRL_DATA (prop),
> GRL_METADATA_KEY_URL);
> const gchar *mime = grl_data_get_string (GRL_DATA (prop),
> GRL_METADATA_KEY_MIME);
>
> For applications that only wants the values of a specific key,
> forgetting about
> other keys, there is API in GrlDataMulti to get them:
> grl_data_multi_get_single_all() returns a list of values for that 
> key.
>
> As I told at first, this is a work in progress, that probably needs 
> some
> polishment. In any case, here is to get some feedback.
>
>
>           J.A.
>
>
>
> Juan A. Suarez Romero (9):
>   core: Create relations between keys
>   core: Add a new class to handle multivalued properties.
>   core: Add place to store related keys and values.
>   core: Add API to handle multivalued data
>   core: Create relation with key "url"
>   core: Set an order when creating a relation ship
>   core: Create property with an initial set of keys
>   core: Use a representative element when handling multivalued data
>   core: Add API to get single multivalued elements
>
>  src/Makefile.am           |    4 +
>  src/data/grl-data-multi.c |  511
> +++++++++++++++++++++++++++++++++++++++++++++
>  src/data/grl-data-multi.h |  112 ++++++++++
>  src/data/grl-media.c      |    2 +-
>  src/data/grl-media.h      |    6 +-
>  src/data/grl-property.c   |   84 ++++++++
>  src/data/grl-property.h   |  101 +++++++++
>  src/grl-metadata-key.c    |   17 ++
>  src/grl-plugin-registry.c |   89 ++++++++
>  src/grl-plugin-registry.h |    7 +
>  10 files changed, 929 insertions(+), 4 deletions(-)
>  create mode 100644 src/data/grl-data-multi.c
>  create mode 100644 src/data/grl-data-multi.h
>  create mode 100644 src/data/grl-property.c
>  create mode 100644 src/data/grl-property.h
>
> _______________________________________________
> grilo-list mailing list
> grilo-list gnome org
> http://mail.gnome.org/mailman/listinfo/grilo-list