Created attachment 296885 [details] [review]
gapplication: add bind_busy_property()

Balancing g_application_{un,}mark_busy() is non-trivial in some cases.

Make it a bit more convenient by allowing to bind multiple boolean
properties (from different objects) to the busy state. As long as these
properties are true, the application is marked as busy.

Created attachment 296887 [details] [review]
gapplication: add bind_busy_property()

Add new function to gio-sections.txt

Review of attachment 296887 [details] [review]:

::: gio/gapplication.c
@@ +2720,3 @@
+
+/**
+ * g_application_mark_busy_while:

Looks like you changed your mind about the function name and forgot to update it here.

I've at times toyed with the idea of allowing a more general property binding with multiple sources and a transform function that would compute the output from all the inputs.

But maybe it is better to treat this as a special case.

Created attachment 296892 [details] [review]
gapplication: add bind_busy_property()

> Looks like you changed your mind about the function name and forgot to update
> it here.

Indeed, I've written it for gnome-logs (cf. bug #744567) first where I called
it that. Thanks!

Review of attachment 296892 [details] [review]:

Seems quite nice.  Feel free to push after addressing the trivial issues below (or attach another patch if you want to discuss further).

::: gio/gapplication.c
@@ +2730,3 @@
+ * Multiple such bindings can exist, but only one property can be bound
+ * per object. Calling this function again for the same object replaces
+ * a previous binding. If property is %NULL, the binding is destroyed.

@property

@@ +2732,3 @@
+ * a previous binding. If property is %NULL, the binding is destroyed.
+ *
+ * The binding is also destroyed when @object is deleted.

Maybe mention that the refcount of @application is increased during the binding, but no ref is taken on @object.  Also avoid the word "deleted" here.

@@ +2743,3 @@
+  GClosure *closure = NULL;
+  guint notify_id;
+  gulong signal_id;

better call this handler_id -- gsignal is quite clear on its 'handler' vs. 'signal' terminology.

@@ +2745,3 @@
+  gulong signal_id;
+
+  g_return_if_fail (application != NULL);

why not G_IS_APPLICATION?

Thanks for review!

Attachment 296892 [details] pushed as 0f2b541 - gapplication: add bind_busy_property()

I thought of one more thing after you merged this:

It would be nice to be able to do this with no property at all (ie: busy as long as a particular object exists).

Getting a bit evil now, but it seems like we could use a special property name for that, like "exists" or "*" or (most likely) just the empty string.

Then we're really changing the focus of this API to be about an _object_ rather than a property, so probably we would want to change the name as well.  This would also be a nicer fit for the fact that we can only have one property per object (since it's about the object now, and not the properties).

g_application_bind_busy_object() perhaps?

Possible counterargument: garbage collection.  Tying an externally-visible effect to the lifecycle of an object is typically considered to be at least a little bit evil.

Interesting idea. The GC argument is strong, but it could still be useful for languages with deterministic object destruction. We have a lot of precedence for APIs that are useless in bindings but worthwhile to have in C or Vala.

To be honest, I don't really see the benefit. If you don't have an object with a property around, just make one. You probably want to show the busy state somewhere else in your application ui as well (not only in the panel). So it's useful to have this kind of property to bind to GtkSpinner::active, for example.

> Getting a bit evil now, but it seems like we could use a special property name > for that, like "exists" or "*" or (most likely) just the empty string.

No. This is just evil. I'd rather have an additional function bind_busy_object() which does what you describe.

I propose we add that if we get a first user of it? Feel free to reopen the bug if you disagree.

I meant to comment here again as well:

I think the restriction of 'only one property per object' is a bit ugly and artificial. I assume it comes about because you want to misuse bind_busy( ... NULL) to unbind. How about an explicit unbind instead ?

It was a side-effect of earlier versions of the patch that used a qdata for the state object -- you could only have one qdata per object.

Now that we are using a detailed signal connection as the state object holder, the restriction no longer exists (although we still have the restriction that an object may only be a busy-marker for one application).

If we moved to an explicit unbind API then it also means that we would free up the NULL case for property_name to be used for "no property" as described in comment 8.

The precautionary approach for now would be to simply disallow this function to be called with NULL property name and to provide no means of breaking a binding (short of destructing an object).  I actually think that's probably just fine, when you consider the scopes of objects likely to be involved in this arrangement.

Created attachment 296985 [details] [review]
gapplication: tune busy-binding

I agree that this restriction is weird. Lift it and add an explicit unbind.

Created attachment 296986 [details] [review]
gapplication: tune busy-binding

... and add the new function to gio-sections.txt. One of these days I'll
remember it before submitting a patch.

Review of attachment 296986 [details] [review]:

::: gio/gapplication.c
@@ +2776,2 @@
+void
+g_application_unbind_busy_property (GApplication *application,

missing doc string

@@ +2792,3 @@
+  handler_id = g_signal_handler_find (object, G_SIGNAL_MATCH_ID | G_SIGNAL_MATCH_DETAIL | G_SIGNAL_MATCH_FUNC,
+                                      notify_id, property_quark, NULL, g_application_notify_busy_binding, NULL);
+  g_signal_handler_disconnect (object, handler_id);

you should deal with a failure to find the handler more gracefully

Also: do we plan to rename this and/or allow NULL property for binding to object lifecycle?

Created attachment 297129 [details] [review]
gapplication: tune busy-binding

Oops, thanks. Added the doc string and throw a critical when calling
unbind_property() with a property that wasn't bound.

> Also: do we plan to rename this and/or allow NULL property for binding to
> object lifecycle?

To be honest, I don't care much for that for the reasons I gave in comment #10.

Review of attachment 297129 [details] [review]:

Looks good, please merge.

Some nice ideas for follow-up APIs:

 - add a boolean "busy" property to GApplication to allow the app to query its composite busy state

 - use that property to write tests for this API

Attachment 297129 [details] pushed as 2d3d8cd - gapplication: tune busy-binding