Code review comment for lp:~charlesk/indicator-power/lp-1075192

Revision history for this message
Charles Kerr (charlesk) wrote :

> I'm not sure how this fixes bug 1075192.

Well, there are two parts to this bug -- the general case of the sorting, and the specific case of the AC Adapter's label. This patch fixes the first part, and since this should prevent an AC Adapter from ever being chosen as a primary device, the second part won't ever arise.

> (a) according to your explanation in comment 4 on the bug, I should experience the same problem -- but I am not able to reproduce this at all

Running i-power 12.10.4 on a laptop with a fully-charged battery should trigger the behavior. You can also mockup an equivalent configuration by hardwiring it in indicator_power_set_devices():

   + /* for testing only -- forcing a specific configuration */
   + devices = NULL;
   + devices = g_slist_append (devices, indicator_power_device_new (
   + "/org/freedesktop/UPower/devices/line_power_AC",
   + UP_DEVICE_KIND_LINE_POWER, 0, UP_DEVICE_STATE_UNKNOWN, 0));
   + devices = g_slist_append (devices, indicator_power_device_new (
   + "/org/freedesktop/UPower/devices/battery_BAT0",
   + UP_DEVICE_KIND_BATTERY, 100, UP_DEVICE_STATE_FULLY_CHARGED, 0));

> (b) a label should *never* be displayed, no matter how the devices are sorted

The AC Adapter is the only device where this happens, and with proper sorting we'll never choose that device as a primary.

Still, it's a fair point that the code should be correct.

I've added unit tests for this case in r157 and the fix in r158.

> (c) with this fix, the label would still appear when there's only line power available (although we don't show the indicator in that case)

This seems to be the same as (b)?

« Back to merge proposal