GNOME Bugzilla – Bug 675509
add extra dbus hooks
Last modified: 2012-05-29 17:12:28 UTC
Created attachment 213495 [details] [review] patch series When an application wants to use GApplication, but has its own DBus API in addition to GApplication's, there is a problem with the object registration. It needs to happen before GApplication tries to own the bus name, but there was no hook to do so without first acquiring the dbus connection oneself, then exporting, then creating the GApplication. The attached patch adds a hook that will be called at the right time, and another hook to undo anything the first hook did, to be called on unregistration. This will greatly simplify the gnome-terminal GApplication setup.
This looks pretty good. One question, though: what is the point of returning FALSE? Presumably registration fails because the pathname is in use -- do we try again with a different pathname? In the case of unique apps (that have their path at a well-known location) we can't really do that... So we just abort if it returns FALSE? Could it just abort for itself?
Well, all the other calls in g_application_impl_attempt_primary() do check for errors and make that an early return, so I copied that. (You're right however that those calls don't look like they ever really would fail in reality (?)...) The error will bubble up and can then be handled cleanly by the caller (ie g_application_run() or a manual call of g_application_register()). I think that's the right way; I want to handle errors myself cleanly instead of just abort()ing the programme inside some library call (for *runtime* errors).
Why is the registration cancellable? Do we expect this to block? Registering an object on the bus is not cancellable or blocking... Do we expect people to chain-up the register/unregister? If so, this gets a bit trickier in the presence of early-returns/cancellable/error, etc. Perhaps we should provide a code example showing the proper way to chain up. Your NULL checks on the hooks before calling them are not necessary because you have dummy implementations in the base class. Please don't commit the whitespace changes to the introspection XML as part of this patch. I'm not totally comfortable about the require-dbus flag. At present, dbus is the only backend and it actually looks as if we are moving further in that direction (ie: dbus will always be the only backend). What would this flag do in the case that we have a the dbus backend in use (ie: everywhere) but we fail to connect to the session bus for some transient reason? I'd prefer just not to have it... Although I agree with not using a critical for the error message, I'd also ask that to be done separately.
(In reply to comment #3) > Why is the registration cancellable? Do we expect this to block? Registering > an object on the bus is not cancellable or blocking... I thought I'd pass it down because the attempt_primary function already has it available, but I don't actually use it in g-t; and you're probably right that it will never be useful. I'll remove it. > Do we expect people to chain-up the register/unregister? If so, this gets a > bit trickier in the presence of early-returns/cancellable/error, etc. Perhaps > we should provide a code example showing the proper way to chain up. I only added the default impls for convenience (and then didn't even use them, as you noticed below); I think most users will just derive from GtkApplication directly, so I think we could get away with not providing default impls, and then users generally will not need to chain up... although that would preclude us from ever making use of it in GtkApplication, I don't foresee any need for it to use these hooks, so that should be ok. ? > Your NULL checks on the hooks before calling them are not necessary because you > have dummy implementations in the base class. I'll either remove the dummy impl or the checks, depending on the discussion about chain-up above. > I'm not totally comfortable about the require-dbus flag. At present, dbus is > the only backend and it actually looks as if we are moving further in that > direction (ie: dbus will always be the only backend). What would this flag do > in the case that we have a the dbus backend in use (ie: everywhere) but we fail > to connect to the session bus for some transient reason? I'd prefer just not > to have it... I'll remove that patch for now; we can always add it later if we ever get a non-DBus backend. What I'd expect to happen if it can't connect to the session bus is to return the error from g_application_register. > Please don't commit the whitespace changes to the introspection XML as part of > this patch. > Although I agree with not using a critical for the error message, I'd also ask > that to be done separately. Committed the whitespace path directly after the discussion and ok on IRC; the g_critical one will be split off into its own bug.
Created attachment 214885 [details] [review] application: Add dbus register/unregister hooks Updated patch for comments: - removed GCancellable - removed NULL checks - added example - removed REQUIRE_DBUS flag addition
Review of attachment 214885 [details] [review]: Great. Thanks so much.
Pushed to master.