GNOME Bugzilla – Bug 628648
Add Grilo plugin
Last modified: 2011-07-01 09:32:49 UTC
To allow us to remove some others.
Created attachment 169399 [details] [review] Add Grilo plugin This new plugin should remove the need for many of the other plugins currently shipped by Totem.
I'll probably want to create a new plugin, using the YouTube plugin as a base, for search sources, and we'll see what to do for browse sources.
Created attachment 169421 [details] [review] Add Grilo plugin This new plugin should remove the need for many of the other plugins currently shipped by Totem.
*** Bug 393095 has been marked as a duplicate of this bug. ***
*** Bug 555823 has been marked as a duplicate of this bug. ***
*** Bug 564399 has been marked as a duplicate of this bug. ***
*** Bug 633933 has been marked as a duplicate of this bug. ***
Created attachment 187963 [details] [review] Grilo plugin This patch is a re-work of previous patch, to update to latest Grilo changes, and to address some issues Bastien told on irc. This plugin now shows the content in two different sidebars, one for browsing through the multimedia sources, and another sidebar to perform searches. Note that this is a first version that very likely needs lots of improvements. I attach it now to get feedback about what issues should be improved, and what changes would be required.
Review of attachment 187963 [details] [review]: Missing changes to POTFILES.in/skip ::: src/plugins/grilo/totem-grilo.c @@ +72,3 @@ +#define YOUTUBE_KEY "AI39si7Ix-bf9xRLx5PK1sjsG2FQjSHblunuaI8YvHdYbkV2BcgOemHE6Nzb9t6LaMsekofKGSwgAETzbo7OvL8L3vH4vrFaqw" +#define VIMEO_KEY "db924d3b7f1d5f3d6fdfeb4488ad01e2" +#define VIMEO_SECRET "bef979d87f28ec47" Can't the keys be shipped somewhere else? Otherwise we'd need to do new releases to support new backends, which makes the point rather moot. @@ +74,3 @@ +#define VIMEO_SECRET "bef979d87f28ec47" + +enum { ICON_BOX = 0, missing line feed here. @@ +81,3 @@ + +typedef struct { + Totem *totem; Tabs for indentation @@ +133,3 @@ + MODEL_RESULTS_IS_PRETHUMBNAIL, + MODEL_RESULTS_DESCRIPTION, + MODEL_RESULTS_DURATION, NUM_RESULTS? and in the above as well, so we don't need to hard code the number of columns. @@ +136,3 @@ +}; + +static guint max_items = MAX_ITEMS_DEFAULT; Why a global? @@ +147,3 @@ + +static gchar * +seconds_to_strtime (gint total_seconds) There's already code in totem to do that. @@ +223,3 @@ + + const gchar *icon_name[] = { GTK_STOCK_DIRECTORY, + "gnome-mime-audio", That's most likely the wrong name. @@ +224,3 @@ + const gchar *icon_name[] = { GTK_STOCK_DIRECTORY, + "gnome-mime-audio", + "gnome-mime-video", Ditto. @@ +252,3 @@ + GdkPixbuf *thumbnail = NULL; + + if (!only_icon) { only_icon == FALSE @@ +260,3 @@ + if (!thumbnail) { + file_uri = g_file_new_for_uri (url_thumb); + fistream = g_file_read (file_uri, NULL, NULL); I would really rather not do sync calls here. @@ +1098,3 @@ + NULL); + + grl_plugin_registry_add_config_from_file (registry, config_file, NULL); Won't that also add providers that we know offer non-video/non-audio data (such as photos)? @@ +1172,3 @@ + +void +gw_spin_max_items_value_changed_cb (GtkSpinButton *spin, Why do we need a configuration item for this?
(In reply to comment #9) > +#define YOUTUBE_KEY > "AI39si7Ix-bf9xRLx5PK1sjsG2FQjSHblunuaI8YvHdYbkV2BcgOemHE6Nzb9t6LaMsekofKGSwgAETzbo7OvL8L3vH4vrFaqw" > +#define VIMEO_KEY "db924d3b7f1d5f3d6fdfeb4488ad01e2" > +#define VIMEO_SECRET "bef979d87f28ec47" > > Can't the keys be shipped somewhere else? Otherwise we'd need to do new > releases to support new backends, which makes the point rather moot. Yes. In fact, only the keys for the current set are hard-coded (that is, Vimeo and Youtube). But if you take a look to setup_config() function, the last line loads a configuration file that are applied to all plugins. In this configuration file one can add all the configuration options for the already existent plugin set, or for new plugins. This configuration options can include, of course the required API keys and secrets. > @@ +133,3 @@ > + MODEL_RESULTS_IS_PRETHUMBNAIL, > + MODEL_RESULTS_DESCRIPTION, > + MODEL_RESULTS_DURATION, > > NUM_RESULTS? and in the above as well, so we don't need to hard code the number > of columns. > Sorry, I don't get what you mean here. > @@ +136,3 @@ > +}; > + > +static guint max_items = MAX_ITEMS_DEFAULT; > > Why a global? > Because user can change the max. amount of results to get in the plugin configuration. Now that you are mentioning it, I don't store the current value, so everytime I user restarts Totem, the value is always initialized to the default MAX_ITEMS_DEFAULT. My plan is to change this behaviour, and rather than limiting the max. amount of results, just get a chunk of them, and when user scrolls down, get more results. > @@ +224,3 @@ > + const gchar *icon_name[] = { GTK_STOCK_DIRECTORY, > + "gnome-mime-audio", > + "gnome-mime-video", > > Ditto. What are the suggested names? > + > + grl_plugin_registry_add_config_from_file (registry, config_file, NULL); > > Won't that also add providers that we know offer non-video/non-audio data (such > as photos)? > No. This only sets up Grilo plugins configuration based on the content of the file. Regarding the load of image providers, right now there is no API in Grilo to filter sources by type of media provider (hope to add it in the short future). So right now, everytime a non-audio/non-video content is received, it is just filter out, and not shown in Totem. > @@ +1172,3 @@ > + > +void > +gw_spin_max_items_value_changed_cb (GtkSpinButton *spin, > > Why do we need a configuration item for this? Right now, when browsing/searching, only 300 results are obtained. This spin allows use to query more (or less) elements than those 300. Anyway, as I said, my plan is to get rid of it, and use the scrolling to get more results when user scrolls down. Regarding the other issues mentioned, I'll take care of them.
(In reply to comment #10) > (In reply to comment #9) > > +#define YOUTUBE_KEY > > "AI39si7Ix-bf9xRLx5PK1sjsG2FQjSHblunuaI8YvHdYbkV2BcgOemHE6Nzb9t6LaMsekofKGSwgAETzbo7OvL8L3vH4vrFaqw" > > +#define VIMEO_KEY "db924d3b7f1d5f3d6fdfeb4488ad01e2" > > +#define VIMEO_SECRET "bef979d87f28ec47" > > > > Can't the keys be shipped somewhere else? Otherwise we'd need to do new > > releases to support new backends, which makes the point rather moot. > > Yes. In fact, only the keys for the current set are hard-coded (that is, Vimeo > and Youtube). But if you take a look to setup_config() function, the last line > loads a configuration file that are applied to all plugins. In this > configuration file one can add all the configuration options for the already > existent plugin set, or for new plugins. This configuration options can > include, of course the required API keys and secrets. It only loads it from the user's config dir though: g_get_user_config_dir (), > > @@ +133,3 @@ > > + MODEL_RESULTS_IS_PRETHUMBNAIL, > > + MODEL_RESULTS_DESCRIPTION, > > + MODEL_RESULTS_DURATION, > > > > NUM_RESULTS? and in the above as well, so we don't need to hard code the number > > of columns. > > > > Sorry, I don't get what you mean here. Never mind the browser model is defined in the .ui file (gw_browse_store_results), not created in code. > > @@ +136,3 @@ > > +}; > > + > > +static guint max_items = MAX_ITEMS_DEFAULT; > > > > Why a global? > > > > Because user can change the max. amount of results to get in the plugin > configuration. > > Now that you are mentioning it, I don't store the current value, so everytime I > user restarts Totem, the value is always initialized to the default > MAX_ITEMS_DEFAULT. > > My plan is to change this behaviour, and rather than limiting the max. amount > of results, just get a chunk of them, and when user scrolls down, get more > results. Ok. > > @@ +224,3 @@ > > + const gchar *icon_name[] = { GTK_STOCK_DIRECTORY, > > + "gnome-mime-audio", > > + "gnome-mime-video", > > > > Ditto. > > What are the suggested names? See table 10: http://standards.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html > > + > > + grl_plugin_registry_add_config_from_file (registry, config_file, NULL); > > > > Won't that also add providers that we know offer non-video/non-audio data (such > > as photos)? > > > > No. This only sets up Grilo plugins configuration based on the content of the > file. > > Regarding the load of image providers, right now there is no API in Grilo to > filter sources by type of media provider (hope to add it in the short future). > > So right now, everytime a non-audio/non-video content is received, it is just > filter out, and not shown in Totem. Right. We don't want to offer an empty flickr stream to the user. > > @@ +1172,3 @@ > > + > > +void > > +gw_spin_max_items_value_changed_cb (GtkSpinButton *spin, > > > > Why do we need a configuration item for this? > > Right now, when browsing/searching, only 300 results are obtained. This spin > allows use to query more (or less) elements than those 300. > > Anyway, as I said, my plan is to get rid of it, and use the scrolling to get > more results when user scrolls down. > > > Regarding the other issues mentioned, I'll take care of them. Great.
(In reply to comment #10) > My plan is to change this behaviour, and rather than limiting the max. amount > of results, just get a chunk of them, and when user scrolls down, get more > results. Note that the YouTube plugin already does this, and it should be fairly easy to copy the code for that across to the Grilo plugin.
> > Yes. In fact, only the keys for the current set are hard-coded (that is, Vimeo > > and Youtube). But if you take a look to setup_config() function, the last line > > loads a configuration file that are applied to all plugins. In this > > configuration file one can add all the configuration options for the already > > existent plugin set, or for new plugins. This configuration options can > > include, of course the required API keys and secrets. > > It only loads it from the user's config dir though: > g_get_user_config_dir (), Yep. Now that you're mentioning it, probably it would be a good idea adding also a system-wide configuration file with the options we want to use for all users. This configuration file could store for instance API keys for new plugins. The file could be distributed with totem-grilo plugin. Thus, we would have a system and user wide configuration files.
Created attachment 188140 [details] [review] Grilo plugin (v2) Attached it is a new version addressing the issues commented, and also improved in other non-commented issues. Main issues solved are: * Thumbnails streams are read asynchronously, which improved a lot responsiveness. * A global conf file has been added. This global conf file contains the api keys for Youtube and Vimeo plugin, but can store any other configuration element needed by the Grilo plugins. * Got rid of max_items configuration. Now it behaves similar to totem-youtube plugin: scrolling down in search/browse gets more elements.
Against which version did you create this patch? It doesn't apply cleanly against master.
The plugin registry loading is also broken. (totem:14281): Grilo-WARNING **: [plugin-registry] grl-plugin-registry.c:294: Could not open plugins' info directory '/home/hadess/Projects/gnome-install/share/grilo-0.1/plugins': Error opening directory '/home/hadess/Projects/gnome-install/share/grilo-0.1/plugins': No such file or directory (totem:14281): Grilo-WARNING **: [plugin-registry] grl-plugin-registry.c:610: Could not open plugin directory: '/home/hadess/Projects/gnome-install/lib64/grilo-0.1' Well, yes, Grilo is installed system-wide, not in the prefix.
I think there are also a number of problems within the plugin itself. It shouldn't start loading the available sources unless the browse sidebar is shown. Currently, I get a crash on startup because it crashes within the YouTube plugin. The browse sidebar isn't selected, and I haven't browsed within the YouTube hierarchy either.
+ Trace 227191
(In reply to comment #17) > Currently, I get a crash on startup because it crashes within the YouTube > plugin. The browse sidebar isn't selected, and I haven't browsed within the > YouTube hierarchy either. That's bug #650401 which has already been fixed in libgdata 0.8.1 (released) and master (it's not Grilo's fault). If you're running either of those versions of libgdata, let me know and I'll investigate further.
(In reply to comment #18) > (In reply to comment #17) > > Currently, I get a crash on startup because it crashes within the YouTube > > plugin. The browse sidebar isn't selected, and I haven't browsed within the > > YouTube hierarchy either. > > That's bug #650401 which has already been fixed in libgdata 0.8.1 (released) > and master (it's not Grilo's fault). If you're running either of those versions > of libgdata, let me know and I'll investigate further. I'm updating libgdata now. Doesn't stop the fact that it shouldn't be loading the YouTube feeds if I haven't actually browsed them.
(In reply to comment #15) > Against which version did you create this patch? It doesn't apply cleanly > against master. I've made it against gnome-3-0 branch.
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > Currently, I get a crash on startup because it crashes within the YouTube > > > plugin. The browse sidebar isn't selected, and I haven't browsed within the > > > YouTube hierarchy either. > > > > That's bug #650401 which has already been fixed in libgdata 0.8.1 (released) > > and master (it's not Grilo's fault). If you're running either of those versions > > of libgdata, let me know and I'll investigate further. > > I'm updating libgdata now. Doesn't stop the fact that it shouldn't be loading > the YouTube feeds if I haven't actually browsed them. It loads them because inmediately after loading the Youtube source, it creates the browse content hierarchy, that is, the available categories and the #children on each of them, so it doesn't to do it again in the future. Thus, when user starts to browse he doesn't need to wait until getting that category, because it was already obtained.
Review of attachment 188140 [details] [review]: Here are some review comments looking at the code from a micro level rather than a macro level. I haven't had a chance to run it yet, so these are just from inspection. ::: src/plugins/grilo/totem-grilo.c @@ +78,3 @@ + ICON_VIDEO, + ICON_DEFAULT +}; typedef this and use the type name instead of gint elsewhere in the code to make things a little clearer. @@ +87,3 @@ + + /* Thumb cache to speed up loading */ + GHashTable *cache_thumbnails; It might be helpful to comment on the types the hash table maps from and to (e.g. “string → GdkPixbuf”). @@ +204,3 @@ + + if (pixbuf[icon_type] == NULL) { + screen = gdk_screen_get_default (); Should this not be getting the screen for Totem's main window? @@ +280,3 @@ + + url_thumb = grl_media_get_thumbnail (media); + if (url_thumb) { url_thumb != NULL @@ +291,3 @@ + thumb_data->media = g_object_ref (media); + thumb_data->file = g_object_ref (file_uri); + thumb_data->iter = gtk_tree_iter_copy (iter); Is it possible for the data in the tree to change between copying this iter and using it in get_stream_thumbnail_cb()? If so, the iter will be invalid and GTK+ will complain; you should use a GtkTreeRowReference instead. @@ +409,3 @@ + parent = bud->parent; + + if (error && error != NULL @@ +413,3 @@ + GRL_CORE_ERROR, + GRL_CORE_ERROR_OPERATION_CANCELLED) == FALSE) { + g_critical ("Error: %s", error->message); Should this be a user-visible error dialogue instead? What sort of errors can occur here? @@ +416,3 @@ + } + + if (media) { media != NULL @@ +426,3 @@ + /* Filter images */ + if (GRL_IS_MEDIA_IMAGE (media)) { + g_object_unref (media); You could move the g_object_unref(media) out of both branches of this if statement and put it after them to make the control flow a little simpler. @@ +449,3 @@ + + g_object_unref (media); + if (thumbnail) { thumbnail != NULL @@ +455,3 @@ + g_free (pretty_duration); + + GtkTreePath *path = gtk_tree_model_get_path (self->priv->browser_model, parent); Declarations need to be at the beginning of the block. path also needs to be freed. @@ +474,3 @@ + gint page) +{ + if (source) { source != NULL @@ +477,3 @@ + BrowseUserData *bud = g_slice_new (BrowseUserData); + bud->totem_grilo = g_object_ref (self); + bud->parent = gtk_tree_iter_copy (parent); Again, you might need to be using GtkTreeRowReferences instead of iters here. @@ +513,3 @@ + const gchar *url; + const GList *slow_keys; + GList *url_keys; slow_keys and url_keys could be moved into the block they're used in for clarity. @@ +526,3 @@ + if (resolve_url) { + slow_keys = grl_metadata_source_slow_keys (GRL_METADATA_SOURCE (source)); + if (g_list_find ((GList *) slow_keys, GRL_METADATA_KEY_URL)) { g_list_find (…) != NULL @@ +558,3 @@ + GRL_CORE_ERROR, + GRL_CORE_ERROR_OPERATION_CANCELLED) == FALSE) { + g_critical ("Error: %s", error->message); What sort of error messages can appear here? Should they be presented to the user? @@ +608,3 @@ +{ + gtk_widget_set_sensitive (self->priv->search_search_button, FALSE); + gtk_widget_set_sensitive (self->priv->search_cancel_button, TRUE); Should any ongoing searches be cancelled at this point, so that we don't end up with a conflation of results from both? @@ +705,3 @@ + -1); + + if (content && content != NULL @@ +887,3 @@ + gtk_list_store_clear (GTK_LIST_STORE (self->priv->search_results_model)); + self->priv->search_source = NULL; + } If it's possible for a search to be ongoing against the source at this point, you should probably cancel the search first too. @@ +1112,3 @@ +{ + self->priv->browser_model = g_object_ref (gtk_builder_get_object (builder, "gw_browse_store_results")); + self->priv->browser = g_object_ref (gtk_builder_get_object (builder, "gw_browse")); There's no need to ref these widgets; once they're inserted into the widget hierarchy with totem_add_sidebar_page(), they'll be referenced by their container widgets. @@ +1126,3 @@ + self); + + totem_add_sidebar_page (self->priv->totem, "grilo-browse", "Browse", “Browse” should be translatable. @@ +1140,3 @@ + self->priv->search_search_button = g_object_ref (gtk_builder_get_object (builder, "gw_search_search_button")); + self->priv->search_cancel_button = g_object_ref (gtk_builder_get_object (builder, "gw_search_cancel_button")); + self->priv->search_entry = g_object_ref (gtk_builder_get_object (builder, "gw_search_text")); No need to ref these either. @@ +1182,3 @@ + self); + + totem_add_sidebar_page (self->priv->totem, "grilo-search", "Search", “Search” should be translatable. @@ +1202,3 @@ + + url = grl_media_get_url (self->priv->selected_media); + if (url) { url != NULL
(In reply to comment #22) > > @@ +413,3 @@ > + GRL_CORE_ERROR, > + GRL_CORE_ERROR_OPERATION_CANCELLED) == FALSE) { > + g_critical ("Error: %s", error->message); > > Should this be a user-visible error dialogue instead? What sort of errors can > occur here? > > > @@ +558,3 @@ > + GRL_CORE_ERROR, > + GRL_CORE_ERROR_OPERATION_CANCELLED) == FALSE) { > + g_critical ("Error: %s", error->message); > > What sort of error messages can appear here? Should they be presented to the > user? > Very likely. These errors are from the plugins, when they are not able to perform the required tasks. Examples can be unable to connect to the web service, unable to parse the XML with results, and so forth.
Review of attachment 188140 [details] [review]: ::: src/plugins/grilo/totem-grilo.c @@ +608,3 @@ +{ + gtk_widget_set_sensitive (self->priv->search_search_button, FALSE); + gtk_widget_set_sensitive (self->priv->search_cancel_button, TRUE); It shouldn't happen. Search is only invoked either when user press the "Search" button or when scrolls down the list of results. While a search is in progress, the "Search" button is disabled, so user can't press the button. And when user scrolls down, it is checked if a search is actually in progress, and if so, then it doesn't start a new search, but keeps the current one. So unless I'm missing some race-condition, it shouldn't mix the results.
(In reply to comment #24) > Review of attachment 188140 [details] [review]: > > ::: src/plugins/grilo/totem-grilo.c > @@ +608,3 @@ > +{ > + gtk_widget_set_sensitive (self->priv->search_search_button, FALSE); > + gtk_widget_set_sensitive (self->priv->search_cancel_button, TRUE); > > It shouldn't happen. > > Search is only invoked either when user press the "Search" button or when > scrolls down the list of results. > > While a search is in progress, the "Search" button is disabled, so user can't > press the button. And when user scrolls down, it is checked if a search is > actually in progress, and if so, then it doesn't start a new search, but keeps > the current one. > > So unless I'm missing some race-condition, it shouldn't mix the results. I forgot to mention this is a reply for comment: "Should any ongoing searches be cancelled at this point, so that we don't end up with a conflation of results from both?"
(In reply to comment #22) > @@ +887,3 @@ > + gtk_list_store_clear (GTK_LIST_STORE > (self->priv->search_results_model)); > + self->priv->search_source = NULL; > + } > > If it's possible for a search to be ongoing against the source at this point, > you should probably cancel the search first too. > When a source is removed, pending operations should be stopped. That's in theory. In practice, at least with UPnP source, you get a crash :( I've bug #650974 to cope with this issue.
(In reply to comment #26) > When a source is removed, pending operations should be stopped. That's in ^^^^^^^ automatically > theory. In practice, at least with UPnP source, you get a crash :( > > I've bug #650974 to cope with this issue.
Created attachment 188479 [details] [review] Grilo plugin (v3) New version of Grilo plugin. This patch applies on top of master. I've address almost all issues mentioned in the reviews. Thanks to the reviewers. Some of those issues have been fixed in Grilo, while others have been filed as Grilo bugs. Hope to fix them soon.
Created attachment 189550 [details] sidebar mockup This is the mockup we'll have now: - no more "tabs" drop-down menu, instead each sidebar will have its own menu item (with potential keyboard shortcut, just as the Properties tab has) - no "search" button in the search sidebar, we'll handle "return" instead - the widget to use for search results is probably GtkIconView I'll be working on the first item now.
I don't mean to side track the conversation but to comment on your mockup and sidebars in general: When the user opens the sidebar in the future UI design, could you make it so that the vertical height of totem does not change and things get squeezed into the current vertical height? At the moment in the version present in F15 and earlier, the sidebar must have a different minimum height or different means of determining the height of the entire window and on a small screen (like a netbook), when the sidebar is opened, it nearly pushes the control buttons off the bottom of the screen. There are a number of problems similar to this in Gnome 3. I was thinking of starting some sort of tracker bug for small screen issues and then separating each particular problem into their own bug reports. Would this be worthwhile to the Gnome devs?
(In reply to comment #30) > I don't mean to side track the conversation but to comment on your mockup and > sidebars in general: > > When the user opens the sidebar in the future UI design, could you make it so > that the vertical height of totem does not change and things get squeezed into > the current vertical height? > > At the moment in the version present in F15 and earlier, the sidebar must have > a different minimum height or different means of determining the height of the > entire window and on a small screen (like a netbook), when the sidebar is > opened, it nearly pushes the control buttons off the bottom of the screen. > > There are a number of problems similar to this in Gnome 3. I was thinking of > starting some sort of tracker bug for small screen issues and then separating > each particular problem into their own bug reports. Would this be worthwhile to > the Gnome devs? File a separate bug about the problem. It's no place to discuss it here.
A couple more UI bugs: - When you switch to the search tab, the combo box drop-down is big, then becomes smaller as items are added to it. Bad. - The widget organisation (removing the buttons, drop-down and search entry side-by-side) needs to be done before it's merged. - The presentation widget for the search results could be a GtkIconView, that would allow for an easy reflow when showing results, and making it bigger. - We need to have an easy way to blacklist plugins we don't want to see, both in the search and the browse sidebars (filesystem is one we don't want to see in browse for example) Once that's done, we can merge the patch, and carry on working on UI problems, or bugs.
Oh, and add: +Builtin=true to the grilo.plugin.in please.
(In reply to comment #32) > - We need to have an easy way to blacklist plugins we don't want to see, both > in the search and the browse sidebars (filesystem is one we don't want to see > in browse for example) > What about this? Add a Grilo configuration screen to Totem where a list of all available sources are shown, and user can blacklist them. So this blacklisted sources are not loaded. If new sources appears in the future, by default they will be whitelisted, and user always can go to the configuration screen and blacklist them. So in future plugin will remind to not load them.
I don't think we need configuration for this. I would want a hard-coded list of blacklisted plugins within Totem itself.
(In reply to comment #35) > I don't think we need configuration for this. I would want a hard-coded list of > blacklisted plugins within Totem itself. Well, I'm proposing this because if new plugins arrive to the system, maybe user do not want to show them in the list.
(In reply to comment #32) > A couple more UI bugs: > - When you switch to the search tab, the combo box drop-down is big, then > becomes smaller as items are added to it. Bad. Uhm.. I can't reproduce this issue. I do not appreciate any change in the combobox. > - The widget organisation (removing the buttons, drop-down and search entry > side-by-side) needs to be done before it's merged. > Some questions regarding this: - In the mockup, besides the combobox it says "Web Search". Should I add that message in the plugin, or is something that Totem will do? And if I need to do, I think "Search" instead of "Web Search" fits better (not all content come from Web; see UPnP or Tracker) - There are no more "Search" button, but handling it with 'Return'. In the mockup, I see a small magnifying glass at the right. Clicking on that glass should perform the search too? - I don't see a "Cancel" button. Does that mean that search is not cancellable? Note that even in this case, search is done in chunks, so you will never receive all the results in a single step. - In current plugin, each result is shown with its duration. I don't see that duration in the mockup. Should I get rid of it then? - In the case of some sources, when artist/author is available, this field is shown with the title. I don't see this in the current mockup. Should I keep this way of showing artist+title in the rework? - Just to confirm: in the mockup results are shown in two columns. I guess I must do the same, right?
(In reply to comment #36) > (In reply to comment #35) > > I don't think we need configuration for this. I would want a hard-coded list of > > blacklisted plugins within Totem itself. > > Well, I'm proposing this because if new plugins arrive to the system, maybe > user do not want to show them in the list. We'll change Totem, not a problem. (In reply to comment #37) > (In reply to comment #32) > > A couple more UI bugs: > > - When you switch to the search tab, the combo box drop-down is big, then > > becomes smaller as items are added to it. Bad. > > Uhm.. I can't reproduce this issue. I do not appreciate any change in the > combobox. I'll try and reproduce the problem later. > > - The widget organisation (removing the buttons, drop-down and search entry > > side-by-side) needs to be done before it's merged. > > > > Some questions regarding this: > > - In the mockup, besides the combobox it says "Web Search". Should I add that > message in the plugin, or is something that Totem will do? And if I need to do, > I think "Search" instead of "Web Search" fits better (not all content come from > Web; see UPnP or Tracker) It probably would just say "Search" as a tab title. > - There are no more "Search" button, but handling it with 'Return'. In the > mockup, I see a small magnifying glass at the right. Clicking on that glass > should perform the search too? No, it would behave like the "search" entry in the "system settings" overview. Secondary icon is search disabled by default, and if there's text, the 2ndary icon will be a "<X|" erase icon. > - I don't see a "Cancel" button. Does that mean that search is not cancellable? > Note that even in this case, search is done in chunks, so you will never > receive all the results in a single step. I don't think we really need a cancel, as long as we don't carry on loading stuff in the background when there's no user activity (eg. we would probably cancel a search if you started erasing text in the search entry). > - In current plugin, each result is shown with its duration. I don't see that > duration in the mockup. Should I get rid of it then? Yep. A simple title (Artist - Title for music) should be enough. > - In the case of some sources, when artist/author is available, this field is > shown with the title. I don't see this in the current mockup. Should I keep > this way of showing artist+title in the rework? Not as separate items. > - Just to confirm: in the mockup results are shown in two columns. I guess I > must do the same, right? Not 2 columns, using a GtkIconView, we can reflow if we get more space, so the only thing fixed is the thumbnail size).
Created attachment 190245 [details] [review] Grilo plugin (v4) Updated Grilo plugin using the design in the mockup
Created attachment 190303 [details] [review] Grilo plugin (v5)
1) Putting garbage in the search box I get: " Search Error No object satisfies given search criteria. " This really needs to be presented differently. And my thinking is also that the API shouldn't return an error for a lack of matches. 2) The icon sizing seems broken in the search results. The music icons are huge, but the other icons are small. 3) I'm not sure we should be showing directories in the search results when we can't navigate inside them. Only match media, not containers in the searches. 4) Switching "search" source (from UPnP server to Podcast for example) should clear the search entry and the search results. 5) The "Browse" treeview is missing a header (see the Playlist). 6) The "Browse" treeview is missing a call to gtk_tree_view_columns_autosize() so that the scrollbars disappear when they're not used anymore.
(In reply to comment #41) > 1) Putting garbage in the search box I get: > " > Search Error > No object satisfies given search criteria. > " > > This really needs to be presented differently. And my thinking is also that the > API shouldn't return an error for a lack of matches. Weird. Grilo API sends no error when there are no results. The situation described seems like the backend is returning an error when searching that garbage, and Grilo forwards that error to the client. Can you tell me which source are you looking, and the garbage string? > 2) The icon sizing seems broken in the search results. The music icons are > huge, but the other icons are small. > Uhm... I didn't notice it. The music icons seems to have the same size as the video icons. Anyway, I did a small change in code that maybe can fix this issue. I'll upload it in the next version of plugin. > 3) I'm not sure we should be showing directories in the search results when we > can't navigate inside them. Only match media, not containers in the searches. > Sources should only send media items on search. Can you tell me which source is the offending one?
commit 2c31c7bd3db1d3156c0c7ea18832b03a2920c591 Author: Juan A. Suarez Romero <jasuarez@igalia.com> Date: Mon Jun 20 18:19:46 2011 +0000 plugins: add Grilo plugin https://bugzilla.gnome.org/show_bug.cgi?id=628648 Will file bugs for the rest.
(In reply to comment #41) > 1) Putting garbage in the search box I get: > " > Search Error > No object satisfies given search criteria. > " > > This really needs to be presented differently. And my thinking is also that the > API shouldn't return an error for a lack of matches. bug 653756 > 2) The icon sizing seems broken in the search results. The music icons are > huge, but the other icons are small. Will need to try and fix this properly later on. > 3) I'm not sure we should be showing directories in the search results when we > can't navigate inside them. Only match media, not containers in the searches. Bug 653759 > 4) Switching "search" source (from UPnP server to Podcast for example) should > clear the search entry and the search results. Not the search entry, but it cancels the search, and clears the results. > 5) The "Browse" treeview is missing a header (see the Playlist). Fixed. > 6) The "Browse" treeview is missing a call to gtk_tree_view_columns_autosize() > so that the scrollbars disappear when they're not used anymore. Fixed.