Created attachment 289339 [details] [review]
patch: Add Gio::Resource

Here's a first version of the new class Gio::Resource.

Some gdk-pixbuf functions have been deprecated in favour of GResource.
GResource ought to be wrapped in glibmm.

I attach a first version of Gio::Resource to this patch to give people a
chance to comment on it before I push it.

Are the names of all methods acceptable? Unfortunately, it's not possible
to derive all names from glib's function names in the usual way by just
stripping the class name.
  - g_resources_register() can't be called register(), because register is a
    C/C++ keyword.
  - g_resources_open_stream() can't be called open_stream(), if
    g_resource_open_stream() is called open_stream(), because their parameter
    lists are identical. One of them is static, the other non-static.
  - Is load() is good name, or should it be called create_from_file()?

(In reply to comment #1)
> Created an attachment (id=289339) [details] [review]
>   - g_resources_register() can't be called register(), because register is a
>     C/C++ keyword.

I'd call then register_global() and unregister_global(). The name you chose is to verbose IMHO.

>   - g_resources_open_stream() can't be called open_stream(), if
>     g_resource_open_stream() is called open_stream(), because their parameter
>     lists are identical. One of them is static, the other non-static.
>   - Is load() is good name, or should it be called create_from_file()?

create_from_file() would be more in line with what the other class have.

Review of attachment 289339 [details] [review]:

Thanks for doing this.

::: gio/src/resource.hg
@@ +136,3 @@
+
+public:
+#m4 _CONVERSION(`char**',`std::vector<Glib::ustring>',`Glib::ArrayHandler<Glib::ustring>::array_to_vector($3, Glib::OWNERSHIP_DEEP)')

I generally put these right before the _WRAP_METHOD() that uses them, because they are so specific.

@@ +141,3 @@
+  _WRAP_METHOD(static Glib::RefPtr<Resource> load(const std::string& filename), g_resource_load, errthrow)
+  _WRAP_METHOD(Glib::RefPtr<InputStream> open_stream(const Glib::ustring& path, ResourceLookupFlags lookup_flags = RESOURCE_LOOKUP_FLAGS_NONE) const, g_resource_open_stream, errthrow)
+  _WRAP_METHOD(Glib::RefPtr<Glib::Bytes> lookup_data(const Glib::ustring& path, ResourceLookupFlags lookup_flags = RESOURCE_LOOKUP_FLAGS_NONE) const, g_resource_lookup_data, errthrow)

I wonder about the constness for lookup_data() and open_stream(). Here they are const but they return a non-const object.

@@ +145,3 @@
+
+  _WRAP_METHOD_DOCS_ONLY(g_resource_get_info)
+  bool get_info(const Glib::ustring& path, gsize& size, ResourceFlags& flags, ResourceLookupFlags lookup_flags = RESOURCE_LOOKUP_FLAGS_NONE) const;

The size and flags parameters can be null. This lets me easily use get_info() to just discover if a file even exists in the resource.

Created attachment 290522 [details] [review]
patch: Add Gio::Resource

Updated patch.

I have shortened some method names and changed load() to create_from_file(),
like Hubert suggested.

> I generally put these right before the _WRAP_METHOD() that uses them, because
> they are so specific.

Done

> I wonder about the constness for lookup_data() and open_stream(). Here they are
> const but they return a non-const object.

The description of Glib::Bytes says that it represents an immutable byte
sequence. To be clearer, I've changed the return type of lookup_data() and
lookup_data_global() to Glib::RefPtr<const Glib::Bytes>.

An InputStream copies data when you read from the stream. Gio::InputStream::
read() is non-const, so a const InputStream is not very useful. But you can't
change the underlying data through the stream, can you? So I suppose the
Gio::Resource can't be changed through a non-const InputStream. That's why I
think open_stream() can be const.

> The size and flags parameters can be null. This lets me easily use get_info()
> to just discover if a file even exists in the resource.

I have added overloaded get_info() and get_info_global() without the 'flags'
and 'size' parameters.

I wonder if all the get_info[_global]() methods shall really return bool, or
if void is better. They either return true, or throw a Gio::ResourceError
exception. Or would it be better to return bool, and not throw an exception?

We should indeed avoid both a success/failure boolean and an exception.

I guess we need to keep the exception because it provides information and could one day provide more.

When you've pushed this, I'll use it in Glom instead of the C API.

Thanks.

https://git.gnome.org/browse/glibmm/commit/?id=05cc3d31e9b0bd17af43629452b955d0fece0ddf

get_info() and get_info_global() now return void.

Thanks. I'm using that now in Glom, though it turns out that I only needed to use Gio::Resource::get_info_global() and Gio::Resource::enumerate_children_global().

Actually, now I see that get_info() and get_info_global() can be odd when used with the default parameters. It feels odd that the getter doesn't return a bool, but it would also feel odd to check the bool when an exception would be thrown anyway:

bool Glom::Utils::get_resource_exists(const std::string& resource_path)
{
  try
  {
     Gio::Resource::get_info_global(resource_path);
     return true;
  }
  catch (const Gio::ResourceError&)
  {
     return false;
  }
}

Yes, I agree it's odd, when you just want to know if a certain resource exists.
We could add
  bool get_info_nothrow(const Glib::ustring& path,
    ResourceLookupFlags lookup_flags = RESOURCE_LOOKUP_FLAGS_NONE) const;
and
  static bool get_info_global_nothow(const Glib::ustring& path,
    ResourceLookupFlags lookup_flags = RESOURCE_LOOKUP_FLAGS_NONE);
that would return a bool instead of throwing an exception.

s/get_info_global_nothow/get_info_global_nothrow/

Created attachment 291064 [details] [review]
patch: Gio::Resource: Add get_info_nothrow() and get_info_global_nothrow()

Do you want me to add these methods?

I'm not very fond of the names get_info(), get_info_global(),
get_info_nothrow(), get_info_global_nothrow() on the methods without the
size and flags parameters. Is get_file_exists() etc. better?

I notice that Glom::Utils::get_resource_exists() takes a std::string parameter,
while I've used Glib::ustring in all Gio::Resource methods except
create_from_file(). That's because the description of g_resource_load() says
that the filename parameter is in the GLib filename encoding. Nothing is said
about character encoding in the description of other GResource functions.
I've assumed that they are encoded in UTF-8. That's a reasonable assumption,
since those path names have been read from XML files.

Sorry for not responding sooner.

Yes, please, I was thinking that get_file_exists() would be better. In theory we could end up reporting that it doesn't exist because of an unrelated GError, but I think it's OK.

"glib filename encoding" generally means "unknown encoding". We try to use std::string for filepaths/filenames elsewhere, so please change that too.

We should only use ustring when we are sure that it's UTF-8. That generally means we have to get the GTK+ people to say so clearly in the documentation, which is generally hard.

I have pushed https://git.gnome.org/browse/glibmm/commit/?id=26d5c9611cd1bf8d89fc6fd2984dfd0498fcaf71