What's exactly the problem? why .img files? what does broken mean? the missing image icon? we are supposed to fallback to package icon when we don't know the mime type

(In reply to Carlos Garcia Campos from comment #1)
> we are supposed to fallback to package icon when we
> don't know the mime type

The problem is we do know the MIME type. We pass it to g_content_type_get_symbolic_icon, then if g_content_type_get_symbolic_icon returns NULL we fall back to using package-x-generic-symbolic. But the return value of g_content_type_get_symbolic_icon never is not nullable, so this is wrong. If GLib can't find a reasonable icon, it returns a broken image icon (screenshot attached).

Created attachment 322657 [details]
screenshot

Created attachment 322704 [details] [review]
download-widget: Fix icon for .img files

If the file is an .img file, a broken image icon is displayed instead
of the package-x-generic-symbolic. This happens because GLib can't find
a proper icon for the given content type.

Check the icon name and fall back to using package-x-generic-symbolic
if the icon name matches the "broken image" icon's name.

Created attachment 322705 [details] [review]
download-widget: Fix icon for .img files

Oops

Review of attachment 322705 [details] [review]:

Thanks

::: lib/widgets/ephy-download-widget.c
@@ +56,3 @@
 static GParamSpec *obj_properties[LAST_PROP];
 
+#define GENERIC_APPLICATION_ICON_NAME "application-x-generic"

Since you only use this once, I would just hardcode it at the point of use, like we already do for "package-x-generic-symbolic." Don't tell your programming teacher.

@@ +144,3 @@
+    /* If GLib can't find an icon for the given content type, it returns a
+     * broken image icon. If that's the case, fall back to using
+     * package-x-generic-symbolic. */

I prefer a style with the closing * of the */ in the same column as the rest of the *s:

/*
 *
 *
 */

@@ +147,3 @@
+    generic_icon_name = g_content_type_get_generic_icon_name (content_type);
+    if (g_strcmp0 (generic_icon_name, GENERIC_APPLICATION_ICON_NAME) == 0)
+      icon = g_icon_new_for_string ("package-x-generic-symbolic", NULL);

We can simplify this to get rid of the redundant "package-x-generic-symbolic" lines:

if (content_type) {
  // Declare it here for reduced scope
  char *generic_icon_name = g_content_type_get_generic_icon_name (content_type);

  if (g_strcmp0 (generic_icon_name, "application-x-generic") != 0)
    icon = g_content_type_get_symbolic_icon (content_type);

  g_free (generic_icon_name);
}

if (!icon)
  icon = g_icon_new_for_string ("package-x-generic-symbolic", NULL);

(In reply to Michael Catanzaro from comment #3)
> Created attachment 322657 [details]
> screenshot

That's the image missing icon, the fallback GtkImage uses when it can't find the icon in the theme

Review of attachment 322705 [details] [review]:

I don't think this is the right fix, it looks like a generic workaround for a very specific problem. Let me explain what I think is going on here. The content type of .img is detected as application/x-raw-disk-image. g_content_type_get_symbolic_icon() creates a GThemedIcon with application-x-raw-disk-image-symbolic, and the generic icon application-x-generic-symbolic (because this is application-). When GtkImage tries to render the GIcon it doesn't find any of the icons in the theme, nor the fallbacks either (the non symbolic ones) and ends up falling back to missing-image. So, I think there isn't any bug here, we just need an icon for either application-x-raw-disk-image or application-x-generic in the theme. But since we always fallback to package-x-generic-symbolic for downloads I think it makes sense to try to prevent the missing icon from being shown and use always the package one. In that case we need a workaround for any missing icon, not just the ones starting with application-.

Created attachment 322720 [details] [review]
Different approach

I think this is simpler and should fix any other cases.

Review of attachment 322720 [details] [review]:

Yeah, I was confused by the application- prefix on the icon too; I should probably not review patches when I'm tired....

::: lib/widgets/ephy-download-widget.c
@@ +147,3 @@
+       * missing in the theme.
+       */
+      g_themed_icon_append_name (G_THEMED_ICON (icon), "package-x-generic-symbolic");

OK, I didn't realize you could do this. Nice.