GNOME Bugzilla – Bug 700913
Make GetPrimaryDevice always return the composite device, cleanups
Last modified: 2013-06-17 16:48:57 UTC
Right now, the Power menu in the shell has a weird behavior. If you have your power cord plugged in, we return an error from GetPrimaryDevice, so the shell goes back to enumerating all batteries. We probably shouldn't do this, and instead always return the composite device from GetPrimaryDevice.
Created attachment 245164 [details] [review] power: Make GetPrimaryDevice always return the composite device With one battery, the composite device should report what we want as well, so this shouldn't see any major changes in most cases. The reason this is wanted is so that when the device is charging or fully charged, we'll get the composite device as well, rather than throwing an error.
Created attachment 245165 [details] [review] power: Clean up code relating to the composite device A lot of code was trying to be generic about the kind of devices the composite device would handle, when the surrounding code just took batteries. Remove some of this genericism.
Review of attachment 245164 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ -795,3 @@ - } - - /* just use the original device if only one primary battery */ That particular change could probably be done separately from the engine_get_primary_device() removal. It should be innocuous enough. @@ -1843,3 @@ - - /* not discharging */ - if (state != UP_DEVICE_STATE_DISCHARGING) From our discussion with Richard: <hughsie> hadess, okay, the reason it was added was to avoid batteries not involved in any action, i.e. in a pending state <hughsie> it might be more correct to do: /* not charging or discharging */ if (state != UP_DEVICE_STATE_CHARGING && state != UP_DEVICE_STATE_DISCHARGING) continue; <hughsie> or if (state == UP_DEVICE_STATE_PENDING_CHARGE || state == UP_DEVICE_STATE_PENDING_DISCHARGE) continue; You'd need to do the latter check in engine_update_composite_device(). @@ +3733,3 @@ /* return object */ if (g_strcmp0 (method_name, "GetPrimaryDevice") == 0) { + value = device_to_variant_blob (manager->priv->device_composite); And if there are no batteries, the composite device contains nothing of interest, and you should return an error. @@ +3741,3 @@ /* return array */ if (g_strcmp0 (method_name, "GetDevices") == 0) { + UpDevice *device; The move of this declaration could be done in a separate patch, it's a small somewhat-related cleanup.
Review of attachment 245165 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +529,3 @@ + /* Doing it here as it could be a composite device */ + g_object_get (device, "percentage", &percentage, NULL); + return percentage; This function was already broken, and it's just as broken I think. You'd return the percentage of battery for whatever the first battery would be. So if the first listed battery is a mouse battery, and not the laptop one, you'd return the battery percentage for that. Probably not what you want. The code can be largely simplified by returning -1 if there's no UP_DEVICE_KIND_BATTERY, or the "percentage" property value if there is a laptop/ups battery. You should be able to use "is-present" to keep whether the composite device represents 0 or more devices. Please split this change in a separate patch as well, as it shouldn't change the behaviour. @@ +560,3 @@ + if (kind != device_kind) + continue; The changes to engine_get_icon_priv() look fine but they can also be made separately.
Created attachment 246434 [details] [review] power: Ensure that we always have a composite device In the scenario when we only have one battery, we short-circuit and use that instead of creating a composite device. We want to always expose the composite device as an API, so never do that.
Created attachment 246435 [details] [review] power: Remove get_composite_device This was always a bit unnecessary, as it was only called with a device kind of BATTERY.
Created attachment 246436 [details] [review] power: Ensure that the composite device only represent battery devices The code before was a tad too generic..
Created attachment 246437 [details] [review] power: Use is-present to mark whether the composite device is relevant
Created attachment 246438 [details] [review] power: Make the Percentage property always reflect the composite device And ensure that it returns -1 when we have no devices in the composite device.
Created attachment 246439 [details] [review] power: Make GetPrimaryDevice always return the composite device With one battery, the composite device should report what we want as well, so this shouldn't see any major changes in most cases. The reason this is wanted is so that when the device is charging or fully charged, we'll get the composite device as well, rather than throwing an error.
Comment on attachment 245165 [details] [review] power: Clean up code relating to the composite device Since you seemed to want patches split out, here's all the patches split out. Review comments have been taken into account.
Review of attachment 246435 [details] [review]: Looks good.
Review of attachment 246436 [details] [review]: Looks good.
Review of attachment 246437 [details] [review]: Looks good.
Review of attachment 246438 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +518,3 @@ + if (is_present) + return percentage; + else You can remove the "else" here, as you'll follow through. @@ +519,3 @@ + return percentage; + else + return -1; I'd write "-1.0" to make sure it gets passed out as a double.
Review of attachment 246439 [details] [review]: That looks fine, but I'd want some test cases for it. test_action_multiple_batteries in plugins/power/test.py could do with checking the icon/percentage values for example.
Review of attachment 246434 [details] [review]: That looks good, thanks.
The tests fail when I run them: g_dbus_connection_real_closed: Remote peer vanished with error: Underlying GIOStream returned 0 bytes on an async read (g-io-error-quark, 0). Exiting. ====================================================================== ERROR: test_action_critical_battery (__main__.PowerPluginTest) action on critical battery ---------------------------------------------------------------------- Traceback (most recent call last):
+ Trace 232047
notify_log = self.p_notify.stdout.read()
---------------------------------------------------------------------- Was this because of something I did?
(In reply to comment #18) > The tests fail when I run them: Yep, bisect says this broke it: [4f5ec16c1287d963412ce8f5e0dc4de31290c1a4] power: Ensure that we always have a composite device My guess is that we're relying on a property that didn't get copied/updated to the fake battery.
Created attachment 246829 [details] [review] power: Update time_to_empty/time_to_full from the real batteries So that we use upower's knowledge of that value instead of making up our own. This fixes the test_action_critical_battery test failing.
(In reply to comment #20) > Created an attachment (id=246829) [details] [review] > power: Update time_to_empty/time_to_full from the real batteries > > So that we use upower's knowledge of that value instead of making > up our own. This fixes the test_action_critical_battery test failing. Richard, why are we doing quick and dirty calculations of time-to-empty/time-to-full when creating a composite device, instead of adding those values from the different batteries? Should we check n_batteries == 1 in those tests instead of time_to_full/time_to_empty being 0 in those conditionals?
(In reply to comment #21) > Richard, why are we doing quick and dirty calculations of > time-to-empty/time-to-full when creating a composite device, instead of adding > those values from the different batteries? Most laptops with multiple batteries don't produce two sets of discharge values that always make sense. I think this was working around buggy firmware, and at least on my Lenovo T510 the batteries don't seem to produce discharge data for batteries in the pending state. I think that patch is probably okay.
I think that the problem is that the batteries discharge serially, not at the same time, in some cases, so you'd need to average time-to-empty based on the batteries' charges. The patch needs a little work. If there's a buggy firmware though, the work-around to produce usable data should be in upower, or the kernel, not in gnome-settings-daemon (otherwise we'll have the exact same bug in gnome-control-center...).
After rebasing on top of a few more bug fixes. Attachment 246434 [details] pushed as 3a9a004 - power: Ensure that we always have a composite device Attachment 246435 [details] pushed as 5fd4b04 - power: Remove get_composite_device Attachment 246436 [details] pushed as 67fd2e7 - power: Ensure that the composite device only represent battery devices Attachment 246437 [details] pushed as 269c7f7 - power: Use is-present to mark whether the composite device is relevant Attachment 246438 [details] pushed as c93c955 - power: Make the Percentage property always reflect the composite device Attachment 246439 [details] pushed as eedfa80 - power: Make GetPrimaryDevice always return the composite device Attachment 246829 [details] pushed as 6b463f7 - power: Update time_to_empty/time_to_full from the real batteries