Merge lp://staging/~charlesk/indicator-power/coverage into lp://staging/indicator-power/12.10

Very nice work! And impressive test coverage.

Note: I couldn't test it yet, as I'm still on precise. /me ducks.

src/Makefile.am:207:
  * any reason you're using noinst_HEADERS instead of listing the headers
    in the libraries sources? (the latter is recommended for programs and
    convenience libs in the automake docs)

dbus-listener.c
  * afaict, it could have been written by gdbus-codegen
  * 305: g_signal_new is passed one too many arguments. Please add the
    docstring with the number and names of parameters to it [1]
  * dispose() is missing g_bus_unwatch_name (priv->watcher_id)
  * get_devices_cb:144: trailing whitespace ;)
  * the cancellable is passed into all functions and unreffed in dispose()
    is the cancellable cancelled on unref or do we need to do that explicitely?
    (I can't find anything in the docs)

dbus-listener.h:
  * 50: the signal name is overly complicated (it doesn't need to be namespaced
    to the class)
  * indicator_power_dbus_listener_new is declared but not defined in the .c

device.c:
  * class_init: I prefer saving all pspecs in a static array and installing
    them all at once with g_object_class_install_properties. This also allows
    using g_object_notify_by_pspec
  * finalize doesn't chain up to the parent class
  * get/set_property:
     - use G_OBJECT_WARN_INVALID_PROPERTY_ID in the 'default' switch case
     - PROP_ICON is not handled
     - kind and state should check for sane input values
  * 260 ff: I don't think we can use Richard's code (indicators are CA, right?)
  * indicator_power_device_new_from_variant: check whether the input variant
    has the correct type before calling g_variant_get
  * 298: reference functions with 'function_name()' instead of #function_name
    in doc strings

device.h:
  * 43 ff: no need to namespace gobject properties (tbh, I wouldn't even
    #define them, but simply use the strings)

indicator-power.c:
  * 119: ah, that's why you didn't notice the implementation of
    indicator_power_dbus_listener_new was missing :)
  * ref the floating widgets in the private struct
    (just something I noticed, it has nothing to do with this work)

indicator-power.h:
  * device.h include is superfluous

test/test-dbus-{listener,device}.cc:
  * ensure_glib_initalized: g_type_init can be called multiple times

[1] http://developer.gnome.org/gtk-doc-manual/stable/documenting_symbols.html.en
    Section 3.3.4

Lars, thanks for the great review!

> any reason you're using noinst_HEADERS instead of listing the headers
> in the libraries sources? (the latter is recommended for programs and
> convenience libs in the automake docs)

Fixed r204

> dbus-listener.c
>
> 305: g_signal_new is passed one too many arguments. Please add the
> docstring with the number and names of parameters to it [1]

Fixed r205

> dispose() is missing g_bus_unwatch_name (priv->watcher_id)

Fixed r206

> get_devices_cb:144: trailing whitespace ;)

Fixed r207

> the cancellable is passed into all functions and unreffed in dispose()
> is the cancellable cancelled on unref or do we need to do that explicitely?
> (I can't find anything in the docs)

This is a great catch. Looking at the gio code, my assumption was wrong,
it needs to be done explicitly.

Fixed r208

> dbus-listener.h:
> * 50: the signal name is overly complicated (it doesn't need to be namespaced
> to the class)

Fixed r209

> * indicator_power_dbus_listener_new is declared but not defined in the .c

Fixed r210

> device.c:
> class_init: I prefer saving all pspecs in a static array and installing
> them all at once with g_object_class_install_properties. This also allows
> using g_object_notify_by_pspec

Interesting, I don't see that in any of the other indicator code. TIL.

Changed r211

> * finalize doesn't chain up to the parent class

Fixed r212

> * get/set_property:
> - use G_OBJECT_WARN_INVALID_PROPERTY_ID in the 'default' switch case

Fixed r213

> PROP_ICON is not handled

PROP_ICON was cruft, removed r211

> kind and state should check for sane input values

This is already done by glib's plumbing because the pspec defined the sane range. (No change)

> 260 ff: I don't think we can use Richard's code (indicators are CA, right?)

Before my refactoring, i-power was already doing this. One difference is now I'm giving proper attribution. (No change, but maybe a ted question?)

> indicator_power_device_new_from_variant: check whether the input variant has the correct type before calling g_variant_get

Fixed r214.

> 298: reference functions with 'function_name()' instead of #function_name in doc strings

Fixed r215.

> device.h: 43 ff: no need to namespace gobject properties (tbh, I wouldn't even #define them, but simply use the strings)

Fixed r216.

However I've kept the #defines since that idiom catches misspelled keys at compile time...

> indicator-power.c:
> * ref the floating widgets in the private struct
> (just something I noticed, it has nothing to do with this work)

Agreed, but unchanged for this pass...

> indicator-power.h:
> * device.h include is superfluous

Fixed r217

> test/test-dbus-{listener,device}.cc:
> * ensure_glib_initalized: g_type_init can be called multiple times

I don't think this is correct. Are you sure?

Thanks for the quick fixes!

On 06/06/2012 10:28 PM, Charles Kerr wrote:
>> the cancellable is passed into all functions and unreffed in dispose()
>> is the cancellable cancelled on unref or do we need to do that explicitely?
>> (I can't find anything in the docs)
>
> This is a great catch. Looking at the gio code, my assumption was wrong,
> it needs to be done explicitly.
>
> Fixed r208

Thanks for checking.

> [...]
>> kind and state should check for sane input values
>
> This is already done by glib's plumbing because the pspec defined the sane range. (No change)

You're right of course. Sorry.

>> 260 ff: I don't think we can use Richard's code (indicators are CA, right?)
>
> Before my refactoring, i-power was already doing this. One difference is now I'm giving proper attribution. (No change, but maybe a ted question?)

Definitely a Ted question.

>> indicator_power_device_new_from_variant: check whether the input variant has the correct type before calling g_variant_get
>
> Fixed r214.

Hm, I was thinking more along the lines of

  g_variant_is_of_type (v, G_VARIANT_TYPE ("(susdut)"));

The fix in r214 only checks for a tuple with the correct number of elements.

>> test/test-dbus-{listener,device}.cc:
>> * ensure_glib_initalized: g_type_init can be called multiple times
>
> I don't think this is correct. Are you sure?

I'm not sure. Actually, I think you might be right.