GNOME Bugzilla – Bug 676120
The hint bars aren't very nice
Last modified: 2012-08-14 19:10:32 UTC
Created attachment 214140 [details] screenshot We can do a lot better with the bars that show up to suggest you something besides browse a removable device.
Created attachment 214143 [details] wireframe
Created attachment 214497 [details] [review] Only place one x-content-bar at a time
Created attachment 214498 [details] [review] Simplify the content type labels
The app icons aren't shown currently because we disable showing icons in all buttons. Perhaps we should add a property to gtkbutton to allow forcing the icon to show?
Created attachment 214503 [details] [review] Always show icon in application launchers
Review of attachment 214497 [details] [review]: Makes a lot of sense; I have some comments on the code: ::: src/nautilus-x-content-bar.c @@ +190,3 @@ + + if (g_content_type_is_a (bar->priv->x_content_types[n], "x-content/win32-software")) + continue; I think these filters on the values of x_content_types should be applied before assigning the value to priv->x_content_types, since the number of valid content types will influence the string shown by the bar. I think it's OK not to show the bar (e.g. make it commit harakiri by calling gtk_widget_destroy() on itself) when after filtering, we find no useful content types to display, as you hint to in a comment above. @@ +312,3 @@ g_object_class_install_property (object_class, + PROP_X_CONTENT_TYPES, + g_param_spec_variant ( Why GVariant? Since you don't seem to store the variant anyway (but you do store the strv), I think it's better to just use g_param_spec_boxed with G_TYPE_STRV here (and the part above in _set/get_property would just use g_value_set/get_boxed())
Review of attachment 214498 [details] [review]: Looks good; maybe add a comment for translator to disambiguate the context of those "Contains..." strings?
Review of attachment 214503 [details] [review]: Provided the GTK patch is accepted, looks good.
Created attachment 214573 [details] [review] Only place one x-content-bar at a time
Created attachment 214574 [details] [review] Simplify the content type labels
Created attachment 214575 [details] [review] Always show icon in application launchers
Created attachment 214576 [details] screenshot with patches
Review of attachment 214573 [details] [review]: ::: src/nautilus-x-content-bar.c @@ +166,3 @@ + default_app = g_app_info_get_default_for_type (x_content_types[n], FALSE); + if (default_app == NULL) + message = get_message_for_x_content_type (x_content_types[0]); You're leaking default_app here. Since you will iterate again on the GPtrArray later calling again g_app_info_get_default_for_type() on each of the types, I think it's cleaner if you do: GPtrArray *types, *apps; for (n = 0; x_content_types[n] != NULL; n++) { // add the type to types and the default_app to apps if they're valid } and then iterate on the apps array to add buttons later.
Review of attachment 214574 [details] [review]: Looks good.
Review of attachment 214575 [details] [review]: [ Need to wait for the GTK patch ]
Created attachment 214578 [details] [review] Only place one x-content-bar at a time
Review of attachment 214578 [details] [review]: Looks good.
Attachment 214574 [details] pushed as 631a070 - Simplify the content type labels Attachment 214575 [details] pushed as f02cafd - Always show icon in application launchers Attachment 214578 [details] pushed as dde03f7 - Only place one x-content-bar at a time
Just a note: the translator comments will not appear in PO files, because they are not placed exactly before _(). If you want to show them in PO files (could be good) it should be /* Customize greeting for well-known x-content types */ - /* translators: these describe the contents of removable media */ if (strcmp (x_content_type, "x-content/audio-cdda") == 0) { + /* translators: these describe the contents of removable media */ message = g_strdup (_("Audio CD"));
*** Bug 665327 has been marked as a duplicate of this bug. ***
*** Bug 602366 has been marked as a duplicate of this bug. ***