Created attachment 352668 [details] [review]
SpinButton: Pass signal_input double&, not double*

The double* argument is to store the converted value from the text in
the SpinButton, so there is always a target to write to. Therefore, to
correctly use C++ style, this should use a reference and not a pointer.

Created attachment 352669 [details] [review]
convert_gtk.m4: Remove tautological conversions

char and gchar are equal, and therefore so are their pointers. There is
no purpose to defining conversions for these.

Both patches look fine and quite obvious.

Though I don't think that gmmproc agrees with you that "therefore so are their
pointers". The m4 macros just compare text strings. They don't understand that
gchar** is equal to char** because gchar is equal to char. The conversions you
want to remove are unnecessary because of
  _EQUAL(gchar**,char**)
in glibmm/tools/m4/convert_glib.m4.

That explains it - thanks.

Should I push the second patch, then? i.e. is it fine to rely on conversions from convert_glib, or do you tend to prefer explicitly having them in convert_gtk too, even if redundant?

While we are thinking about updating the API here:

This function returns an int. The really valid values are:

 * 0 (false): input was not handled
 * 1 (true) : successful conversion
 * -1       : input error

Should there perhaps be a specific enum for this? Something like

class SpinButton {
[...]
  enum class InputResult {
    InputError = -1,
    NotHandled,
    Successful Conversion
  };
};

and then an m4 conversion back to int for GTK+ (assuming that would work)

> is it fine to rely on conversions from convert_glib

Sure, that's why convert_glib.m4 and several other files are included in
convert_gdk.m4 and convert_gtk.m4. Gtkmm relies on m4 macros in glibmm, atkmm
and pangomm. You can push the second patch, if gmmproc can still generate all
the .h and .cc files. (I suppose it can.)
The first patch, of course, is only for the master branch, as it changes ABI
and API.

> Should there perhaps be a specific enum for this?
(the output value of signal_input()'s signal handlers)

Murray didn't quite like it at
https://mail.gnome.org/archives/gtkmm-list/2017-May/msg00045.html
unless it's added in gtk+. I tend to agree with him. We should only rarely add
enums that don't correspond to enums in gtk+.

(In reply to Kjell Ahlstedt from comment #6)
> > is it fine to rely on conversions from convert_glib
> 
> Sure, that's why convert_glib.m4 and several other files are included in
> convert_gdk.m4 and convert_gtk.m4. Gtkmm relies on m4 macros in glibmm, atkmm
> and pangomm. You can push the second patch, if gmmproc can still generate all
> the .h and .cc files. (I suppose it can.)

Thanks. As for 3-22, I'll give it a try and see; I expect it to be fine.


> > Should there perhaps be a specific enum for this?
> (the output value of signal_input()'s signal handlers)
> 
> Murray didn't quite like it at
> https://mail.gnome.org/archives/gtkmm-list/2017-May/msg00045.html
> unless it's added in gtk+. I tend to agree with him. We should only rarely
> add
> enums that don't correspond to enums in gtk+.

Oh yeah, I forgot about that. Thank you! The reasoning makes sense. I'm asking in gtk+ whether they'll consider a patch for this; so far, it sounds like it's worth a try.

(In reply to Kjell Ahlstedt from comment #6)
> > is it fine to rely on conversions from convert_glib
> 
> Sure, that's why convert_glib.m4 and several other files are included in
> convert_gdk.m4 and convert_gtk.m4. Gtkmm relies on m4 macros in glibmm, atkmm
> and pangomm. You can push the second patch, if gmmproc can still generate all
> the .h and .cc files. (I suppose it can.)

Most of the top of convert_gtk.m4 is a direct paste of convert_glib.m4. Would it be a good idea to remove that and just keep the stuff that gtkmm adds?

With a little help from 'git blame' I found that some lines were copied from
convert_gtk.m4 to convert_glib.m4 by commit
https://git.gnome.org/browse/glibmm/commit/?id=ac5deb9dad49a2919aa7b5e1b4e50fba81b64c03
but they were not deleted from convert_gtk.m4. I see no reason why they should
be kept in convert_gtk.m4, except possibly for one of the copied lines:
  _EQUAL(GdkAtom,Gdk::Atom)
This line should not be in convert_glib.m4. If it shall exist at all, it shall
be in convert_gtk.m4 or convert_gdk.m4. But I doubt that there is a Gdk::Atom.

Thanks again - I'll push the commit removing the redundant conversions to both branches, then the API change to master.

Indeed, at a quick glance, methods documented as returning Gdk::Atom actually return std::string or Glib::ustring, thanks to conversions in convert_glib.m4 - and removing that conversion from convert_gtk.m4 means both branches still build fine. So I guess that one can be dropped too.

Pushed. Then I realised I should probably move the double* => double& conversion out of convert_gtk to convert_glib, right? If so, I can do that later on.