Comments on GApplication

[Owen Taylor <otaylor redhat com>]

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
 - Hiding of g_type_init() and gtk_init()
 - Single instance
 - "Actions" exported externally.
 - The mainloop

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'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". 

(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?)

- Owen

===
  void (* action)(GApplication *app, const char *name, guint timestamp);
===

Maybe use a detailed signal?

===
typedef enum
{
    G_APPLICATION_FLAG_DISABLE_DEFAULT_IPC = 1 << 0
} GApplicationFlags;

GApplication *          g_application_new                           (int                *argc,
                                                                     char             ***argv,
                                                                     const char         *appid,
                                                                     GApplicationFlags   flags);
===

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

Do we really want to bind argc/argv processing into GApplication? That always
a source of huge complexity in libgnome.

===
GApplication *          g_application_try_new                       (int                *argc,
                                                                     char             ***argv,
                                                                     const char         *appid,
                                                                     GApplicationFlags   flags,
                                                                     GApplicationExistsCallback on_other_process_exists,
                                                                     gpointer            user_data);
===

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. A signal seems better to me than a custom
callback, and avoids the problem you have here that callback has no
destroy notify.

===
GApplication *          g_application_try_new_full                  (int                *argc,
                                                                     char             ***argv,
                                                                     GApplicationFlags   flags,
                                                                     GType               class_type,
                                                                     GApplicationExistsCallback on_other_process_exists,
                                                                     gpointer            user_data,
                                                                     guint               n_parameters,
                                                                     GParameter         *parameters);
GApplication *          g_application_new_full                      (int                *argc,
                                                                     char             ***argv,
                                                                     const char         *appid,
                                                                     GApplicationFlags   flags,
                                                                     const char         *first_arg,
                                                                     ...) G_GNUC_NULL_TERMINATED;
===

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

_try_ definitely means "fail gracefully and return an error rather than failing hard"

Don't know why try_new_full() takes a GParameter array, and new_full() takes varargs.
(I don't see g_application_new_full() in the C file)

====                                                                 
GApplication *          g_application_get_instance                  (void);
====

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.

===
void                    g_application_add_action                    (GApplication      *app,
                                                                     const char        *name,
                                                                     const char        *description);
void                    g_application_set_action_enabled            (GApplication      *app,
                                                                     const char        *name,
                                                                     gboolean           enabled);
void                    g_application_invoke_action                 (GApplication      *app,
                                                                     const char        *name,
                                                                     guint              timestamp);
===

===
GMainLoop *             g_application_get_mainloop                  (GApplication      *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 

===
void                    g_application_run                           (GApplication      *app);             
gboolean                g_application_quit                          (GApplication      *app, 
                                                                     guint              timestamp);
===


===
+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.

(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.)

- Owen