>   //TODO:_WRAP_METHOD(void list_schemas(bool recursive,  gchar*** non_relocatable, gchar*** relocatable), g_settings_schema_source_list_schemas)

This is a heckuva ugly signature. What is the best way to handle this?

 (A) Pass nullable pointers to 2 x vector<ustring>, which will simply be overwritten with the results if non-null, possible returning both sets.
 (B) Manually implement 2 different methods, list_schemas_non_relocatable() and list_schemas_relocatable(), which will call the GIO function and return a vector<ustring> by value of only the requested set.

I think B is better, but just want to confirm.

(In reply to Daniel Boles from comment #1)
>  (B) Manually implement 2 different methods, list_schemas_non_relocatable()
> and list_schemas_relocatable(), which will call the GIO function and return
> a vector<ustring> by value of only the requested set.

 (C) 1 method with a bool argument specifying whether the user wants the relocatable or non-relocatable set.

Created attachment 352968 [details] [review]
SettingsSchemaSource: Implement list_schemas()

Review of attachment 352968 [details] [review]:

::: gio/src/settingsschemasource.ccg
@@ +27,3 @@
+{
+  auto gobject = const_cast<GSettingsSchemaSource*>(gobj());
+  gchar** schemas;

I went with gchar just to be extra explicit that it's going to hold the result of a call to a C function. But might you prefer char instead?

@@ +30,3 @@
+
+  if (relocatable)
+    g_settings_schema_source_list_schemas(gobject, recursive, nullptr, &schemas);

Passing nullptr to C APIs is guaranteed to work, but might you prefer NULL instead?

::: gio/src/settingsschemasource.hg
@@ +66,3 @@
+  /** Lists the schemas in a given source.
+   *
+   * @param recursive If true, the list will include parent sources. If false,

I realise these 2 bool arguments should be swapped so they match the order in the argument list.

Created attachment 352971 [details] [review]
SettingsSchemaSource: Wrap …new_from_directory()

[disclaimer: not tested yet]

Created attachment 352972 [details] [review]
SettingsSchemaSource: Implement list_schemas()

> SettingsSchemaSource: Wrap …new_from_directory()

An alternative API:
  static Glib::RefPtr<SettingsSchemaSource> create(
    const Glib::ustring& directory, bool trusted,
    const Glib::RefPtr<SettingsSchemaSource>& parent = get_default());

The glib documentation says that parent shall be get_default() except in very
unusual situations.

There is a trick you can use when the C documentation is almost acceptable, but
has to be modified somewhat.

1. Add _WRAP_METHOD_DOCS_ONLY(g_settings_schema_source_new_from_directory)
   or another suitable gmmproc directive.
2. Let gmmproc generate code:
      cd gio/src
      make .stamps/settingsschemasource.stamp
3. Copy the generated documentation to settingsschemasource.hg and delete
   _WRAP_METHOD_DOCS_ONLY.
4. Make necessary changes in the documentation.

Then most formatting will be correct, such as @a parent instead of just parent,
when parent is the name of a parameter, and <tt>true</tt> instead of just true.
I don't think @throw will be correct automatically. The exception shall be
documented as
  * @throw Glib::Error
or a subclass of Glib::Error, if you know that the exception will always be an
object of a certain subclass. Unfortunately glib and gtk+ usually don't document
exceptions in enough detail.


> SettingsSchemaSource: Implement list_schemas()

Looks fine.

Possibly add an overload that fetches both lists with one call to
g_settings_schema_source_list_schemas(). It would be slightly faster, when you
want both lists. But it can wait until someone asks for it.

Thanks for the reviews!

I'll think more about the first one.


> > SettingsSchemaSource: Implement list_schemas()
> 
> Looks fine.
> 
> Possibly add an overload that fetches both lists with one call to
> g_settings_schema_source_list_schemas(). It would be slightly faster, when
> you
> want both lists. But it can wait until someone asks for it.

I did feel like this would be better. Although presumably this will never be a performance bottleneck. The issue I had was being unable to think of a really nice signature for returning multiple values:

 * Output pointers would mean the user has to supply vectors, which then can have pre-existing contents, which we would clobber during the output assignment.
 * Output references would mean we would have to populate both (since the user cannot pass null to indicate they don't care), which wastes time just as much.
 * We could return a custom struct or just an std::pair, but that would have the same problem of not making either list really optional, unless we also add bool arguments to the method and return vectors of size() == 0 when the user doesn't care...

It gets complicated fast. So, given that the slowdown should not really matter, I settled for the simpler option.

However, if you have a preference for a better way to do it, then I'd be happy to try implementing it.

Thanks!

> SettingsSchemaSource: Implement list_schemas()

I was thinking of having two overloaded list_schemas(), the one in your patch
and one more like g_settings_schema_source_list_schemas(), perhaps

  void list_schemas(bool recursive, std::vector<Glib::ustring>& non-relocatable,
    std::vector<Glib::ustring>& relocatable);

I don't say that it's necessary. It may be too much trouble for something that
is not at all important.

(In reply to Kjell Ahlstedt from comment #7)
> > SettingsSchemaSource: Wrap …new_from_directory()
> 
> An alternative API:
>   static Glib::RefPtr<SettingsSchemaSource> create(
>     const Glib::ustring& directory, bool trusted,
>     const Glib::RefPtr<SettingsSchemaSource>& parent = get_default());
> 
> The glib documentation says that parent shall be get_default() except in very
> unusual situations.

If it's non-null (empty RefPtr in our case), i.e. if any parent is specified at all. I don't know, but is it correct to presume the user is more likely to want the default source as the parent, rather than no source? Otherwise they would have to explicitly pass an empty RefPtr if they don't want the default as the parent, which might be unexpected/ugly.

Rather academic, I know, since evidently no one has wanted to use this enough, so far. :)

> is it correct to presume the user is more likely to want the default source as
> the parent, rather than no source?

I don't know what's most likely. If parent = get_default() is not obviously
the most likely choice, the create() method could be

  static Glib::RefPtr<SettingsSchemaSource> create(
    const Glib::ustring& directory, bool trusted,
    const Glib::RefPtr<SettingsSchemaSource>& parent =
      Glib::RefPtr<SettingsSchemaSource>());

Yeah, that is what I'm leaning towards; it seems better to not recurse unless asked, than to recurse to the default schema; albeit noting that if the user does want to recurse then they probably want the default.

On that note, can we start using = {} for default arguments? It looks much tidier!

I did notice that doesn't seem to work in _WRAP macros though; I don't know whether it could be made to.

You're right. parent = {} is better. I just haven't learned all the new
possibilities in C++11 and C++14 yet.

Several gmmproc macros can't handle curly braces ({}) properly. That's why I
chose newin "3,90" instead of the more natural newin{3,90} for the optional
'newin' parameter when I added it in most of the _WRAP macros.
I suppose it would be possible to fix, but I don't know how difficult it would
be. In _WRAP_METHOD, curly braces delimit extra info to gmmproc, e.g.

  _WRAP_METHOD(void get_accel(guint& accelerator_key, Gdk::ModifierType& accelerator_mods{>>}) const, gtk_accel_label_get_accel)
  _WRAP_METHOD(void expose_widget(const Glib::ustring& name, Widget& widget{object}), gtk_builder_expose_object)
  _WRAP_METHOD(void set_accept_label(const Glib::ustring& accept_label{NULL} = Glib::ustring()), gtk_file_chooser_native_set_accept_label, newin "3,92")
  _WRAP_METHOD(void attach_next_to(Widget& child, Widget& sibling{?}, PositionType side, int width = 1, int height = 1), gtk_grid_attach_next_to)

I have pushed the patch
  gmmproc: Accept curly braces in default values in _WRAP macros
to the master branch. If you like, you can add

#m4 _CONVERSION(`const Glib::RefPtr<SettingsSchemaSource>&', `GSettingsSchemaSource*', __CONVERT_CONST_REFPTR_TO_P)
  _WRAP_METHOD(static Glib::RefPtr<SettingsSchemaSource> create(
    const Glib::ustring& directory, bool trusted{.},
    const Glib::RefPtr<SettingsSchemaSource>& parent{.} = {}),
    g_settings_schema_source_new_from_directory, errthrow)

(In reply to Kjell Ahlstedt from comment #14)
> I have pushed the patch
>   gmmproc: Accept curly braces in default values in _WRAP macros
> to the master branch.

Cool, thanks - I imagine that will be really helpful for tidying up declarations!

I'll revisit these patches again some day soon. Thanks for the suggestion; I hadn't considered using _WRAP_METHOD for a static create() function, so that should make for a much smaller patch.

(In reply to Kjell Ahlstedt from comment #14)
> I have pushed the patch
>   gmmproc: Accept curly braces in default values in _WRAP macros
> to the master branch.

Is this a non-option for the stable branch? (currently 2-52, presumably soon to be 2-54)

At the moment this option is not available in the glibmm-2-52 branch.
I don't know if it's okay to push it there. It would certainly be okay to push
it to a glibmm-2-54 branch before glibmm 2.54.0 is released.

Feel free to make the new glibmm-2-54 branch (from glibmm-2.52) and push it, along with any other API additions (but not breaks). Thanks.

I have created the glibmm-2-54 branch and pushed some gmmproc patches there,
such as the patch that makes it possible to specify default values of type
xxx = {} in _WRAP_METHOD.

I plan to transfer more patches from master to glibmm-2-54.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glibmm/issues/19.