Bugfix: Fixes #484, taskband - UPower battery state crash

icons/luna/mappings

@@ -316,7 +316,7 @@ status/battery-empty-->battery_empty
 status/battery-empty-charging-->battery_low_charging
 status/battery-full-->battery_full
 status/battery-full-charged-->battery_full_on_ac
-status/battery-full-charging-->battery_full_on_ac
+status/battery-full-charging-->battery_full_charging
 status/battery-good-->battery_medium
 status/battery-good-charging-->battery_medium_charging
 status/battery-low-->battery_low

icons/luna/res/16x16/battery_full_charging.png

@@ -0,0 +1 @@
+battery_charging.png

shell/taskband/src/systray/power.c

@@ -6,50 +6,26 @@
 #include "behaviour.h"
 #include "power.h"
 
-//
-// GTK OOP CLASS/INSTANCE DEFINITIONS
-//
-struct _WinTCNotificationPowerClass
-{
-    WinTCNotificationBehaviourClass __parent__;
-};
-
-struct _WinTCNotificationPower
-{
-    WinTCNotificationBehaviour __parent__;
-
-    // Power stuff
-    //
-    UpClient* up_client;
-    UpDevice* up_last_battery;
-    gchar*    up_last_battery_path;
-};
-
 //
 // FORWARD DECLARATIONS
 //
 static void wintc_notification_power_constructed(
     GObject* object
 );
-static void wintc_notification_power_finalize(
+static void wintc_notification_power_dispose(
     GObject* object
 );
 
+static void wintc_notification_power_register_main_battery(
+    WinTCNotificationPower* power
+);
+static void wintc_notification_power_update_icon(
+    WinTCNotificationPower* power
+);
+
 static UpDeviceLevel battery_pct_to_enum(
     gdouble percentage
 );
-static void check_and_register_battery(
-    WinTCNotificationPower* power,
-    UpDevice*               device
-);
-static void update_battery_battery_level(
-    WinTCNotificationPower* power,
-    UpDevice*               device
-);
-static void update_client_on_battery(
-    WinTCNotificationPower* power,
-    UpClient*               client
-);
 
 static void on_up_client_device_added(
     UpClient* up_client,
@@ -72,6 +48,24 @@ static void on_up_device_battery_notify(
     gpointer    user_data
 );
 
+//
+// GTK OOP CLASS/INSTANCE DEFINITIONS
+//
+struct _WinTCNotificationPowerClass
+{
+    WinTCNotificationBehaviourClass __parent__;
+};
+
+struct _WinTCNotificationPower
+{
+    WinTCNotificationBehaviour __parent__;
+
+    // Power stuff
+    //
+    UpClient* up_client;
+    UpDevice* up_device_battery;
+};
+
 //
 // GTK TYPE DEFINITIONS & CTORS
 //
@@ -88,7 +82,7 @@ static void wintc_notification_power_class_init(
     GObjectClass* object_class = G_OBJECT_CLASS(klass);
 
     object_class->constructed = wintc_notification_power_constructed;
-    object_class->finalize    = wintc_notification_power_finalize;
+    object_class->dispose     = wintc_notification_power_dispose;
 }
 
 static void wintc_notification_power_init(
@@ -102,72 +96,57 @@ static void wintc_notification_power_constructed(
     GObject* object
 )
 {
+    (G_OBJECT_CLASS(wintc_notification_power_parent_class))
+        ->constructed(object);
+
     WinTCNotificationPower* power = WINTC_NOTIFICATION_POWER(object);
 
     // Connect to upower, enumerate existing devices and attach signals for
     // picking up new ones
     //
-    GPtrArray* all_devices;
-    UpDevice*  device;
-
     power->up_client = up_client_new();
-    if (power->up_client)
+
+    if (!power->up_client)
     {
-        WINTC_LOG_DEBUG("Connected to upower");
+        g_warning("%s", "taskband: power: Failed to connect to upower");
-
+        return;
-        all_devices = up_client_get_devices2(power->up_client);
-
-        for (guint i = 0; i < all_devices->len; i++)
-        {
-            device = all_devices->pdata[i];
-
-            check_and_register_battery(power, device);
-        }
-
-        g_signal_connect(
-            power->up_client,
-            "device-added",
-            G_CALLBACK(on_up_client_device_added),
-            power
-        );
-        g_signal_connect(
-            power->up_client,
-            "device-removed",
-            G_CALLBACK(on_up_client_device_removed),
-            power
-        );
-        g_signal_connect(
-            power->up_client,
-            "notify::on-battery",
-            G_CALLBACK(on_up_client_battery_notify),
-            power
-        );
-
-        update_client_on_battery(power, power->up_client);
-
-        g_ptr_array_unref(all_devices);
-    }
-    else
-    {
-        g_warning("Failed to connect to upower");
     }
 
-    (G_OBJECT_CLASS(
+    WINTC_LOG_DEBUG("taskband: power: Connected to upower");
-        wintc_notification_power_parent_class
+
-    ))->constructed(object);
+    wintc_notification_power_register_main_battery(power);
+
+    g_signal_connect(
+        power->up_client,
+        "device-added",
+        G_CALLBACK(on_up_client_device_added),
+        power
+    );
+    g_signal_connect(
+        power->up_client,
+        "device-removed",
+        G_CALLBACK(on_up_client_device_removed),
+        power
+    );
+    g_signal_connect(
+        power->up_client,
+        "notify::on-battery",
+        G_CALLBACK(on_up_client_battery_notify),
+        power
+    );
 }
 
-static void wintc_notification_power_finalize(
+static void wintc_notification_power_dispose(
     GObject* object
 )
 {
     WinTCNotificationPower* power = WINTC_NOTIFICATION_POWER(object);
 
-    g_free(power->up_last_battery_path);
+    g_clear_object(&(power->up_device_battery));
+    g_clear_object(&(power->up_client));
 
-    (G_OBJECT_CLASS(
+    (G_OBJECT_CLASS(wintc_notification_power_parent_class))
-        wintc_notification_power_parent_class
+        ->dispose(object);
-    ))->finalize(object);
 }
 
 //
@@ -189,104 +168,137 @@ WinTCNotificationPower* wintc_notification_power_new(
 //
 // PRIVATE FUNCTIONS
 //
-static UpDeviceLevel battery_pct_to_enum(
+static void wintc_notification_power_register_main_battery(
-    gdouble percentage
+    WinTCNotificationPower* power
 )
 {
-    // FIXME: Not checked against Windows XP
+    // Reset state (for our sanity really)
     //
-    if (percentage <= 15.0f)
+    g_clear_object(&(power->up_device_battery));
-    {
-        return UP_DEVICE_LEVEL_CRITICAL;
-    }
-    else if (percentage <= 25.0f)
-    {
-        return UP_DEVICE_LEVEL_LOW;
-    }
-    else if (percentage <= 75.0f)
-    {
-        return UP_DEVICE_LEVEL_NORMAL;
-    }
-    else if (percentage <= 90.0f)
-    {
-        return UP_DEVICE_LEVEL_HIGH;
-    }
-    else
-    {
-        return UP_DEVICE_LEVEL_FULL;
-    }
-}
 
-static void check_and_register_battery(
+    // ATM we can only raise one notification icon so just try to find 'a'
-    WinTCNotificationPower* power,
+    // battery and make that the main one
-    UpDevice*               device
+    //
-)
+    GPtrArray* all_devices = up_client_get_devices2(power->up_client);
-{
+    gboolean   on_battery  = up_client_get_on_battery(power->up_client);
-    guint device_kind;
 
-    g_object_get(
+    for (guint i = 0; i < all_devices->len; i++)
-        device,
+    {
-        "kind", &device_kind,
+        UpDevice* device = (UpDevice*) all_devices->pdata[i];
-        NULL
+        guint     device_kind;
-    );
+        gboolean  power_supply;
+        guint     state;
 
-    if (device_kind != UP_DEVICE_KIND_BATTERY)
+        g_object_get(
+            device,
+            "kind",         &device_kind,
+            "power-supply", &power_supply,
+            "state",        &state,
+            NULL
+        );
+
+        if (device_kind == UP_DEVICE_KIND_BATTERY)
+        {
+            // This is the battery we want if:
+            //   - We're on battery, and this is the battery providing power
+            //   - We're not on battery, and this battery is charging
+            //
+            if (
+                (on_battery && power_supply) ||
+                (
+                    !on_battery &&
+                    (
+                        state == UP_DEVICE_STATE_CHARGING ||
+                        state == UP_DEVICE_STATE_FULLY_CHARGED
+                    )
+                )
+            )
+            {
+                power->up_device_battery = g_object_ref(device);
+                break;
+            }
+        }
+    }
+
+    g_ptr_array_unref(all_devices);
+
+    // Update the icon for current battery state
+    //
+    wintc_notification_power_update_icon(power);
+
+    // Connect up signals to monitor battery, assuming we have one?
+    //
+    if (!(power->up_device_battery))
     {
         return;
     }
 
-    update_battery_battery_level(power, device);
-
     g_signal_connect(
-        device,
+        power->up_device_battery,
         "notify::percentage",
         G_CALLBACK(on_up_device_battery_notify),
         power
     );
     g_signal_connect(
-        device,
+        power->up_device_battery,
         "notify::power-supply",
         G_CALLBACK(on_up_device_battery_notify),
         power
     );
     g_signal_connect(
-        device,
+        power->up_device_battery,
         "notify::state",
         G_CALLBACK(on_up_device_battery_notify),
         power
     );
 }
 
-static void update_battery_battery_level(
+static void wintc_notification_power_update_icon(
-    WinTCNotificationPower* power,
+    WinTCNotificationPower* power
-    UpDevice*               device
 )
 {
-    gdouble  percentage;
+    // Handle the situation where we have no battery, even if we're supposedly
-    gboolean power_supply;
+    // on battery supply
-    guint    state;
+    //
-
+    if (!(power->up_device_battery))
-    g_object_get(
-        device,
-        "percentage",   &percentage,
-        "power-supply", &power_supply,
-        "state",        &state,
-        NULL
-    );
-
-    if (!power_supply)
     {
+        if (up_client_get_on_battery(power->up_client))
+        {
+            g_object_set(
+                power,
+                "icon-name", "battery-missing",
+                NULL
+            );
+        }
+        else
+        {
+            g_object_set(
+                power,
+                "icon-name", "ac-adapter",
+                NULL
+            );
+        }
+
         return;
     }
 
-    // Update icon based on levels
+    // Okay we have a battery, so update the icon for it
     //
     // We do not use 'battery-level' from the upower API, because it's not
     // reliable (reports UP_DEVICE_LEVEL_NONE) -- so just use percentage
     //
-    const gchar* icon_name;
+    gdouble  percentage;
+    guint    state;
+
+    g_object_get(
+        power->up_device_battery,
+        "percentage",   &percentage,
+        "state",        &state,
+        NULL
+    );
 
     UpDeviceLevel battery_level = battery_pct_to_enum(percentage);
+    const gchar*  icon_name;
     gboolean      is_charging   = state == UP_DEVICE_STATE_CHARGING;
 
     switch (battery_level)
@@ -330,50 +342,33 @@ static void update_battery_battery_level(
         "icon-name", icon_name,
         NULL
     );
-
-    // Keep track of last battery, for when switching to-from AC power
-    //
-    const gchar* this_battery_path = up_device_get_object_path(device);
-
-    if (g_strcmp0(power->up_last_battery_path, this_battery_path) != 0)
-    {
-        g_free(power->up_last_battery_path);
-
-        power->up_last_battery      = device;
-        power->up_last_battery_path = g_strdup(this_battery_path);
-    }
 }
 
-static void update_client_on_battery(
+static UpDeviceLevel battery_pct_to_enum(
-    WinTCNotificationPower* power,
+    gdouble percentage
-    UpClient*               client
 )
 {
-    if (up_client_get_on_battery(client))
+    // FIXME: Not checked against Windows XP
+    //
+    if (percentage <= 15.0f)
     {
-        if (power->up_last_battery)
+        return UP_DEVICE_LEVEL_CRITICAL;
-        {
+    }
-            update_battery_battery_level(power, power->up_last_battery);
+    else if (percentage <= 25.0f)
-        }
+    {
-        else
+        return UP_DEVICE_LEVEL_LOW;
-        {
+    }
-            g_object_set(
+    else if (percentage <= 75.0f)
-                power,
+    {
-                "icon-name", "battery-missing",
+        return UP_DEVICE_LEVEL_NORMAL;
-                NULL
+    }
-            );
+    else if (percentage <= 90.0f)
-        }
+    {
+        return UP_DEVICE_LEVEL_HIGH;
     }
     else
     {
-        if (!power->up_last_battery)
+        return UP_DEVICE_LEVEL_FULL;
-        {
-            g_object_set(
-                power,
-                "icon-name", "ac-adapter",
-                NULL
-            );
-        }
     }
 }
 
@@ -382,48 +377,44 @@ static void update_client_on_battery(
 //
 static void on_up_client_device_added(
     WINTC_UNUSED(UpClient* up_client),
-    UpDevice* up_device,
+    WINTC_UNUSED(UpDevice* up_device),
-    gpointer  user_data
+    gpointer user_data
 )
 {
     WinTCNotificationPower* power = WINTC_NOTIFICATION_POWER(user_data);
 
-    check_and_register_battery(power, up_device);
+    wintc_notification_power_register_main_battery(power);
 }
 
 static void on_up_client_device_removed(
-    WINTC_UNUSED(UpClient*    up_client),
+    WINTC_UNUSED(UpClient* up_client),
-    const gchar* object_path,
+    WINTC_UNUSED(const gchar* object_path),
-    gpointer     user_data
+    gpointer user_data
 )
 {
     WinTCNotificationPower* power = WINTC_NOTIFICATION_POWER(user_data);
 
-    if (g_strcmp0(power->up_last_battery_path, object_path))
+    wintc_notification_power_register_main_battery(power);
-    {
-        power->up_last_battery = NULL;
-        g_free(power->up_last_battery_path);
-    }
 }
 
 static void on_up_client_battery_notify(
-    UpClient* up_client,
+    WINTC_UNUSED(UpClient* up_client),
     WINTC_UNUSED(GParamSpec* pspec),
-    gpointer  user_data
+    gpointer user_data
 )
 {
     WinTCNotificationPower* power = WINTC_NOTIFICATION_POWER(user_data);
 
-    update_client_on_battery(power, up_client);
+    wintc_notification_power_register_main_battery(power);
 }
 
 static void on_up_device_battery_notify(
-    UpDevice* up_device,
+    WINTC_UNUSED(UpDevice* up_device),
     WINTC_UNUSED(GParamSpec* pspec),
-    gpointer  user_data
+    gpointer user_data
 )
 {
     WinTCNotificationPower* power = WINTC_NOTIFICATION_POWER(user_data);
 
-    update_battery_battery_level(power, up_device);
+    wintc_notification_power_update_icon(power);
 }