GNOME Bugzilla – Bug 748896
src: Try all GrlSources in grl_multiple_get_media_from_uri()
Last modified: 2015-06-16 11:34:21 UTC
See the commit message for reasoning: grl_multiple_get_media_from_uri() can fail if a GrlSource says it can get a GrlMedia for a URI, but then returns NULL.
Created attachment 302868 [details] [review] src: Try all GrlSources in grl_multiple_get_media_from_uri() Previously, grl_multiple_get_media_from_uri() would stop as soon as a source returned TRUE for grl_source_test_media_from_uri() but then returned a NULL GrlMedia in its grl_source_get_media_from_uri() callback. That’s probably not what the user wanted, so instead continue to asynchronously try the remaining GrlSources until a GrlMedia is returned or the end of the sources list is reached.
(In reply to Philip Withnall from comment #1) > Created attachment 302868 [details] [review] [review] > src: Try all GrlSources in grl_multiple_get_media_from_uri() > > Previously, grl_multiple_get_media_from_uri() would stop as soon as > a source returned TRUE for grl_source_test_media_from_uri() but then > returned a NULL GrlMedia in its grl_source_get_media_from_uri() > callback. Do we have any sources in grilo-plugins that would behave like that? Any chance you could add a test for that case in grilo-plugins as well? > That’s probably not what the user wanted, so instead continue to > asynchronously try the remaining GrlSources until a GrlMedia is returned > or the end of the sources list is reached. That seems like the correct behaviour, but now it never returns an error. Should the first error be remembered and sent?
Review of attachment 302868 [details] [review]: Marking as needs-work as per comment.
(In reply to Bastien Nocera from comment #2) > (In reply to Philip Withnall from comment #1) > > Created attachment 302868 [details] [review] [review] [review] > > src: Try all GrlSources in grl_multiple_get_media_from_uri() > > > > Previously, grl_multiple_get_media_from_uri() would stop as soon as > > a source returned TRUE for grl_source_test_media_from_uri() but then > > returned a NULL GrlMedia in its grl_source_get_media_from_uri() > > callback. > > Do we have any sources in grilo-plugins that would behave like that? I can't prove it, but I suspect the Tracker plugin. > Any chance you could add a test for that case in grilo-plugins as well? Will do. > > That’s probably not what the user wanted, so instead continue to > > asynchronously try the remaining GrlSources until a GrlMedia is returned > > or the end of the sources list is reached. > > That seems like the correct behaviour, but now it never returns an error. > Should the first error be remembered and sent? I don't know; I thought about it, but I can't think of any particularly good reason for reporting the first error, or reporting no errors.
(In reply to Philip Withnall from comment #4) > (In reply to Bastien Nocera from comment #2) > > (In reply to Philip Withnall from comment #1) > > > That’s probably not what the user wanted, so instead continue to > > > asynchronously try the remaining GrlSources until a GrlMedia is returned > > > or the end of the sources list is reached. > > > > That seems like the correct behaviour, but now it never returns an error. > > Should the first error be remembered and sent? > > I don't know; I thought about it, but I can't think of any particularly good > reason for reporting the first error, or reporting no errors. I'd rather it reported the first error if every call failed.
(In reply to Philip Withnall from comment #4) > (In reply to Bastien Nocera from comment #2) > > (In reply to Philip Withnall from comment #1) > > > Created attachment 302868 [details] [review] [review] [review] [review] > > > src: Try all GrlSources in grl_multiple_get_media_from_uri() > > > > > > Previously, grl_multiple_get_media_from_uri() would stop as soon as > > > a source returned TRUE for grl_source_test_media_from_uri() but then > > > returned a NULL GrlMedia in its grl_source_get_media_from_uri() > > > callback. > > > > Do we have any sources in grilo-plugins that would behave like that? > > I can't prove it, but I suspect the Tracker plugin. > > > Any chance you could add a test for that case in grilo-plugins as well? > > Will do. So since the problem is in the Tracker plugin, and testing that is a nightmare (there are no existing unit tests for it, no test framework, and a huge amount of harness state to set up), I don’t think I can write a test for this. (In reply to Bastien Nocera from comment #5) > (In reply to Philip Withnall from comment #4) > > (In reply to Bastien Nocera from comment #2) > > > (In reply to Philip Withnall from comment #1) > > > > That’s probably not what the user wanted, so instead continue to > > > > asynchronously try the remaining GrlSources until a GrlMedia is returned > > > > or the end of the sources list is reached. > > > > > > That seems like the correct behaviour, but now it never returns an error. > > > Should the first error be remembered and sent? > > > > I don't know; I thought about it, but I can't think of any particularly good > > reason for reporting the first error, or reporting no errors. > > I'd rather it reported the first error if every call failed. The patch currently reports a GRL_CORE_ERROR_MEDIA_FROM_URI_FAILED error if it gets to the end of the GrlSources list and hasn’t found a result. I think this makes as much sense as reporting the first resolution error.
Comment on attachment 302868 [details] [review] src: Try all GrlSources in grl_multiple_get_media_from_uri() Marking as a-c-n as per discussion.
Cheers. Attachment 302868 [details] pushed as 179e6c3 - src: Try all GrlSources in grl_multiple_get_media_from_uri()