GNOME Bugzilla – Bug 644383
[feature-request] Metadata request are not cancellable
Last modified: 2011-04-13 10:03:03 UTC
Calling grl_metadata_source_resolve() can take time. And by the time the request has been completed, we might want to cancel the operation. It would be cool to have a cancel() method for metadata sources.
Yes, it makes sense. We should add this to our roadmap.
We are working on it.
Created attachment 185289 [details] [review] core: Move operation handling to GrlMetadataSource So we can also handle metadata's operation states. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185290 [details] [review] core: Move cancel() to GrlMetadataSource So GrlMetadataSource's methods can be cancelled too. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185291 [details] [review] core: Make get_media_from_uri() a cancellable operation Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185292 [details] [review] core: Send operation id if operation is cancellable metadata() and media_from_uri() are both cancellable operations, but none of them are sending the operation id in the callback. This commit fix this. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185293 [details] [review] core: Make resolve() a cancellable operation Fixes https://bugzilla.gnome.org/show_bug.cgi?id=644383 Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185294 [details] [review] core: Cancel pending resolve() in full-resolution mode With GRL_RESOLVE_FULL flag, metadata/search/browse invoke resolve() to resolve keys that they couldn't resolve. If the operation is cancelled, cancel all the resolve() pending to end. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185295 [details] [review] apple-trailers: set/get operation data is in GrlMetadataSource These operations have been moved from GrlMediaSource to GrlMetadataSource. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185296 [details] [review] jamendo: set/get operation data is in GrlMetadataSource These operations have been moved from GrlMediaSource to GrlMetadataSource. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185297 [details] [review] shoutcast: set/get operation data is in GrlMetadataSource These operations have been moved from GrlMediaSource to GrlMetadataSource. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185298 [details] [review] all: cancel() belongs to GrlMetadataSource cancel() method has been moved from GrlMediaSource to GrlMetadataSource. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185299 [details] [review] all: Send operation id in metadata()/media_from_uri()'s callbacks These callbacks requires now to send the operation id. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185300 [details] [review] all: resolve() is a cancellable operation As such, the callback needs to send the operation ID. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185715 [details] [review] core: Move operation handling to GrlMetadataSource So we can also handle metadata's operation states. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185716 [details] [review] core: Move cancel() to GrlMetadataSource So GrlMetadataSource's methods can be cancelled too. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185717 [details] [review] core: Make get_media_from_uri() a cancellable operation Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185718 [details] [review] core: Send operation id if operation is cancellable metadata() and media_from_uri() are both cancellable operations, but none of them are sending the operation id in the callback. This commit fix this. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185719 [details] [review] core: Make resolve() a cancellable operation Fixes https://bugzilla.gnome.org/show_bug.cgi?id=644383 Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185720 [details] [review] core: Cancel pending resolve() in full-resolution mode With GRL_RESOLVE_FULL flag, metadata/search/browse invoke resolve() to resolve keys that they couldn't resolve. If the operation is cancelled, cancel all the resolve() pending to end. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185721 [details] [review] lastfm-albumart: Add cancellable resolve() Implement cancel() for resolve() operation. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Created attachment 185722 [details] [review] local-metadata: Add cancellable resolve() Implement cancel() for resolve() operation. Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>
Review of attachment 185716 [details] [review]: ::: src/grl-media-source.c @@ +2050,3 @@ + GRL_WARNING ("grl_media_source_cancel() is deprecated. " + "Use instead grl_metadata_source_cancel()"); "Use grl_metadata_source_cancel() instead" ::: src/grl-metadata-source.c @@ +1428,3 @@ + + /* FIXME COMMENT: Mark the operation as finished, if the source does + not implement cancellation or it did not make it in time, we will "FIXME:" is enough, the "comment" is implicit :P
Review of attachment 185720 [details] [review]: ::: src/grl-media-source.c @@ +978,3 @@ + grl_metadata_source_cancel (GRL_METADATA_SOURCE (key), + GPOINTER_TO_UINT (value)); +} This is quite misleading, specially considering the meaning of "key" in Grilo :) If "key" is a source, call it source, not key. And if value is an operation id, call it operation_id, not value :) @@ +995,3 @@ + if (resolve_id > 0) { + g_hash_table_remove (cb_info->pending_callbacks, source); Can resolve_id be <= 0?
Review of attachment 185719 [details] [review]: ::: src/grl-metadata-source.c @@ +368,3 @@ + rrc->spec->resolve_id)) { + /* if the plugin already set an error, we don't care because we're + * cancelled */ This comment is not well indented.
The patches look good to me except for some nitpicks I mentioned. Anyway, please test this well with grilo-test-ui, both with full-resoultion and without it, check that browse and search operations are properly cancelled, etc
> + > + /* FIXME COMMENT: Mark the operation as finished, if the source does > + not implement cancellation or it did not make it in time, we will > > "FIXME:" is enough, the "comment" is implicit :P He, he. Actually, I meant "FIX THIS COMMENT", but I forgot to fix it. Anyway, I fixed the comment and also applied your suggestions. Thanks!
Review of attachment 185720 [details] [review]: ::: src/grl-media-source.c @@ +978,3 @@ + grl_metadata_source_cancel (GRL_METADATA_SOURCE (key), + GPOINTER_TO_UINT (value)); +} Well, see that the context is static void cancel_resolve (gpointer key, gpointer value, gpointer user_data) { grl_metadata_source_cancel (GRL_METADATA_SOURCE (key), GPOINTER_TO_UINT (value)); } cancel_resolve() is a GHFunc invoked for each pair (key, value) in a hash table: that is why I kept the names "key" and "value", as it is in the GHFunc signature. Anyway, if this is causing a confussion, I will rename the variables to "source" and "operation_id", respectively. @@ +995,3 @@ + if (resolve_id > 0) { + g_hash_table_remove (cb_info->pending_callbacks, source); resolve_id can't be < 0, as it is a guint. But it can be 0: it happens when full_resolution_done_cb() is invoked directly. As it it needs a operation id parameter, we use 0 as a "fake" id. full_resolution_done_cb() is invoked directly when there is an error in full_resolution_ctl_cb(), or when there is no any metadata source to use to solve pending keys, so it must be invoked directly to send the results (you can see this in full_resolution_ctl_cb() too).
(In reply to comment #25) > Review of attachment 185719 [details] [review]: > > ::: src/grl-metadata-source.c > @@ +368,3 @@ > + rrc->spec->resolve_id)) { > + /* if the plugin already set an error, we don't care because we're > + * cancelled */ > > This comment is not well indented. Fixed, thanks
Created attachment 185813 [details] [review] core: Move cancel() to GrlMetadataSource New version addressing the comments
Created attachment 185814 [details] [review] core: Cancel pending resolve() in full-resolution mode New version applying changes from comment #24
Created attachment 185815 [details] [review] core: Make resolve() a cancellable operation Applied changes from comment #25
Thanks for all your comments. Pachtset landed in core: efa9264 core: Cancel pending resolve() in full-resolution mode 97c9bd0 core: Make resolve() a cancellable operation 15d1e0d core: Send operation id if operation is cancellable 7652ee7 core: Make get_media_from_uri() a cancellable operation c64f733 core: Move cancel() to GrlMetadataSource 7e991e7 core: Move operation handling to GrlMetadataSource and in plugins: 81c8fa1 local-metadata: Add cancellable resolve() ca84d95 lastfm-albumart: Add cancellable resolve() ac6e313 all: resolve() is a cancellable operation 5fd7ee0 all: Send operation id in metadata()/media_from_uri()'s callbacks defa93b all: cancel() belongs to GrlMetadataSource f78a243 shoutcast: set/get operation data is in GrlMetadataSource 65081c4 jamendo: set/get operation data is in GrlMetadataSource 5bb0d0c apple-trailers: set/get operation data is in GrlMetadataSource