GNOME Bugzilla – Bug 790712
Code cleanup patchset for bluetooth-client.c
Last modified: 2017-11-23 12:25:18 UTC
Just a few follow up cleanups to the patch series in bug #782530.
Created attachment 364199 [details] [review] lib: Listen to and access properties through the GObject wrapper This completes the move to GObject properties started in the earlier patchset by Bastien (bug #782530). With this change there are no GVariant users left in bluetooth-client.c.
Created attachment 364200 [details] [review] lib: Unify resolving of device type and icon There were three different code paths that could be hit depending on the the order that properties were set or updated. Unify this into one code path to ensure that the result is always static.
Created attachment 364202 [details] [review] lib: Remove unused variable Fixes a compile wraning introduced in commit 27e88fc0 (lib: Simplify Properties setting).
Review of attachment 364199 [details] [review]: Is this really any more readable though? The patch certainly isn't...
Review of attachment 364200 [details] [review]: ::: lib/bluetooth-client.c @@ +290,2 @@ static void +device_resolve_type_and_icon (Device1 *device, BluetoothType *type, const char **icon) You need an assertion that the type and icon are set, or handle NULL pointers being passed in. g_return_if_fail() would be perfect here. @@ +293,3 @@ + const char *bdaddr; + + bdaddr = device1_get_address (device); Move this down next to the icon setting. ::: lib/bluetooth-utils.c @@ +234,3 @@ } + return BLUETOOTH_TYPE_ANY; BLUETOOTH_TYPE_ANY is 1, not 0. We use 0 to know whether it was parsed or not. Maybe you'd want to add a BLUETOOTH_TYPE_UNKNOWN in a separate patch?
(In reply to Bastien Nocera from comment #4) > Review of attachment 364199 [details] [review] [review]: > > Is this really any more readable though? The patch certainly isn't... The loop is being removed and the indentation changes. So yeah, the hunks look ugly, but the change is really straight forward. Maybe try looking at the old/new code side by side instead? (In reply to Bastien Nocera from comment #5) > Review of attachment 364200 [details] [review] [review]: > [SNIP] > @@ +234,3 @@ > } > > + return BLUETOOTH_TYPE_ANY; > > BLUETOOTH_TYPE_ANY is 1, not 0. > > We use 0 to know whether it was parsed or not. > > Maybe you'd want to add a BLUETOOTH_TYPE_UNKNOWN in a separate patch? OK, fine with me. Just need a bit more logic in device_resolve_type_and_icon then.
Review of attachment 364202 [details] [review]: > Fixes a compile wraning "warning". As for the other commits, this should be in the commit subject. Looks good otherwise.
Created attachment 364218 [details] [review] lib: Listen to and access properties through the GObject wrapper This completes the move to GObject properties started in the earlier patchset by Bastien (bug #782530). With this change there are no GVariant users left in bluetooth-client.c.
Created attachment 364219 [details] [review] lib: Unify resolving of device type and icon There were three different code paths that could be hit depending on the the order that properties were set or updated. Unify this into one code path to ensure that the result is always static.
Review of attachment 364219 [details] [review]: Apart from that nit with the g_return_if_fail(), looks good to commit. ::: lib/bluetooth-client.c @@ +291,3 @@ +device_resolve_type_and_icon (Device1 *device, BluetoothType *type, const char **icon) +{ + g_return_if_fail(!type || !icon); You can have 2 separate ones. It also means that the warning you'd get would tell you which one didn't meet the requirements.
Review of attachment 364218 [details] [review]: I don't find this very useful, apart from the device_list_uuids_v() removal, but I guess we'll have to live it this. ::: lib/bluetooth-client.c @@ +556,1 @@ gboolean notify = FALSE; Set notify to TRUE here and set it to FALSE in the last conditional "} else { ... }" for unhandled properties.
The following fixes have been pushed: 1b955ae lib: Unify resolving of device type and icon ed4701d lib: Listen to and access properties through the GObject wrapper
Created attachment 364269 [details] [review] lib: Unify resolving of device type and icon There were three different code paths that could be hit depending on the the order that properties were set or updated. Unify this into one code path to ensure that the result is always static.
Created attachment 364270 [details] [review] lib: Listen to and access properties through the GObject wrapper This completes the move to GObject properties started in the earlier patchset by Bastien (bug #782530). With this change there are no GVariant users left in bluetooth-client.c.