Created attachment 273828 [details] [review]
0001-Application-Show-the-new-command-line-parsing-API.patch

This patch shows how to use this new API.

You can try it like so:
./example --foo --goo=something --hoo=something.txt --bar=2 somefile.txt

There is a problem still:

Each OptionEntry needs to be kept alive, because GApplication doesn't copy its strings. We could store copies of the OptionEntry in a std::vector<> member variable in Gio::Application, though we'd need some cleverness to avoid breaking ABI.

I'd welcome opinions about this.

1.
  template <typename T_ArgType>
  void add_main_option_entry(const Glib::OptionEntry& entry);

Should this really be a template function?
The template argument must be explicitly specified in each call.
There can be no general implementation, only specializations.
I'd prefer a set of normal functions in a case like this one.
  add_main_option_entry_bool(const Glib::OptionEntry& entry)
  add_main_option_entry_int(const Glib::OptionEntry& entry)
  etc.

2.
The implementation of Gio::Application::add_option_group() must be equivalent
to that of Glib::OptionContext::add_group().

void Application::add_option_group(OptionGroup& group)
{
  //Strangely, GApplication actually takes ownership of the GOptionGroup,
  // deleting it later.
  g_application_add_option_group(gobj(), (group).gobj_give_ownership());
}

3.
> Each OptionEntry needs to be kept alive, because GApplication doesn't copy
> its strings.

I can see two ways of handling this problem, none of them very good.

3.1
Accept the fact, and document it.
The documentation of Glib::OptionContext::add_group() says:
"Note that the group will not be copied, so it should exist for as long as
the context exists."

The same must be said in the documentation of Gio::Application::
add_option_group(). That's natural and consistent. The documentation of
g_application_add_option_group() says:
"This function is comparable to g_option_context_add_group()."

In the same way, it can be said in the documentation of Gio::Application::
add_main_option_entry() something like:
"Note that the entry will not be deeply copied, so it should exist for as long
as the application exists."

This is not natural and consistent. There's no similar restriction for
Glib::OptionGroup::add_entry().

3.2
Like you suggest, add some private data to Gio::Application, perhaps a
std::vector<Glib::OptionEntry>. That can be done without breaking ABI with a
std::map<const Gio::Application*, ExtraApplicationData> extra_application_data,
similar to what has been done in libxml++/libxml++/parsers/parser.cc,
glibmm/glib/glibmm/main.cc and glibmm/glib/glibmm/objectbase.[h|cc].
It's an ugly solution, but it can be used, if 3.1 is unacceptable.

(In reply to comment #3)
> 1.
>   template <typename T_ArgType>
>   void add_main_option_entry(const Glib::OptionEntry& entry);
> 
> Should this really be a template function?
> The template argument must be explicitly specified in each call.
> There can be no general implementation, only specializations.
> I'd prefer a set of normal functions in a case like this one.
>   add_main_option_entry_bool(const Glib::OptionEntry& entry)
>   add_main_option_entry_int(const Glib::OptionEntry& entry)
>   etc.

Yes, thanks for that sanity check. I think I was trying too hard to keep it as convenient as Glib::OptionGroup::add_entry(), but using a C++ type is not more convenient if we are not using a variable of that type.

> 3.
> > Each OptionEntry needs to be kept alive, because GApplication doesn't copy
> > its strings.
> 
> I can see two ways of handling this problem, none of them very good.
> 
> 3.1
> Accept the fact, and document it.
> The documentation of Glib::OptionContext::add_group() says:
> "Note that the group will not be copied, so it should exist for as long as
> the context exists."

It's ugly even for Glib::OptionContext, and it was probably hard for us to avoid it because of the use of the underlying arg_data, but I'd like to avoid it this time if possible.

(In reply to comment #3)
> I'd prefer a set of normal functions in a case like this one.
>   add_main_option_entry_bool(const Glib::OptionEntry& entry)
>   add_main_option_entry_int(const Glib::OptionEntry& entry)
>   etc.

Do you prefer that, or would you prefer to just use the existing
add_main_option_entry(const Glib::OptionEntry& entry, GOptionArg arg_type)
?

I'd prefer just
void add_main_option_entry(const Glib::OptionEntry& entry, GOptionArg arg_type)
or even better
void
add_main_option_entry(const Glib::OptionEntry& entry, Glib::OptionArg arg_type)

If we start using enum GOptionArg in public API, shouldn't we wrap it?
  _WRAP_ENUM(OptionArg, GOptionArg, NO_GTYPE)
in optionentry.hg?

All these methods are unnecessary, aren't they:
template <typename T_ArgType>
void add_main_option_entry(const Glib::OptionEntry& entry);
void add_main_option_entry_filename(const Glib::OptionEntry& entry);
void add_main_option_entry_filenames(const Glib::OptionEntry& entry);

Created attachment 275503 [details] [review]
0001-Add-add_main_option_entry.patch

OK, then here is the updated and simpler patch for glibmm. We still need to avoid the need to keep the OptionEntry alive. Avoiding the use of OptionEntry in the UI all together might be nice.

Created attachment 275504 [details] [review]
0001-Application-Show-the-new-command-line-parsing-API.patch 2

And here is the corresponding patch for gtkmm-documentation.

(In reply to comment #3)
> 2.
> The implementation of Gio::Application::add_option_group() must be equivalent
> to that of Glib::OptionContext::add_group().
> 
> void Application::add_option_group(OptionGroup& group)
> {
>   //Strangely, GApplication actually takes ownership of the GOptionGroup,
>   // deleting it later.
>   g_application_add_option_group(gobj(), (group).gobj_give_ownership());
> }

Maybe, though we'd have to investigate the code more, I think.


However, I'd like to just ignore this function for now, as it doesn't seem to be very useful:
https://bugzilla.gnome.org/show_bug.cgi?id=727602#c8

Ignoring it would give us the chance to just keep OptionEntry and OptionGroup out of the Application API altogether. add_main_option_entry() could then take several parameters instead of an OptionEntry.

(In reply to comment #10)
> Ignoring it would give us the chance to just keep OptionEntry and OptionGroup
> out of the Application API altogether.

Yes, I suppose application programmers can specify all options with
add_main_option_entry(). We can ignore add_option_group() until someone asks
for it.

> add_main_option_entry() could then take
> several parameters instead of an OptionEntry.

I have just tested a version of add_main_option_entry() that takes an
OptionEntry that need not be kept alive after the call. I also have come to
the conclusion that it would be easier to use, if it takes several parameters
instead of an OptionEntry. I'll make such a version and attach a patch.
I don't promise that it will be today.

That's great. Thanks.

Created attachment 275677 [details] [review]
patch: Gio::Application: Add add_main_option_entry() (glibmm)

Here's an add_main_option_entry() that takes several parameters instead of an
OptionEntry. Some details can be discussed, such as:
- Shall there be one add_main_option_entry() with an OptionArg parameter, or
  add_main_option_entry_int(), etc. without that parameter?
- Shall G_OPTION_REMAINING be the default value of long_name?
- What's the best order of the parameters?

I'm not sure it's really necessary to protect ExtraApplicationData with
a mutex in application.ccg. Is it possible (and useful) to have more than one
Application instance in a program? I can't see that glib makes any attempt to
make GApplication a singleton. The mutex makes the code safer, and it does not
cost very much. add_main_option_entry() will be called only a moderate number
of times.

Created attachment 275678 [details] [review]
patch: Application: Show the new command-line parsing API (gtkmm-documentation)

When I've compare Gio::Application::add_main_option_entry() and
Glib::OptionGroup::add_entry(), I've changed my mind.
One add_main_option_entry_*() function per data type would be better.
We would also like functions that take a SlotOptionArgString or
SlotOptionArgFilename parameter. Like so:

  void add_main_option_entry_bool(......)
  void add_main_option_entry_int(......)
  void add_main_option_entry_double(......)
  void add_main_option_entry_ustring(......)
  void add_main_option_entry_filename(......)
  void add_main_option_entry_ustrings(......)
  void add_main_option_entry_filenames(......)
  void add_main_option_entry_ustring(const SlotOptionArgString& slot, ......)
  void add_main_option_entry_filename(const SlotOptionArgFilename& slot, .....)

where ...... = const Glib::ustring& long_name, etc. Then we don't need to wrap
enum GOptionArg.

The functions that take a slot parameter will be somewhat limited, if there is
more than one Application instance. I suppose that will be very unusual.
Since we can't get hold of the GOptionGroup that GApplication uses, we can't
give it a gpointer data, which we would like to be a pointer to the
Gio::Application wrapper. The callback function will not know which Application
instance the decoded option belongs to. The callback function only knows the
name and value of the decoded option. This means that two Application instances
must not add option entries with the same name.

I can make a new patch, but preferably only after we've agreed on the API.
Wrapping g_application_add_main_option_entries() tends to be an iterative
process.

(In reply to comment #15)
> When I've compare Gio::Application::add_main_option_entry() and
> Glib::OptionGroup::add_entry(), I've changed my mind.
> One add_main_option_entry_*() function per data type would be better.

I don't have a strong opinion about it, so I'm happy to go with your choice, but what changed your mind?

For me, it would be nice to hide the slight strangeness of GOptionArg: G_OPTION_ARG_NONE means boolean and G_OPTION_ARG_CALLBACK isn't a type. We could hide that by using our own enum instead though.

On the other hand, having so many parameters to the method (to avoid GOptionEntry) makes it seem worse to have so many versions of that method.

> We would also like functions that take a SlotOptionArgString or
> SlotOptionArgFilename parameter.

Let's do that in an additional step, if at all. I don't know if it's even possible with the VariantDict options ways of doing thngs.

(In reply to comment #16)
> I don't have a strong opinion about it, so I'm happy to go with your choice,
> but what changed your mind?

I don't have a strong opinion, either. I just thought that since OptionGroup
has one add_entry() per data type, Gio::Application should also have that.
But the difference is of course that in Glib::OptionGroup there is a good
reason (parameters with different data types).

> For me, it would be nice to hide the slight strangeness of GOptionArg:
> G_OPTION_ARG_NONE means boolean and G_OPTION_ARG_CALLBACK isn't a type. We
> could hide that by using our own enum instead though.

That's a good idea. Our own enum, defined in Gio::Application. And just one
add_main_option_entry() (possibly 2 more with slot parameters).

>> We would also like functions that take a SlotOptionArgString or
>> SlotOptionArgFilename parameter.
>Let's do that in an additional step, if at all. I don't know if it's even
>possible with the VariantDict options ways of doing thngs.

I once spent quite some time implementing such functions in OptionGroup
(bug 589197). I suppose they are useful.

Options decoded by a callback function (G_OPTION_ARG_CALLBACK) won't be
included in the VariantDict. They are handled by the callback function in the
slot. In this case we get the chance to do our own postprocessing that we can't
do with other options. We make GOptionEntry.arg_data point to a callback
function in application.ccg, which copies each C string to a Glib::ustring or
a std::string before the slot function is called. Similar to what is done in
Glib::OptionGroup::option_arg_callback().

Created attachment 276174 [details] [review]
patch: Gio::Application: Add add_main_option_entry() (glibmm)

Yet another version of add_main_option_entry(), now also functions that take
slot parameters for custom-decoded command-line options.

The new functions still lack documentation. Apart from that, I think this
version is good enough.

Created attachment 276175 [details] [review]
patch: Application: Show the new command-line parsing API (gtkmm-documentation)

And a corresponding test case.

Is there any way that we can associate an extra_application_data with each GApplication instance via GObject data instead of using a static instance that we need to protect via a mutex?

Also, I'd prefer if we could add the callback support as a separate commit, just to keep the two commits simpler, if that's not too difficult.

(In reply to comment #20)
> Is there any way that we can associate an extra_application_data with each
> GApplication instance via GObject data instead of using a static instance that
> we need to protect via a mutex?

You're right. We can register yet another GQuark, and let it store a pointer
to an ExtraApplicationData. Why didn't I think of that? Probably because I've
used my ugly way of adding extra private data several times before. In those
cases I couldn't use a GQuark. Either the wrapped C object is not a GObject
(xmlpp::Parser in libxml++ and Glib::Source) or the data is needed before the
GObject is created (Glib::ObjectBase).

option_arg_callback_data is actually private static data. It could be declared
like that in the .hg file, but I prefer the way I've done it in the patch.
When it occurs only in the .ccg file, it's clear to everyone (even to me :)
that it's not part of the API or ABI.

> Also, I'd prefer if we could add the callback support as a separate commit,
> just to keep the two commits simpler, if that's not too difficult.

That's OK.

I'll make new patches. This time I'll probably add documentation.

Created attachment 276439 [details] [review]
patch: Gio::Application: Add add_main_option_entry() and enum OptionType

Created attachment 276440 [details] [review]
patch: Gio::Application: Add add_main_option_entry() taking a slot parameter

Review of attachment 276439 [details] [review]:

This looks great. Please feel free to push it. Thanks.

(In reply to comment #21)
> (In reply to comment #20)
> option_arg_callback_data is actually private static data. It could be declared
> like that in the .hg file, but I prefer the way I've done it in the patch.
> When it occurs only in the .ccg file, it's clear to everyone (even to me :)
> that it's not part of the API or ABI.

Why can't we use the GObject data for this too? And why does the mutex seems necessary - are similar parts of GApplication itself also protected by locks?

(In reply to comment #25)
> Why can't we use the GObject data for this too?

When the Application_option_arg_callback() is called, gpointer data is 0.
We don't know which Gio::Application or GApplication the command-line option
belongs to. g_application_add_main_option_entries() creates a GOptionGroup
with user_data = NULL. Application_option_arg_callback() must be able to
find the slot to call with only the option_name as input data.

Compare Glib::OptionGroup which creates a GOptionGroup with user_data = this.

> And why does the mutex seems
> necessary - are similar parts of GApplication itself also protected by locks?

I don't think there are similar parts in GApplication. Our problem is that we
would like to have one OptionArgCallbackDataMap per Application instance, but
we can't. Application_option_arg_callback() would not know which Application's
data to access. I doubt that GApplication has any such problem.
If we had a chance to specify the user_data to use when
g_application_add_main_option_entries() creates its GOptionGroup, we wouldn't
have this problem.

I should have added a warning in the documentation of the
add_main_option_entry() taking a slot, a warning saying that two Application
instances must not both add options with the same name.

I still don't know if it's possible and useful to create more than one
Application instance in a program. Provided there is only one Application
instance, or all Application instances are used in the same thread, no mutex
is necessary. Perhaps we could just say that the add_main_option_entry() taking
a slot is not thread-safe and must be used in only one thread. Then we wouldn't
need the mutex. Such a restriction would affect very few users (probably none).

(In reply to comment #26)
> (In reply to comment #25)
> > Why can't we use the GObject data for this too?
> 
> When the Application_option_arg_callback() is called, gpointer data is 0.
> We don't know which Gio::Application or GApplication the command-line option
> belongs to. g_application_add_main_option_entries() creates a GOptionGroup
> with user_data = NULL. Application_option_arg_callback() must be able to
> find the slot to call with only the option_name as input data.

Ah, yes, I see that the callback's user_data is set from the group, not the entry, which is rather awkward:
https://developer.gnome.org/glib/stable/glib-Commandline-option-parser.html#GOptionArgFunc

So, yes, your way seems to be the only way. You might add a comment about why it has to be done this way. And your mutex seems fairly wise too.

Well done and thanks.

I have pushed a glibmm patch with more comments than the patch in comment 23.
https://git.gnome.org/browse/glibmm/commit/?id=9dec09068d5afbe76839bfe7574b670b2a04fd27

I have also pushed a gtkmm-documenation patch similar to the patch in
comment 19.
https://git.gnome.org/browse/gtkmm-documentation/commit/?id=4fd9b57fa81634ef269a5932cd0f3ea4de63f9db