Created attachment 159873 [details] [review]
Add gtk_recent_info_get_gicon()

Bug #617174.

Created attachment 159874 [details] [review]
Use gtk_recent_info_get_gicon()

Bug #617174.

(In reply to comment #0)
> gtk_recent_info_get_gicon should be added as a convenience function that gets
> the GIcon for the item's mime type.

looks nice, and I definitely want to introduce more GIO into GtkRecentManager.

should gtk_recent_item_get_icon() be deprecated? it would make sense if we're exposing GIcon for MIME type and thumbnails.

(In reply to comment #2)
> Created an attachment (id=159874) [details] [review]
> Use gtk_recent_info_get_gicon()
> 
> Bug #617174.

looks okay; why did you tag it as 'needs-work'?

(In reply to comment #3)
> (In reply to comment #0)
> > gtk_recent_info_get_gicon should be added as a convenience function that gets
> > the GIcon for the item's mime type.
> 
> looks nice, and I definitely want to introduce more GIO into GtkRecentManager.
> 
> should gtk_recent_item_get_icon() be deprecated? it would make sense if we're
> exposing GIcon for MIME type and thumbnails.

Yes, I think it should be deprecated.

> (In reply to comment #2)
> > Created an attachment (id=159874) [details] [review] [details] [review]
> > Use gtk_recent_info_get_gicon()
> > 
> > Bug #617174.
> 
> looks okay; why did you tag it as 'needs-work'?

It had a problem with the icon size in gtkrecentchooserdefault.c where I misinterpreted the code. Also I noticed that more code could be removed from gtkrecentchoosermenu.c. Updated patch attached.

Created attachment 159910 [details] [review]
Use gtk_recent_info_get_gicon()

Bug #617174.

Created attachment 159911 [details] [review]
Deprecate gtk_recent_info_get_icon()

Bug #617174.

Attachment 159873 [details] pushed as b0fe3e4 - Add gtk_recent_info_get_gicon()

The other patches need to be rebased against a recent master.

Since we're breaking API, we should just remove get_icon() in gtk+-3.0 and deprecate it in gtk-2-24; before doing that, though, I want to make sure that the fallback icons are still in place.

While I don't disagree with this API I'd like to point out that always using the mimetype for the icon is problematic. We have similar issues in nautilus with bookmarks. We try to resolve the icon in a wider way, also handling local custom icons or site/mount specific favicon things. However, this causes i/o on the location just to show the bookmark item.

So, I'd like to add a set_icon() as well, so that the icon can be stored when creating the recent item and be recreated without having to do i/o.

(In reply to comment #9)

> So, I'd like to add a set_icon() as well, so that the icon can be stored when
> creating the recent item and be recreated without having to do i/o.

currently, each item in a BookmarkFile can have a custom icon set, by using g_bookmark_file_set_icon(); the icon is specified by a URL, so it can be a local or a remote file. storing the icon data inside the recent files list might not be desirable, as it would add a cost to reading/parsing for every application, but local I/O to a cache directory storing custom icons might be just fine.

GtkRecentManager does not expose this part of the low-level API, but it should be possible to do it.

I didn't mean the icon data, just the resolved icon, so that you don't have to do i/o to calculate it.

A uri is sorta bad though, as we generally want to store at least a themed icon name, even better a list of icon names, and ideally a fully serialized (g_icon_to_string) GIcon (including e.g. emblemed icons).

(In reply to comment #11)
> I didn't mean the icon data, just the resolved icon, so that you don't have to
> do i/o to calculate it.
> 
> A uri is sorta bad though, as we generally want to store at least a themed icon
> name, even better a list of icon names, and ideally a fully serialized
> (g_icon_to_string) GIcon (including e.g. emblemed icons).

a name from the icon theme is also acceptable:

"""
The icon element, if present, specifies the icon that should be associated to the bookmark. The icon element must have the following attributes:

type
  The MIME type, belonging to the image/* family of MIME types, of the icon. If no type is set, implementors should use the application/octect-stream MIME type

href
  The URI to the icon file

name
  The name of the icon inside an icon theme, following the Icon Naming Specification
"""

so it's perfectly legal to specify the name of the icon.

Most GIcons are lists of icon names (fallbacks) though...

(In reply to comment #13)
> Most GIcons are lists of icon names (fallbacks) though...

I think we can easily enough amend the spec to allow a comma-separated list of icons and support fallbacks.

That would be good.

*** Bug 547305 has been marked as a duplicate of this bug. ***

We did not change the GtkRecentData structure to allow for an icon, or an icon list with fallbacks; and the spec has not been amended.

GtkRecentInfo is a read-only data structure, and it would be awkward to add a setter to it; the API would also look weird:

  gtk_recent_manager_add_item (...);
  [ wait until the data is written to storage ]
  item = gtk_recent_manager_lookup_item (...);
  gtk_recent_info_set_icon (item, ...)

I guess this last bit goes on the WONTFIX pile.