Re: Comments on GApplication

[Colin Walters <walters verbum org>]

On Tue, Apr 20, 2010 at 5:28 PM, Owen Taylor <otaylor redhat com> wrote:
> Spent a bit of time looking at GApplication and I'm not sure it's
> completely cohering as an overall picture for me. There seems to be a
> lot of quite different things in it:
>
>  - Option processing

This wasn't being used in GApplication, so I removed it there.

>  - Hiding of g_type_init() and gtk_init()

But for GtkApplication, why not make it the first call?  You can
always pass NULL, NULL of course, and it doesn't have any effect if
you called gtk_init() earlier.

>  - Single instance
>  - "Actions" exported externally.

Right.

>  - The mainloop

This one is optional.

> That aren't fundamentally related to each other. It does make for
> a cleaner looking Hello World, but I'm wondering if it wouldn't be
> better to scope it more narrowly.... maybe just to the "single
> instance / exported presence" aspects.

I could remove the mainloop from GApplication too...but it seems
natural to have it there, like how GtkApplication owns gtk_main().

> I'm also uncertain about the GApplication vs. GtkApplication split. It's
> obviously nice theoretically if we can share infrastructure between
> Clutter apps and GTK+ apps. But it means there is a lot of pointless
> GApplication API (most of it) that doesn't really make sense to
> an application developer, since you'd be unlikely to have a raw
> "GApplication".

Yes, I agree it's weird...but it doesn't really hurt much either.

> (It looks like there's an attempt to make GtkApplication somewhat
> complete, with gtk_application_run(), is that meant to shield
> Hello World from GApplication?)

Yes.

>  void (* action)(GApplication *app, const char *name, guint timestamp);
> ===
>
> Maybe use a detailed signal?

Good suggestion; fixed, thanks!

> ===

> What are the flags about? The one example is undocumented and as far as I can find, unused.

Fixed.

> I don't think a callback if another process exists is enough - you also
> need to know when another process *doesn't* exist, so you can actually go
> ahead and construct your UI.

g_application_new only returns by default if the other process doesn't
exist; see the docs.

> A signal seems better to me than a custom
> callback, and avoids the problem you have here that callback has no
> destroy notify.

The callback is (scope call).

> Tim has very strongly articulated a view in the past that _full means "with GDestroyNotify"
> and not "with random extra stuff".

Ok I removed the new_full, and made _try_new the "everything" variant.

> If this is meant to be single instance, then that needs to be clearly indicated, and
> calling g_application_new() multiple times should be caught a warning/error.

It's documented, and by default we singleton the app.

> Is this accessor needed? I can't think of anything you'd do with it. I guess you could
> call is_running(), but it's never clear what that's for, and

Don't think it hurts, but no strong opinion.
> ===
> +void            gtk_application_quit (GtkApplication *app, guint timestamp);
> ===
>
> We don't include a timestamp in new GTK+ APIs - see, e.g., GtkClipboard.
> GTK+ can almost always do a better job at figuring out a timestamp on its
> own then can an application.

Fixed.

> (The actions API of course, poses an issue for that - I suspect that the
> actions internal API / D-Bus methods should probably contain a timestamp
> that is picked up by GTK+ and automatically used to set the current time.)

Yes; I'll probably need to add an internal hook so that if there's no
GdkEvent we pick up the latest dbus timestamp.