After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 771881 - Low battery notification never refreshed (patch attached)
Low battery notification never refreshed (patch attached)
Status: RESOLVED OBSOLETE
Product: gnome-settings-daemon
Classification: Core
Component: power
3.23.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
: 770826 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2016-09-23 14:47 UTC by Gautier Pelloux-Prayer
Modified: 2019-03-20 11:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add patch connecting 2 more signals (1.89 KB, patch)
2016-09-23 14:47 UTC, Gautier Pelloux-Prayer
none Details | Review
add patch connecting 2 more signals (v2) (9.12 KB, patch)
2016-09-26 11:49 UTC, Gautier Pelloux-Prayer
none Details | Review
patch connecting 2 more signals (v3) (10.70 KB, patch)
2016-10-04 11:08 UTC, Gautier Pelloux-Prayer
needs-work Details | Review

Description Gautier Pelloux-Prayer 2016-09-23 14:47:27 UTC
Created attachment 336161 [details] [review]
add patch connecting 2 more signals

Currently, when a laptop goes low on battery, a notification pops up with a text:

_("Approximately %s remaining (%.0f%%)"), remaining_text, percentage);

However, this text is only refreshed when the battery status change of type (goes to critical, etc.). This is a problem since usually this notification appears when it remains 20 minutes and change to critical at 5 minutes left. During 15 minutes you will get a "Approximately 20 minutes" remaining while it's actually not.
 
Since the notification use the remaining time and percentage of available battery, I would advise to connect such signals as well so that notification is always up-to-date with system information.
Comment 1 Gautier Pelloux-Prayer 2016-09-26 11:49:40 UTC
Created attachment 336259 [details] [review]
add patch connecting 2 more signals (v2)

Actually instead of closing/creating notifications, it's better to directly update the existing one if possible. This avoid having a transitional fade-out/fade-in when notification must be refreshed.
Also in case of a user already dismissed notification, we should not present the notification again.
Comment 2 Gautier Pelloux-Prayer 2016-10-04 11:08:21 UTC
Created attachment 336892 [details] [review]
patch connecting 2 more signals (v3)

Re-indenting properly.
Comment 3 Gautier Pelloux-Prayer 2016-10-31 14:57:59 UTC
Is there anything left I can do to help solving that issue?
Comment 4 Gautier Pelloux-Prayer 2016-12-04 19:55:59 UTC
Hope to have it reviewed before next GNOME release! :)
Comment 5 Gautier Pelloux-Prayer 2017-01-22 21:04:58 UTC
Should I change product to gnome-shell? Shall I contact maintainers by any specific mean?
Comment 6 Bastien Nocera 2017-01-23 08:57:17 UTC
No, you just need to be patient.
Comment 7 Bastien Nocera 2017-01-24 10:50:28 UTC
Review of attachment 336892 [details] [review]:

> power manager: refresh low-battery notifications when battery
 percentage or time remaining change

"power: " not "power manager: "

The line is too long. This also only fixes the problem for builtin batteries or UPSes, eg. batteries that power the system, not for devices that are running low.

In any case, this would require much more changes than you've done so far. First, we need to make sure we have one "low" notification per device, that is 1 Notification for device_composite (laptop batteries and UPS) and one per device (mouse, keyboard, etc.). Then we can think of updating those individual notifications.

::: plugins/power/gsd-power-manager.c
@@ +319,3 @@
 {
+        if (*weak_pointer_location == NULL) {
+            NotifyNotification *notification;

8-spaces indentation, not 4.

@@ +332,3 @@
+                              G_CALLBACK (on_notification_closed), NULL);
+        } else {
+            notify_notification_update(*weak_pointer_location, summary, body, icon_name);

space before parenthesis.

@@ +780,3 @@
+    /* in case of data update, we want to refresh the notification only if user
+    did not already dismissed it*/
+    if (manager->priv->notification_low != NULL) {

This notification is shared by all devices, that is batteries powering the system, and batteries in wireless mice or keyboards. So there's no certainty that the "notification_low" notification will correspond to the same device you're receiving the update on.
Comment 8 Bastien Nocera 2017-03-13 13:08:59 UTC
*** Bug 770826 has been marked as a duplicate of this bug. ***
Comment 9 GNOME Infrastructure Team 2019-03-20 11:36:58 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/issues/305.