Ah. A patch would be welcome.

My pleasure, once I've got the time to get a source tree going.

Created attachment 86586 [details] [review]
Proposed patch

This makes a std::vector<Glib::ustring> out of the StringArrayHandle. It also changes the add_item function to be hand-written, because we need to fill in a C struct.

A don't think however that RecentManager::Data is a convenient API. Basically, one has to fill out a struct as one would in C. We probably cannot remove it anymore, but how about adding some more add_item() overloads that take things like display name, description, mime type etc. as parameter?

We can't change the ABI or API of that struct, though maybe that's OK if nobody could possibly be using it.

I agree about the API, but I worry that a method with many parameters is ugly and less explicit when you read the code, and that also does not allow any parameter to be empty.

I guess we should use this in a test case or example in some sensible way. But I don't know when people should/want to use gtk_recent_manager_add_full().

Thanks for doing this, Armin!

Indeed, we can change the Data API all we want since it was really non-instantiable before this patch. I'm not sure the best way to do this, but I'll point out that RecentManager returns RecentInfo's. Why shouldn't we pass in what we'll get out? This might make RecentInfo kind of strange, since some of its metadata depends on it having been added to a RecentManager already. But yeah, doesn't make much sense to pass in a dummy struct and get out a fancy object. Also note that GtkRecentData allows some data elements to be undefined; display_name and description can both be NULL if we don't want to specify them. Whatever the API winds up being, it should also allow such undefined elements. 

I think it is safe to change the groups field in the Data struct, since it could not have been used anyway.

The mime_type, app_name and app_exec fields are required nevertheless, so they cannot be empty. The display name could be empty in theory, but that would require that we pass NULL to the C function which is not possible when using Glib::ustring. Pretty the same applies to the description field, but I think NULL is treated the same as the empty string here.

I therefore tried adding two add_item() overloads, one with a StringArrayHandle for the groups field and one without, but this is rather difficult because the compiler cannot decide which one to use if it gets a const  char* argument for the field which can either be a ustring or the StringArrayHandle. It would probably work when the StringArrayHandle was the last parameter, but I am no longer fully convinced by this because one has to take pretty much care that all possible use cases work as expected.

RecentInfo seems to be something different from RecentData because it points to an existing recent item while RecentData just contains data to create a new item. You are probably not supposed to create a RecentInfo yourself. There is also already a difference in GTK+ between those two.

>  The display name could be empty in theory, but that would
> require that we pass NULL to the C function which is not possible when using
> Glib::ustring.

We can just do a (str.empty ? str.c_str() : NULL).

> the compiler cannot decide which one to use if it gets a const  char* argument > for the field which can either be a ustring or the StringArrayHandle.

Yes, so we would need to distinguish the methods by using a slightly different method name.

So, what is your opinion on this? Should I just commit that patch? We can still add more API in the future when someone points out an actual use case for gtk_recent_manager_add_full().

The patch seems OK, though we might want to add those extra method overloads. However, I have not yet decided whether we should commit this before branching gtkmm.

It's hard to judge its importance without knowing when gtk_recent_manager_add_full() (which it wraps) is needed. Grant, can you tell us if this is actually very useful to you?

(In reply to comment #9)
> It's hard to judge its importance without knowing when
> gtk_recent_manager_add_full() (which it wraps) is needed. Grant, can you tell
> us if this is actually very useful to you?

A ran into this bug while implementing a most-recently-used list for VMware Workstation and Player. I've got a virtual machine file path, name, MIME type, app name, and command line. is_private is false and I have no use for group names.

Currently (and I did this a while ago, so it's all a little hazy) I use the GTK+ API to add each entry. I have to use those const_casts as you're doing in the patch, which is assuredly a GTK+ bug as gtk_recent_manager_add_full treats (and ought to treat) all fields in the data struct as const anyway.

I don't mind creating a Data object and filling in the fields that interest me. Functions would be nice, and would require the caller to include those fields that are required in the GTK+ API. (If the patch was submitted as-is, a caller could create a Data object and just pass it to add_full uninitialized, with a bunch of empty strings. Not desirable, and not possible in GTK+.) However the permutations of included/unincluded arguments are many, and using different names to differentiate these sounds like a real mess. I'd just like to see the Data class instantiable; maybe we can think about revising the RecentManager scheme in general at a later date.

OK. Please apply this patch. It doesn't look like we can branch gtkmm soon enough (we need a coresponding GTK+ branch), so I don't want to wait too long to fix this.

By the way, I found this sentence in the GtkRecentFiles overview documentation, which we should put in the documentation for these 2 methods themselves, and maybe in our class documentation.
"
The gtk_recent_manager_add_item() function will try and guess some of the meta-data associated to a URI. If you know some of meta-data about the document yourself, set the desired fields of a GtkRecentData structure and pass it to the gtk_recent_manager_add_full() function instead:
"

Committed. I also added a similar documentation comment to the two functions.