Maybe the problem in this case is that actually we are trying to merge two metadata providers in just one: the GIO-based metadata provider and MediaArtStorageSpec-based provider.

In any case, the case is just there: a provider could give a metadata just having either a property value or another property value.

A quick fix could be key_depends() adding a "gboolean *able_to_resolve" parameter to tell if the key can be solved or not. In case it can, we could assume that an empty list means "I can't tell you the dependency, as it can vary".

The good part of it is that requires a minimal change in plugins, and no any change on clients (they do not use that function). Of course, it requires a change in core.


A different solution is change the semantics so returning a NULL list means the an unknown dependency. In this case, core should use supported_keys() to ask plugin which keys it supports, and do not invoke key_depends() for the keys that are not supported.

Basically, previous approach just means do the fix in core, without affecting neither the plugins nor the clients.

I think that able_to_resolve boolean is redundant with ->supported_keys() which already provide that information. Or do I misunderstand what you mean?

For the moment, returning NULL is interpreted as "this key is not supported", which doesn't make sense because core should only call key_depends() for keys returned by supported_keys(). If we fix that issue, then we can assign another meaning to NULL, such as "I don't know".

In other words: I agree with your last solution. Core should never rely on key_depends() to know if a key is supported, supported_keys() is here for that.

(In reply to comment #2)
> I think that able_to_resolve boolean is redundant with ->supported_keys() which
> already provide that information. Or do I misunderstand what you mean?

You didn't :)

In current semantics, key_depends() is also able to tell you if a key can be resolved or not (which in turns is a bit redundant, having supported_keys()). So the proposal tried to keep the same semantic, as well as extending it to include "I can't tell you the dependencies, as it may vary".

> 
> For the moment, returning NULL is interpreted as "this key is not supported",
> which doesn't make sense because core should only call key_depends() for keys
> returned by supported_keys(). If we fix that issue, then we can assign another
> meaning to NULL, such as "I don't know".
> 
> In other words: I agree with your last solution. Core should never rely on
> key_depends() to know if a key is supported, supported_keys() is here for that.

That's my opinion too; we should go with this approach. Just I wanted to give a couple of alternatives ;).

(In reply to comment #0)
(...)
> A simple solution to that would be to consider that if ->key_depends() return
> NULL (the empty list), the dependency is unknown. As of now, the core seems to
> discard[2] a metadata source if its key_depends() return NULL for a given
> metadata key.
> 
> An optimisation to that could be to provide more context to key_depends(), such
> as by providing the GrlMedia for which one wants to find metadata as a
> parameter.

Maybe what we need is a different API: instead of having key_depends returning a GList with the required dependencies we can:

1) have it return a list of lists, so if a certain key can be resolved in 2 ways, we would have a list with two lists.

2) replace key_depends with something like 'gboolean can_resolve(GrlMedia, key_id)', and the plugin would return TRUE or FALSE after inspecting the media object and the key we are asking for.

Option 2) puts some extra work on the plugin side compared to the original key_depends, but it should be no more than a couple of checks in the end, and it solves any flexibility issues.

Notice that even though supported_keys and can_resolve/key_depends could be thought as being somewhat redundant, they are not: supported_keys would say exactly that, what keys are supported, key_depends/can_resolve would say, for a key (even if it is supported) if the data we have is enough to resolve it.  the first one is static, while the second one is dynamic and depends on user data, so depending on the use case you may need one or the other. In the case of our full_resolution implementation we might just ignore key_depends and use only key_depends/can_resolve.

(In reply to comment #4)
> 
> 1) have it return a list of lists, so if a certain key can be resolved in 2
> ways, we would have a list with two lists.
> 
> 2) replace key_depends with something like 'gboolean can_resolve(GrlMedia,
> key_id)', and the plugin would return TRUE or FALSE after inspecting the media
> object and the key we are asking for.
> 
> Option 2) puts some extra work on the plugin side compared to the original
> key_depends, but it should be no more than a couple of checks in the end, and
> it solves any flexibility issues.
2) looks simpler and more efficient to me.
If we go with that way, I think we should have the following rules (or similar ones):
 - the plugin implementation of can_resolve() should not block
 - if the plugin cannot know whether it can resolve that metadata without blocking, it should return TRUE
 - the core should use a FALSE return as a reason not to try with that plugin, but not infer that resolution _will_ work with a TRUE return

I admit that with this set of rules, may_resolve() might be a better name.
> 
> Notice that even though supported_keys and can_resolve/key_depends could be
> thought as being somewhat redundant, they are not: supported_keys would say
> exactly that, what keys are supported, key_depends/can_resolve would say, for a
> key (even if it is supported) if the data we have is enough to resolve it.  the
> first one is static, while the second one is dynamic and depends on user data,
> so depending on the use case you may need one or the other. In the case of our
> full_resolution implementation we might just ignore key_depends and use only
> key_depends/can_resolve.

(In reply to comment #4)

> 2) replace key_depends with something like 'gboolean can_resolve(GrlMedia,
> key_id)', and the plugin would return TRUE or FALSE after inspecting the media
> object and the key we are asking for.

Wouldn't it better that instead of can_resolve() have gboolean resolve(GrlMedia, key_id) so it resolves the key and returns TRUE or FALSE if succeed?

(In reply to comment #4)
> Notice that even though supported_keys and can_resolve/key_depends could be
> thought as being somewhat redundant, they are not: supported_keys would say
> exactly that, what keys are supported, key_depends/can_resolve would say, for a
> key (even if it is supported) if the data we have is enough to resolve it.  the
> first one is static, while the second one is dynamic and depends on user data,
> so depending on the use case you may need one or the other. In the case of our
> full_resolution implementation we might just ignore key_depends and use only
> key_depends/can_resolve.


Well, actually one of the proposals were that key_depends() only would receive a supported key, never a unsupported one. The current implementation is that key_depends() can get an unsupported key, and in some way is acting also as supported_keys(), which shouldn't do.

(In reply to comment #6)
> (In reply to comment #4)
> 
> > 2) replace key_depends with something like 'gboolean can_resolve(GrlMedia,
> > key_id)', and the plugin would return TRUE or FALSE after inspecting the media
> > object and the key we are asking for.
> 
> Wouldn't it better that instead of can_resolve() have gboolean
> resolve(GrlMedia, key_id) so it resolves the key and returns TRUE or FALSE if
> succeed?

I'd like to try to summary/understand what is the goal of key_depends().
To me, this is an optimisation, to avoid calling resolve() on sources that we know will not be able to do anything, having an algorithm like:
if key in source->supported():
  deps = source->key_depends(key)
  if deps included in keys_we_already_have:
    source->resolve(resolve_spec)

In that context, having resolve() behave like you say would mean that the plugin have the responsibility to check at the beginning of resolve() if it can indeed resolve the key with the information it has, and return immediately FALSE if it can't (potentially calling the callback with no result before, or maybe just calling the callback and returning void like before).
Things would still be at least as optimal as the algorithm above, only the check would be moved to the plugin, which means the check could be more dynamic/dependent on the data.
And anyway, I'm guessing all plugins already do that kind of check at the start of resolve(), just to avoid a backend error if some data is missing.

(In reply to comment #8)
> I'd like to try to summary/understand what is the goal of key_depends().
> To me, this is an optimisation, to avoid calling resolve() on sources that we
> know will not be able to do anything, having an algorithm like:
> if key in source->supported():
>   deps = source->key_depends(key)
>   if deps included in keys_we_already_have:
>     source->resolve(resolve_spec)

Yes, but not only that. It is also a method for clients to know what keys are actually supported by the plugin, in case they need to know. In this case the client is grilo itself, but an app could use this method just as well in the implementation of any use case that could require this or that could be optimized based on this information.

> In that context, having resolve() behave like you say would mean that the
> plugin have the responsibility to check at the beginning of resolve() if it can
> indeed resolve the key with the information it has, and return immediately
> FALSE if it can't (potentially calling the callback with no result before, or
> maybe just calling the callback and returning void like before).
> Things would still be at least as optimal as the algorithm above, only the
> check would be moved to the plugin, which means the check could be more
> dynamic/dependent on the data.
> And anyway, I'm guessing all plugins already do that kind of check at the start
> of resolve(), just to avoid a backend error if some data is missing.

Just to ensure we are all on the same page: one thing is metadata we can resolve in principle (supported_keys), another thing is what we can really resolve given a certain set of already known metadata (can_resolve or whatever we want to name this). These two would make sense mostly as synchronous APIs. And then we have resolve, which is intended to do the actual resolution and has an asynchronous nature.

If we mix can_resolve/key_depends with resolve, clients won't be able to test separately. Example of the problems this may raise: Let's say the app wants to let the user select, among the metadata plugins that can resolve a certain key, the ones the user wants to use or the order in which he wants to try. In order to do that, the app needs to test first if the plugins can actually resolve the data the user is interested in, so we need to know that first and resolve later.

I think we do not really gain anything by mixing both APIs and we are actually losing something, so I would not do that.

Another reason (even though this is just a personal thing) is that I just don't like APIs that depending on their return value (the gboolean) change their behavior from synchronous (when we return FALSE and no resolution is done) to asynchronous (when the can do the resolution and return the result in the callback).

(In reply to comment #5)
> (In reply to comment #4)
> > 
> > 1) have it return a list of lists, so if a certain key can be resolved in 2
> > ways, we would have a list with two lists.
> > 
> > 2) replace key_depends with something like 'gboolean can_resolve(GrlMedia,
> > key_id)', and the plugin would return TRUE or FALSE after inspecting the media
> > object and the key we are asking for.
> > 
> > Option 2) puts some extra work on the plugin side compared to the original
> > key_depends, but it should be no more than a couple of checks in the end, and
> > it solves any flexibility issues.
> 2) looks simpler and more efficient to me.
> If we go with that way, I think we should have the following rules (or similar
> ones):
>  - the plugin implementation of can_resolve() should not block
>  - if the plugin cannot know whether it can resolve that metadata without
> blocking, it should return TRUE
>  - the core should use a FALSE return as a reason not to try with that plugin,
> but not infer that resolution _will_ work with a TRUE return
> 
> I admit that with this set of rules, may_resolve() might be a better name.

I agree on the rules, and yes, maybe may_resolve is a better choice.

A drawback with the proposed can_resolve() function is that it doesn't give us why it can't resolve the keys. In most cases, failing is because it requires some fields in order to resolve the key (for instance, most of times getting an albumart for a song requires to have artist and album).

Thus if plugin P1 requires K2 to solve K1, and K2 is provided by P2, it should invoke first P2.resolve() and then P1.resolve(). Without this information, basically it can't plan the execution order.

So trying to keep the same rules proposed to can_resolve(), I think that can_resolve() can have an extra parameter, "GList **required_keys", which is basically a list of keys required to resolve the asked key.

Therefore, we would have the following combinations:

- returns FALSE + NULL required_keys: it means that definitely this plugin is not able to resolve the asked keys.

- returns FALSE + required_keys: it means it can't resolve the asked keys due of lacking of the asked keys. If user provides values for those keys, then it could resolve the metadata.

- returns TRUE + NULL required_keys: it means it can resolve the metadata with the provided media.

- returns TRUE + required_keys: actually this shouldn't happen. A semantic for this combination, if we want to assign one, is that plugin is able to resolve the asked metadata, but providing required_keys would increase the probability of real success. The point here is, as Guillaume commented, plugins must not block when invoking can_resolve(). Thus, a plugin returnin TRUE can fail at the end when trying to do the real resolve. If plugin knows that having a these keys can increase the hitting, it could return TRUE + list of desirable keys.


A special case for the proposal is that invoking can_resolve() with a NULL media and a key would return FALSE and a list of required keys to solve the asked key. That is, it would be behaving like key_depends(). Thus, I think it's worth to get rid of key_depends() and just use can_resolve() to do the work.


Finally, I think also it is worth to add to resolve() callback a list of failed keys, in the same way it is in the GrlMetadataSourceSetMetadataCb. Core/users would be able to detect which keys couldn't be solved, and use a different plugin to solve them.

Started a thread on the mailing list on the subject: http://mail.gnome.org/archives/grilo-list/2011-February/msg00013.html

key_depends() has been deprecated in favour of may_resolve().

Thus, closing it.