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

Proposed by Kai-Heng Feng on 2015-11-13
Status: Rejected
Rejected by: Lars Karlitski on 2015-11-13
Proposed branch: lp:~kaihengfeng/unity-settings-daemon/add-brightness-limit-mechanism
Merge into: lp:unity-settings-daemon
Diff against target: 256 lines (+85/-13)
3 files modified
plugins/power/gpm-common.c (+32/-5)
plugins/power/gpm-common.h (+15/-4)
plugins/power/gsd-power-manager.c (+38/-4)
To merge this branch: bzr merge lp:~kaihengfeng/unity-settings-daemon/add-brightness-limit-mechanism
Reviewer Review Type Date Requested Status
Lars Karlitski (community) Disapprove on 2015-11-13
Martin Pitt 2015-11-13 Disapprove on 2015-11-13
Ubuntu Desktop 2015-11-13 Pending
Review via email: mp+277416@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Martin Pitt (pitti) wrote :

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) wrote :

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) wrote :

> 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) wrote :

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) wrote :

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) wrote :

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) wrote :

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

Mateo Salta (mateo-salta) wrote :

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".

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/power/gpm-common.c'
2--- plugins/power/gpm-common.c 2014-06-26 00:02:19 +0000
3+++ plugins/power/gpm-common.c 2015-11-13 07:45:35 +0000
4@@ -1414,7 +1414,10 @@
5 }
6
7 int
8-backlight_get_percentage (GsdRRScreen *rr_screen, GError **error)
9+backlight_get_percentage (GsdRRScreen *rr_screen,
10+ gfloat min_percentage,
11+ gfloat max_percentage,
12+ GError **error)
13 {
14 GsdRROutput *output;
15 gint now;
16@@ -1429,6 +1432,8 @@
17 min = gsd_rr_output_get_backlight_min (output);
18 max = gsd_rr_output_get_backlight_max (output);
19 now = gsd_rr_output_get_backlight (output, error);
20+ min = MAX (min, max * min_percentage);
21+ max = max * max_percentage;
22 if (now < 0)
23 goto out;
24 value = ABS_TO_PERCENTAGE (min, max, now);
25@@ -1442,6 +1447,8 @@
26 now = backlight_helper_get_value ("get-brightness", error);
27 if (now < 0)
28 goto out;
29+ min = MAX (min, max * min_percentage);
30+ max = max * max_percentage;
31 value = ABS_TO_PERCENTAGE (min, max, now);
32 out:
33 return value;
34@@ -1488,6 +1495,8 @@
35 gboolean
36 backlight_set_percentage (GsdRRScreen *rr_screen,
37 guint value,
38+ gfloat min_percentage,
39+ gfloat max_percentage,
40 GError **error)
41 {
42 GsdRROutput *output;
43@@ -1505,6 +1514,8 @@
44 g_warning ("no xrandr backlight capability");
45 return ret;
46 }
47+ min = MAX (min, max * min_percentage);
48+ max = max * max_percentage;
49 discrete = PERCENTAGE_TO_ABS (min, max, value);
50 ret = gsd_rr_output_set_backlight (output,
51 discrete,
52@@ -1516,6 +1527,8 @@
53 max = backlight_helper_get_value ("get-max-brightness", error);
54 if (max < 0)
55 return ret;
56+ min = MAX (min, max * min_percentage);
57+ max = max * max_percentage;
58 discrete = PERCENTAGE_TO_ABS (min, max, value);
59 ret = backlight_helper_set_value ("set-brightness",
60 discrete,
61@@ -1525,7 +1538,10 @@
62 }
63
64 int
65-backlight_step_up (GsdRRScreen *rr_screen, GError **error)
66+backlight_step_up (GsdRRScreen *rr_screen,
67+ gfloat min_percentage,
68+ gfloat max_percentage,
69+ GError **error)
70 {
71 GsdRROutput *output;
72 gboolean ret = FALSE;
73@@ -1555,6 +1571,8 @@
74 now = gsd_rr_output_get_backlight (output, error);
75 if (now < 0)
76 return percentage_value;
77+ min = MAX (min, max * min_percentage);
78+ max = max * max_percentage;
79 step = BRIGHTNESS_STEP_AMOUNT (max - min + 1);
80 discrete = MIN (now + step, max);
81 ret = gsd_rr_output_set_backlight (output,
82@@ -1572,6 +1590,8 @@
83 max = backlight_helper_get_value ("get-max-brightness", error);
84 if (max < 0)
85 return percentage_value;
86+ min = MAX (min, max * min_percentage);
87+ max = max * max_percentage;
88 step = BRIGHTNESS_STEP_AMOUNT (max - min + 1);
89 discrete = MIN (now + step, max);
90 ret = backlight_helper_set_value ("set-brightness",
91@@ -1584,7 +1604,10 @@
92 }
93
94 int
95-backlight_step_down (GsdRRScreen *rr_screen, GError **error)
96+backlight_step_down (GsdRRScreen *rr_screen,
97+ gfloat min_percentage,
98+ gfloat max_percentage,
99+ GError **error)
100 {
101 GsdRROutput *output;
102 gboolean ret = FALSE;
103@@ -1614,8 +1637,10 @@
104 now = gsd_rr_output_get_backlight (output, error);
105 if (now < 0)
106 return percentage_value;
107+ min = MAX (min, max * min_percentage);
108+ max = max * max_percentage;
109 step = BRIGHTNESS_STEP_AMOUNT (max - min + 1);
110- discrete = MAX (now - step, 0);
111+ discrete = MAX (now - step, min);
112 ret = gsd_rr_output_set_backlight (output,
113 discrete,
114 error);
115@@ -1631,8 +1656,10 @@
116 max = backlight_helper_get_value ("get-max-brightness", error);
117 if (max < 0)
118 return percentage_value;
119+ min = MAX (min, max * min_percentage);
120+ max = max * max_percentage;
121 step = BRIGHTNESS_STEP_AMOUNT (max - min + 1);
122- discrete = MAX (now - step, 0);
123+ discrete = MAX (now - step, min);
124 ret = backlight_helper_set_value ("set-brightness",
125 discrete,
126 error);
127
128=== modified file 'plugins/power/gpm-common.h'
129--- plugins/power/gpm-common.h 2014-06-26 00:02:19 +0000
130+++ plugins/power/gpm-common.h 2015-11-13 07:45:35 +0000
131@@ -58,14 +58,25 @@
132 int gsd_power_backlight_abs_to_percentage (int min, int max, int value);
133 gboolean backlight_available (GsdRRScreen *rr_screen);
134 int backlight_get_abs (GsdRRScreen *rr_screen, GError **error);
135-int backlight_get_percentage (GsdRRScreen *rr_screen, GError **error);
136+int backlight_get_percentage (GsdRRScreen *rr_screen,
137+ gfloat min_percentage,
138+ gfloat max_percentage,
139+ GError **error);
140 int backlight_get_min (GsdRRScreen *rr_screen);
141 int backlight_get_max (GsdRRScreen *rr_screen, GError **error);
142 gboolean backlight_set_percentage (GsdRRScreen *rr_screen,
143 guint value,
144- GError **error);
145-int backlight_step_up (GsdRRScreen *rr_screen, GError **error);
146-int backlight_step_down (GsdRRScreen *rr_screen, GError **error);
147+ gfloat min_percentage,
148+ gfloat max_percentage,
149+ GError **error);
150+int backlight_step_up (GsdRRScreen *rr_screen,
151+ gfloat min_percentage,
152+ gfloat max_percentage,
153+ GError **error);
154+int backlight_step_down (GsdRRScreen *rr_screen,
155+ gfloat min_percentage,
156+ gfloat max_percentage,
157+ GError **error);
158 int backlight_set_abs (GsdRRScreen *rr_screen,
159 guint value,
160 GError **error);
161
162=== modified file 'plugins/power/gsd-power-manager.c'
163--- plugins/power/gsd-power-manager.c 2015-10-22 09:04:20 +0000
164+++ plugins/power/gsd-power-manager.c 2015-11-13 07:45:35 +0000
165@@ -189,6 +189,8 @@
166 /* Brightness */
167 gboolean backlight_available;
168 gint pre_dim_brightness; /* level, not percentage */
169+ gfloat min_brightness_percentage;
170+ gfloat max_brightness_percentage;
171
172 /* Keyboard */
173 GDBusProxy *upower_kdb_proxy;
174@@ -2626,6 +2628,15 @@
175 }
176
177 static void
178+brightness_configure (GsdPowerManager *manager)
179+{
180+ gint min = CLAMP (g_settings_get_int (manager->priv->settings, "min-brightness-percentage"), 0, 100);
181+ gint max = CLAMP (g_settings_get_int (manager->priv->settings, "max-brightness-percentage"), min, 100);
182+ manager->priv->min_brightness_percentage = min * 0.01;
183+ manager->priv->max_brightness_percentage = max * 0.01;
184+}
185+
186+static void
187 main_battery_or_ups_low_changed (GsdPowerManager *manager,
188 gboolean is_low)
189 {
190@@ -3013,6 +3024,11 @@
191 setup_lid_closed_action (manager);
192 return;
193 }
194+ if (g_str_equal (key, "min-brightness-percentage") ||
195+ g_str_equal (key, "max-brightness-percentage")) {
196+ brightness_configure (manager);
197+ return;
198+ }
199 }
200
201 static void
202@@ -3416,6 +3432,7 @@
203 /* coldplug the engine */
204 engine_coldplug (manager);
205 idle_configure (manager);
206+ brightness_configure (manager);
207
208 manager->priv->xscreensaver_watchdog_timer_id = gsd_power_enable_screensaver_watchdog ();
209
210@@ -3590,12 +3607,21 @@
211
212 if (g_strcmp0 (method_name, "GetPercentage") == 0) {
213 g_debug ("screen get percentage");
214- value = backlight_get_percentage (manager->priv->rr_screen, &error);
215+ value = backlight_get_percentage (
216+ manager->priv->rr_screen,
217+ manager->priv->min_brightness_percentage,
218+ manager->priv->max_brightness_percentage,
219+ &error);
220
221 } else if (g_strcmp0 (method_name, "SetPercentage") == 0) {
222 g_debug ("screen set percentage");
223 g_variant_get (parameters, "(u)", &value_tmp);
224- ret = backlight_set_percentage (manager->priv->rr_screen, value_tmp, &error);
225+ ret = backlight_set_percentage (
226+ manager->priv->rr_screen,
227+ value_tmp,
228+ manager->priv->min_brightness_percentage,
229+ manager->priv->max_brightness_percentage,
230+ &error);
231 if (ret) {
232 value = value_tmp;
233 backlight_emit_changed (manager);
234@@ -3603,12 +3629,20 @@
235
236 } else if (g_strcmp0 (method_name, "StepUp") == 0) {
237 g_debug ("screen step up");
238- value = backlight_step_up (manager->priv->rr_screen, &error);
239+ value = backlight_step_up (
240+ manager->priv->rr_screen,
241+ manager->priv->min_brightness_percentage,
242+ manager->priv->max_brightness_percentage,
243+ &error);
244 if (value != -1)
245 backlight_emit_changed (manager);
246 } else if (g_strcmp0 (method_name, "StepDown") == 0) {
247 g_debug ("screen step down");
248- value = backlight_step_down (manager->priv->rr_screen, &error);
249+ value = backlight_step_down (
250+ manager->priv->rr_screen,
251+ manager->priv->min_brightness_percentage,
252+ manager->priv->max_brightness_percentage,
253+ &error);
254 if (value != -1)
255 backlight_emit_changed (manager);
256 } else {

Subscribers

People subscribed via source and target branches