After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 620952 - g_application_register_with_data is an ugly API
g_application_register_with_data is an ugly API
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2010-06-08 11:27 UTC by Christian Persch
Modified: 2010-06-16 15:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add GError to g_application_register_with_data (10.89 KB, patch)
2010-06-13 12:17 UTC, Christian Persch
none Details | Review
Rework GApplication API to use GInitable (20.62 KB, patch)
2010-06-16 04:19 UTC, Colin Walters
none Details | Review
Rework GApplication API to use GInitable (30.96 KB, patch)
2010-06-16 14:09 UTC, Colin Walters
committed Details | Review

Description Christian Persch 2010-06-08 11:27:17 UTC
g_application_register_with_data is sync, but it isn't named ..._sync, and there is no ..._async/..._finish variant of it.

Also, it can fail, but doesn't return gboolean and has no GError** out param.


What's the point of this weird setup with g_application_new + _register_with_data() ? Why not have GApplication implement GInitable (and GAsyncInitiable), pass argv and argc as construct params, and if argv != NULL, try to register it in the initiable_init method?
Comment 1 Colin Walters 2010-06-08 13:29:41 UTC
(In reply to comment #0)
> g_application_register_with_data is sync, but it isn't named ..._sync, and
> there is no ..._async/..._finish variant of it.

Claiming the name synchronously is a key use case for the applications I had in mind, where you need to ensure it's a singleton in the session.  

I'm not opposed to adding an _async variant, but for most applications, it's generally wrong to be doing anything like reading in user data files or creating windows until you know you're the primary name owner.  (If you're not, you need to communicate with whichever process is).

(In reply to comment #0)
> Also, it can fail, but doesn't return gboolean and has no GError** out param.

You mean e.g. if there's no session bus?  Would you want to do something different there other than g_error?  I see this as similar to how Gtk just aborts by default if there's no $DISPLAY.

We could also add a _checked or _try variant I suppose.

> What's the point of this weird setup with g_application_new +
> _register_with_data() ? Why not have GApplication implement GInitable (and
> GAsyncInitiable), pass argv and argc as construct params, and if argv != NULL,
> try to register it in the initiable_init method?

I originally didn't want argv as an object property, but I guess we might as well just accept it being there.
Comment 2 Colin Walters 2010-06-09 20:10:12 UTC
(In reply to comment #0)
>
> What's the point of this weird setup with g_application_new +
> _register_with_data() ? Why not have GApplication implement GInitable (and
> GAsyncInitiable), pass argv and argc as construct params, and if argv != NULL,
> try to register it in the initiable_init method?

One problem with doing this is it changes the API semantics significantly around how we handle the is-remote flag.

Note the way GInitable is intended to work for language bindings is that they *automatically* call the _init method and throw if it fails.  This would mean a delta between C where the app is just a remote proxy until you call _init.

I'm not sure.  Really, the only thing I care about is having sane semantics for *Gtk*Application; if people who want to use GApplication for unconventional cases desire different APIs that's OK.
Comment 3 Paolo Borelli 2010-06-13 08:30:04 UTC
(In reply to comment #1)
> You mean e.g. if there's no session bus?  Would you want to do something
> different there other than g_error?  I see this as similar to how Gtk just
> aborts by default if there's no $DISPLAY.
> 

The reason to also have GApp beside GtkApp is to use them in headless applications where I expect much better logging than simply error out.
Comment 4 Christian Persch 2010-06-13 12:17:27 UTC
Created attachment 163502 [details] [review]
Add GError to g_application_register_with_data

Return FALSE on failure with @error set, instead of just calling
g_error() internally.

Bug #620952.
Comment 5 Christian Persch 2010-06-13 12:19:23 UTC
This is a start, but I realised I need even more control. E.g. in the register phase, I want to add my own dbus interfaces in addition to org.gtk.Application...
Comment 6 Colin Walters 2010-06-16 04:19:32 UTC
Created attachment 163778 [details] [review]
Rework GApplication API to use GInitable

This is a work in progress.
Comment 7 Colin Walters 2010-06-16 14:09:52 UTC
Created attachment 163829 [details] [review]
Rework GApplication API to use GInitable
Comment 8 Matthias Clasen 2010-06-16 15:10:56 UTC
Review of attachment 163829 [details] [review]:

Looks good to me.
Comment 9 Colin Walters 2010-06-16 15:14:14 UTC
Attachment 163829 [details] pushed as 102c5f6 - Rework GApplication API to use GInitable
Comment 10 Christian Persch 2010-06-16 15:53:36 UTC
Just a note:

+ g_object_class_install_property (gobject_class,
+ PROP_ARGV,
+ g_param_spec_boxed ("argv",
+ P_("Argument vector"),
+ P_("System argument vector with type signature aay"),
+ G_TYPE_VARIANT,
+ G_PARAM_READWRITE |
+ G_PARAM_CONSTRUCT_ONLY |
+ G_PARAM_STATIC_STRINGS));
+
+ /**
+ * GApplication:platform-data:
+ *
+ * Platform-specific data retrieved from the operating system
+ * environment. It must be a #GVariant with type signature "a{sv}".
+ *
+ */
+ g_object_class_install_property (gobject_class,
+ PROP_PLATFORM_DATA,
+ g_param_spec_boxed ("platform-data",
+ P_("Platform data"),
+ P_("Environmental data, must have type signature a{sv}"),
+ G_TYPE_VARIANT,
+ G_PARAM_READWRITE |
+ G_PARAM_CONSTRUCT_ONLY |
+ G_PARAM_STATIC_STRINGS));

Bug 610863 adds GParamSpecVariant, which these could then use. (It enforces the variant type.)