Merge lp:~kaihengfeng/unity-settings-daemon/add-brightness-limit-mechanism into lp:unity-settings-daemon

[Kai-Heng Feng]

Add a mechanism that can make user to choose the minimum/maximum adjustable brightness level.

[Martin Pitt (pitti)]

TBH, I'm not a fan of introducing configurability for such things. It sounds too much like an "unbreak my desktop setting" and a hack for a bug, it introduces combinatorial explosion again which nobody will test properly, and it'll be hard to drop such settings again.

My opinion is not authoritative here of course, it's ultimately the desktop teams' decision.

review: Disapprove

[Zygmunt Krynicki (zyga)]

I'm tentatively +1 on this. I agree with Martin that this is an "unbreak my system" knob but ultimately the world where I have this knob is better than the one we are in now, where desktops _are_ broken.

I would love to see an user-visible checkbox that controls if backlight turns off at 0% and a system-vendor tunable that actually controls the 0-100% span.

[Martin Pitt (pitti)]

> I would love to see an user-visible checkbox that controls if backlight turns off at 0% and a system-vendor tunable that actually controls the 0-100% span.

Erk, please no. This is the absolutely worst UI.

[Martin Pitt (pitti)]

Also, this high-level user space is *not* the point to second-guess/workaround kernel drivers. Userspace should use the brightness range that the kernel offers, and if there's somehting wrong with that it needs to be fixed in that driver.

[Lars Karlitski (larsu)]

I strongly agree with Martin's remarks. This should be fixed in the respective drivers. Not only because that's the conceptually correct place, but also because otherwise we'll have to hack around this in every place that brightness is controlled in user space.

> I would love to see an user-visible checkbox that controls if backlight turns off at 0% and a system-vendor tunable that actually controls the 0-100% span.

Why would anyone ever want to turn off the backlight for LCD screens? Turning down the brightness is not the same as turning off the screen.

review: Disapprove

[Martin Pitt (pitti)]

FTR, https://code.launchpad.net/~fourdollars/unity-settings-daemon/fix-lowest-brightness/+merge/277326 addresses the same problem in an (IMHO) much better way, as it keeps the heuristics in the code (and thus changeable) without providing extra knobs.

[Kai-Heng Feng (kaihengfeng)]

Although I still prefer configuration for such an issue, I am happy to see this issue being addressed.

[Mateo Salta (mateo-salta)]

please no, I only see thing on windows devices to trick the battery life stats - setting it lower on battery vs plugged in is one thing(user can still increase to actual max), but forcing a limit that you can't turn up past unless you go into settings is a bad idea.

Unmerged revisions

4118. By Kai-Heng Feng on 2015-11-13

power: Add a mechanism that can limit min/max brightness percentage via gsettings.

In attempt to fix #1381625, I think it should be the user to decide the minimun
brightness policy to be "screen dim" or "screen off". This newly added mechanism
does not change the current behavior, user can decide it by changing
"min-brightness-percentage".

=== modified file 'plugins/power/gpm-common.c'
--- plugins/power/gpm-common.c	2014-06-26 00:02:19 +0000
+++ plugins/power/gpm-common.c	2015-11-13 07:45:35 +0000
@@ -1414,7 +1414,10 @@
 }
 
 int
-backlight_get_percentage (GsdRRScreen *rr_screen, GError **error)
+backlight_get_percentage (GsdRRScreen *rr_screen,
+                          gfloat min_percentage,
+                          gfloat max_percentage,
+                          GError **error)
 {
         GsdRROutput *output;
         gint now;
@@ -1429,6 +1432,8 @@
                 min = gsd_rr_output_get_backlight_min (output);
                 max = gsd_rr_output_get_backlight_max (output);
                 now = gsd_rr_output_get_backlight (output, error);
+                min = MAX (min, max * min_percentage);
+                max = max * max_percentage;
                 if (now < 0)
                         goto out;
                 value = ABS_TO_PERCENTAGE (min, max, now);
@@ -1442,6 +1447,8 @@
         now = backlight_helper_get_value ("get-brightness", error);
         if (now < 0)
                 goto out;
+        min = MAX (min, max * min_percentage);
+        max = max * max_percentage;
         value = ABS_TO_PERCENTAGE (min, max, now);
 out:
         return value;
@@ -1488,6 +1495,8 @@
 gboolean
 backlight_set_percentage (GsdRRScreen *rr_screen,
                           guint value,
+                          gfloat min_percentage,
+                          gfloat max_percentage,
                           GError **error)
 {
         GsdRROutput *output;
@@ -1505,6 +1514,8 @@
                         g_warning ("no xrandr backlight capability");
                         return ret;
                 }
+                min = MAX (min, max * min_percentage);
+                max = max * max_percentage;
                 discrete = PERCENTAGE_TO_ABS (min, max, value);
                 ret = gsd_rr_output_set_backlight (output,
                                                      discrete,
@@ -1516,6 +1527,8 @@
         max = backlight_helper_get_value ("get-max-brightness", error);
         if (max < 0)
                 return ret;
+        min = MAX (min, max * min_percentage);
+        max = max * max_percentage;
         discrete = PERCENTAGE_TO_ABS (min, max, value);
         ret = backlight_helper_set_value ("set-brightness",
                                           discrete,
@@ -1525,7 +1538,10 @@
 }
 
 int
-backlight_step_up (GsdRRScreen *rr_screen, GError **error)
+backlight_step_up (GsdRRScreen *rr_screen,
+                   gfloat min_percentage,
+                   gfloat max_percentage,
+                   GError **error)
 {
         GsdRROutput *output;
         gboolean ret = FALSE;
@@ -1555,6 +1571,8 @@
                 now = gsd_rr_output_get_backlight (output, error);
                 if (now < 0)
                        return percentage_value;
+                min = MAX (min, max * min_percentage);
+                max = max * max_percentage;
                 step = BRIGHTNESS_STEP_AMOUNT (max - min + 1);
                 discrete = MIN (now + step, max);
                 ret = gsd_rr_output_set_backlight (output,
@@ -1572,6 +1590,8 @@
         max = backlight_helper_get_value ("get-max-brightness", error);
         if (max < 0)
                 return percentage_value;
+        min = MAX (min, max * min_percentage);
+        max = max * max_percentage;
         step = BRIGHTNESS_STEP_AMOUNT (max - min + 1);
         discrete = MIN (now + step, max);
         ret = backlight_helper_set_value ("set-brightness",
@@ -1584,7 +1604,10 @@
 }
 
 int
-backlight_step_down (GsdRRScreen *rr_screen, GError **error)
+backlight_step_down (GsdRRScreen *rr_screen,
+                     gfloat min_percentage,
+                     gfloat max_percentage,
+                     GError **error)
 {
         GsdRROutput *output;
         gboolean ret = FALSE;
@@ -1614,8 +1637,10 @@
                 now = gsd_rr_output_get_backlight (output, error);
                 if (now < 0)
                        return percentage_value;
+                min = MAX (min, max * min_percentage);
+                max = max * max_percentage;
                 step = BRIGHTNESS_STEP_AMOUNT (max - min + 1);
-                discrete = MAX (now - step, 0);
+                discrete = MAX (now - step, min);
                 ret = gsd_rr_output_set_backlight (output,
                                                      discrete,
                                                      error);
@@ -1631,8 +1656,10 @@
         max = backlight_helper_get_value ("get-max-brightness", error);
         if (max < 0)
                 return percentage_value;
+        min = MAX (min, max * min_percentage);
+        max = max * max_percentage;
         step = BRIGHTNESS_STEP_AMOUNT (max - min + 1);
-        discrete = MAX (now - step, 0);
+        discrete = MAX (now - step, min);
         ret = backlight_helper_set_value ("set-brightness",
                                           discrete,
                                           error);

=== modified file 'plugins/power/gpm-common.h'
--- plugins/power/gpm-common.h	2014-06-26 00:02:19 +0000
+++ plugins/power/gpm-common.h	2015-11-13 07:45:35 +0000
@@ -58,14 +58,25 @@
 int              gsd_power_backlight_abs_to_percentage  (int min, int max, int value);
 gboolean         backlight_available                    (GsdRRScreen *rr_screen);
 int              backlight_get_abs                      (GsdRRScreen *rr_screen, GError **error);
-int              backlight_get_percentage               (GsdRRScreen *rr_screen, GError **error);
+int              backlight_get_percentage               (GsdRRScreen *rr_screen,
+                                                         gfloat min_percentage,
+                                                         gfloat max_percentage,
+                                                         GError **error);
 int              backlight_get_min                      (GsdRRScreen *rr_screen);
 int              backlight_get_max                      (GsdRRScreen *rr_screen, GError **error);
 gboolean         backlight_set_percentage               (GsdRRScreen *rr_screen,
                                                          guint value,
-                                                         GError **error);
-int              backlight_step_up                      (GsdRRScreen *rr_screen, GError **error);
-int              backlight_step_down                    (GsdRRScreen *rr_screen, GError **error);
+                                                         gfloat min_percentage,
+                                                         gfloat max_percentage,
+                                                         GError **error);
+int              backlight_step_up                      (GsdRRScreen *rr_screen,
+                                                         gfloat min_percentage,
+                                                         gfloat max_percentage,
+                                                         GError **error);
+int              backlight_step_down                    (GsdRRScreen *rr_screen,
+                                                         gfloat min_percentage,
+                                                         gfloat max_percentage,
+                                                         GError **error);
 int              backlight_set_abs                      (GsdRRScreen *rr_screen,
                                                          guint value,
                                                          GError **error);

=== modified file 'plugins/power/gsd-power-manager.c'
--- plugins/power/gsd-power-manager.c	2015-10-22 09:04:20 +0000
+++ plugins/power/gsd-power-manager.c	2015-11-13 07:45:35 +0000
@@ -189,6 +189,8 @@
         /* Brightness */
         gboolean                 backlight_available;
         gint                     pre_dim_brightness; /* level, not percentage */
+        gfloat                   min_brightness_percentage;
+        gfloat                   max_brightness_percentage;
 
         /* Keyboard */
         GDBusProxy              *upower_kdb_proxy;
@@ -2626,6 +2628,15 @@
 }
 
 static void
+brightness_configure (GsdPowerManager *manager)
+{
+        gint min = CLAMP (g_settings_get_int (manager->priv->settings, "min-brightness-percentage"), 0, 100);
+        gint max = CLAMP (g_settings_get_int (manager->priv->settings, "max-brightness-percentage"), min, 100);
+        manager->priv->min_brightness_percentage = min * 0.01;
+        manager->priv->max_brightness_percentage = max * 0.01;
+}
+
+static void
 main_battery_or_ups_low_changed (GsdPowerManager *manager,
                           gboolean         is_low)
 {
@@ -3013,6 +3024,11 @@
                 setup_lid_closed_action (manager);
                 return;
         }
+        if (g_str_equal (key, "min-brightness-percentage") ||
+            g_str_equal (key, "max-brightness-percentage")) {
+                brightness_configure (manager);
+                return;
+        }
 }
 
 static void
@@ -3416,6 +3432,7 @@
         /* coldplug the engine */
         engine_coldplug (manager);
         idle_configure (manager);
+        brightness_configure (manager);
 
         manager->priv->xscreensaver_watchdog_timer_id = gsd_power_enable_screensaver_watchdog ();
 
@@ -3590,12 +3607,21 @@
 
         if (g_strcmp0 (method_name, "GetPercentage") == 0) {
                 g_debug ("screen get percentage");
-                value = backlight_get_percentage (manager->priv->rr_screen, &error);
+                value = backlight_get_percentage (
+                        manager->priv->rr_screen,
+                        manager->priv->min_brightness_percentage,
+                        manager->priv->max_brightness_percentage,
+                        &error);
 
         } else if (g_strcmp0 (method_name, "SetPercentage") == 0) {
                 g_debug ("screen set percentage");
                 g_variant_get (parameters, "(u)", &value_tmp);
-                ret = backlight_set_percentage (manager->priv->rr_screen, value_tmp, &error);
+                ret = backlight_set_percentage (
+                        manager->priv->rr_screen,
+                        value_tmp,
+                        manager->priv->min_brightness_percentage,
+                        manager->priv->max_brightness_percentage,
+                        &error);
                 if (ret) {
                         value = value_tmp;
                         backlight_emit_changed (manager);
@@ -3603,12 +3629,20 @@
 
         } else if (g_strcmp0 (method_name, "StepUp") == 0) {
                 g_debug ("screen step up");
-                value = backlight_step_up (manager->priv->rr_screen, &error);
+                value = backlight_step_up (
+                        manager->priv->rr_screen,
+                        manager->priv->min_brightness_percentage,
+                        manager->priv->max_brightness_percentage,
+                        &error);
                 if (value != -1)
                         backlight_emit_changed (manager);
         } else if (g_strcmp0 (method_name, "StepDown") == 0) {
                 g_debug ("screen step down");
-                value = backlight_step_down (manager->priv->rr_screen, &error);
+                value = backlight_step_down (
+                        manager->priv->rr_screen,
+                        manager->priv->min_brightness_percentage,
+                        manager->priv->max_brightness_percentage,
+                        &error);
                 if (value != -1)
                         backlight_emit_changed (manager);
         } else {