GNOME Bugzilla – Bug 658078
[Patch] main: factor out dbus names acquisition
Last modified: 2011-11-08 00:02:05 UTC
Created attachment 195508 [details] [review] main: factor out dbus names acquisition Currently, adding a new dbus name acquisition is involving duplicating code every time. This patch allows to add a new one by just specifying its name and whether or not a failure is fatal.
Review of attachment 195508 [details] [review]: ::: src/main.c @@ +63,3 @@ + gchar *name, + gboolean fatal, + gchar *other, ...) Use G_GNUC_NULL_TERMINATED. @@ +118,3 @@ + request_name_flags |= DBUS_NAME_FLAG_REPLACE_EXISTING; + shell_dbus_acquire_names(bus, Space between identifier and paren. @@ +132,2 @@ /* ...and the on-screen keyboard service */ + "org.gnome.Caribou.Keyboard", FALSE); You're missing a NULL terminator here.
Created attachment 195514 [details] [review] main: factor out dbus names acquisition Isn't G_GNUC_NULL_TERMINATED only for declarations like in headers ?
Review of attachment 195514 [details] [review]: Yes, to use G_GNUC_NULL_TERMINATED you'd have to make a separate prototype for the function. You can put it inside the C file, it doesn't need to be in a .h file. ::: src/main.c @@ +120,3 @@ + shell_dbus_acquire_names (bus, + request_name_flags, + &request_name_result, So...there's no point returning the result here since it will only be the result of the last name acquisition, and we don't use it after.
Created attachment 195549 [details] [review] main: factor out dbus names acquisition
Review of attachment 195549 [details] [review]: Looks good, thanks.
Created attachment 197362 [details] [review] main: factor out dbus names acquisition Adapt to current master (changes in dbus request flags for caribou)
Review of attachment 197362 [details] [review]: ::: src/main.c @@ +114,3 @@ + shell_dbus_acquire_name (bus, + request_name_flags, + &request_name_result, Watch your whitespace. @@ +133,2 @@ /* ...and the org.freedesktop.Notifications service; we always * specify REPLACE_EXISTING to ensure we kill off This comment, while correct, isn't really appropriate. The mention of REPLACE_EXISTING should go above the acquire_names call. @@ +140,3 @@ + shell_dbus_acquire_name (bus, + DBUS_NAME_FLAG_REPLACE_EXISTING, + &request_name_result, Whitespace.
Created attachment 199538 [details] [review] main: factor out dbus names acquisition
Review of attachment 199538 [details] [review]: Move the REPLACE_EXISTING comment and you should be good to go.
Created attachment 199539 [details] [review] main: factor out dbus names acquisition
Created attachment 199541 [details] [review] main: factor out dbus names acquisition
Review of attachment 199541 [details] [review]: LGTM.
Ping?
Attachment 199541 [details] pushed as 39727d1 - main: factor out dbus names acquisition Pushed, thanks!
The patch is wrong, reopening.
Created attachment 200940 [details] [review] main: Fix shell_dbus_acquire_names() Commit 39727d1156 refactored dbus acquisition, but due to wrong use of va_args we would only ever acquire the first bus name passed.
Review of attachment 200940 [details] [review]: Oops. Good catch.
Created attachment 200942 [details] [review] main: Fix shell_dbus_acquire_names() Sorry, that was still wrong - this one should work correctly.
Created attachment 200943 [details] [review] main: Fix shell_dbus_acquire_names() Smaller, less intrusive version.
Review of attachment 200943 [details] [review]: Works for me.
Attachment 200943 [details] pushed as 2b6c5bb - main: Fix shell_dbus_acquire_names()