Merge lp:~kaihengfeng/unity-settings-daemon/add-brightness-limit-mechanism into lp:unity-settings-daemon
- add-brightness-limit-mechanism
- Merge into trunk
Status: | Rejected |
---|---|
Rejected by: | Lars Karlitski |
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 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Lars Karlitski (community) | Disapprove | ||
Martin Pitt | Disapprove | ||
Ubuntu Desktop | Pending | ||
Review via email: mp+277416@code.launchpad.net |
Commit message
Description of the change
Add a mechanism that can make user to choose the minimum/maximum adjustable brightness level.
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-
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.
Martin Pitt (pitti) wrote : | # |
FTR, https:/
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
-
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
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 | 1414 | } | 1414 | } |
6 | 1415 | 1415 | ||
7 | 1416 | int | 1416 | int |
9 | 1417 | backlight_get_percentage (GsdRRScreen *rr_screen, GError **error) | 1417 | backlight_get_percentage (GsdRRScreen *rr_screen, |
10 | 1418 | gfloat min_percentage, | ||
11 | 1419 | gfloat max_percentage, | ||
12 | 1420 | GError **error) | ||
13 | 1418 | { | 1421 | { |
14 | 1419 | GsdRROutput *output; | 1422 | GsdRROutput *output; |
15 | 1420 | gint now; | 1423 | gint now; |
16 | @@ -1429,6 +1432,8 @@ | |||
17 | 1429 | min = gsd_rr_output_get_backlight_min (output); | 1432 | min = gsd_rr_output_get_backlight_min (output); |
18 | 1430 | max = gsd_rr_output_get_backlight_max (output); | 1433 | max = gsd_rr_output_get_backlight_max (output); |
19 | 1431 | now = gsd_rr_output_get_backlight (output, error); | 1434 | now = gsd_rr_output_get_backlight (output, error); |
20 | 1435 | min = MAX (min, max * min_percentage); | ||
21 | 1436 | max = max * max_percentage; | ||
22 | 1432 | if (now < 0) | 1437 | if (now < 0) |
23 | 1433 | goto out; | 1438 | goto out; |
24 | 1434 | value = ABS_TO_PERCENTAGE (min, max, now); | 1439 | value = ABS_TO_PERCENTAGE (min, max, now); |
25 | @@ -1442,6 +1447,8 @@ | |||
26 | 1442 | now = backlight_helper_get_value ("get-brightness", error); | 1447 | now = backlight_helper_get_value ("get-brightness", error); |
27 | 1443 | if (now < 0) | 1448 | if (now < 0) |
28 | 1444 | goto out; | 1449 | goto out; |
29 | 1450 | min = MAX (min, max * min_percentage); | ||
30 | 1451 | max = max * max_percentage; | ||
31 | 1445 | value = ABS_TO_PERCENTAGE (min, max, now); | 1452 | value = ABS_TO_PERCENTAGE (min, max, now); |
32 | 1446 | out: | 1453 | out: |
33 | 1447 | return value; | 1454 | return value; |
34 | @@ -1488,6 +1495,8 @@ | |||
35 | 1488 | gboolean | 1495 | gboolean |
36 | 1489 | backlight_set_percentage (GsdRRScreen *rr_screen, | 1496 | backlight_set_percentage (GsdRRScreen *rr_screen, |
37 | 1490 | guint value, | 1497 | guint value, |
38 | 1498 | gfloat min_percentage, | ||
39 | 1499 | gfloat max_percentage, | ||
40 | 1491 | GError **error) | 1500 | GError **error) |
41 | 1492 | { | 1501 | { |
42 | 1493 | GsdRROutput *output; | 1502 | GsdRROutput *output; |
43 | @@ -1505,6 +1514,8 @@ | |||
44 | 1505 | g_warning ("no xrandr backlight capability"); | 1514 | g_warning ("no xrandr backlight capability"); |
45 | 1506 | return ret; | 1515 | return ret; |
46 | 1507 | } | 1516 | } |
47 | 1517 | min = MAX (min, max * min_percentage); | ||
48 | 1518 | max = max * max_percentage; | ||
49 | 1508 | discrete = PERCENTAGE_TO_ABS (min, max, value); | 1519 | discrete = PERCENTAGE_TO_ABS (min, max, value); |
50 | 1509 | ret = gsd_rr_output_set_backlight (output, | 1520 | ret = gsd_rr_output_set_backlight (output, |
51 | 1510 | discrete, | 1521 | discrete, |
52 | @@ -1516,6 +1527,8 @@ | |||
53 | 1516 | max = backlight_helper_get_value ("get-max-brightness", error); | 1527 | max = backlight_helper_get_value ("get-max-brightness", error); |
54 | 1517 | if (max < 0) | 1528 | if (max < 0) |
55 | 1518 | return ret; | 1529 | return ret; |
56 | 1530 | min = MAX (min, max * min_percentage); | ||
57 | 1531 | max = max * max_percentage; | ||
58 | 1519 | discrete = PERCENTAGE_TO_ABS (min, max, value); | 1532 | discrete = PERCENTAGE_TO_ABS (min, max, value); |
59 | 1520 | ret = backlight_helper_set_value ("set-brightness", | 1533 | ret = backlight_helper_set_value ("set-brightness", |
60 | 1521 | discrete, | 1534 | discrete, |
61 | @@ -1525,7 +1538,10 @@ | |||
62 | 1525 | } | 1538 | } |
63 | 1526 | 1539 | ||
64 | 1527 | int | 1540 | int |
66 | 1528 | backlight_step_up (GsdRRScreen *rr_screen, GError **error) | 1541 | backlight_step_up (GsdRRScreen *rr_screen, |
67 | 1542 | gfloat min_percentage, | ||
68 | 1543 | gfloat max_percentage, | ||
69 | 1544 | GError **error) | ||
70 | 1529 | { | 1545 | { |
71 | 1530 | GsdRROutput *output; | 1546 | GsdRROutput *output; |
72 | 1531 | gboolean ret = FALSE; | 1547 | gboolean ret = FALSE; |
73 | @@ -1555,6 +1571,8 @@ | |||
74 | 1555 | now = gsd_rr_output_get_backlight (output, error); | 1571 | now = gsd_rr_output_get_backlight (output, error); |
75 | 1556 | if (now < 0) | 1572 | if (now < 0) |
76 | 1557 | return percentage_value; | 1573 | return percentage_value; |
77 | 1574 | min = MAX (min, max * min_percentage); | ||
78 | 1575 | max = max * max_percentage; | ||
79 | 1558 | step = BRIGHTNESS_STEP_AMOUNT (max - min + 1); | 1576 | step = BRIGHTNESS_STEP_AMOUNT (max - min + 1); |
80 | 1559 | discrete = MIN (now + step, max); | 1577 | discrete = MIN (now + step, max); |
81 | 1560 | ret = gsd_rr_output_set_backlight (output, | 1578 | ret = gsd_rr_output_set_backlight (output, |
82 | @@ -1572,6 +1590,8 @@ | |||
83 | 1572 | max = backlight_helper_get_value ("get-max-brightness", error); | 1590 | max = backlight_helper_get_value ("get-max-brightness", error); |
84 | 1573 | if (max < 0) | 1591 | if (max < 0) |
85 | 1574 | return percentage_value; | 1592 | return percentage_value; |
86 | 1593 | min = MAX (min, max * min_percentage); | ||
87 | 1594 | max = max * max_percentage; | ||
88 | 1575 | step = BRIGHTNESS_STEP_AMOUNT (max - min + 1); | 1595 | step = BRIGHTNESS_STEP_AMOUNT (max - min + 1); |
89 | 1576 | discrete = MIN (now + step, max); | 1596 | discrete = MIN (now + step, max); |
90 | 1577 | ret = backlight_helper_set_value ("set-brightness", | 1597 | ret = backlight_helper_set_value ("set-brightness", |
91 | @@ -1584,7 +1604,10 @@ | |||
92 | 1584 | } | 1604 | } |
93 | 1585 | 1605 | ||
94 | 1586 | int | 1606 | int |
96 | 1587 | backlight_step_down (GsdRRScreen *rr_screen, GError **error) | 1607 | backlight_step_down (GsdRRScreen *rr_screen, |
97 | 1608 | gfloat min_percentage, | ||
98 | 1609 | gfloat max_percentage, | ||
99 | 1610 | GError **error) | ||
100 | 1588 | { | 1611 | { |
101 | 1589 | GsdRROutput *output; | 1612 | GsdRROutput *output; |
102 | 1590 | gboolean ret = FALSE; | 1613 | gboolean ret = FALSE; |
103 | @@ -1614,8 +1637,10 @@ | |||
104 | 1614 | now = gsd_rr_output_get_backlight (output, error); | 1637 | now = gsd_rr_output_get_backlight (output, error); |
105 | 1615 | if (now < 0) | 1638 | if (now < 0) |
106 | 1616 | return percentage_value; | 1639 | return percentage_value; |
107 | 1640 | min = MAX (min, max * min_percentage); | ||
108 | 1641 | max = max * max_percentage; | ||
109 | 1617 | step = BRIGHTNESS_STEP_AMOUNT (max - min + 1); | 1642 | step = BRIGHTNESS_STEP_AMOUNT (max - min + 1); |
111 | 1618 | discrete = MAX (now - step, 0); | 1643 | discrete = MAX (now - step, min); |
112 | 1619 | ret = gsd_rr_output_set_backlight (output, | 1644 | ret = gsd_rr_output_set_backlight (output, |
113 | 1620 | discrete, | 1645 | discrete, |
114 | 1621 | error); | 1646 | error); |
115 | @@ -1631,8 +1656,10 @@ | |||
116 | 1631 | max = backlight_helper_get_value ("get-max-brightness", error); | 1656 | max = backlight_helper_get_value ("get-max-brightness", error); |
117 | 1632 | if (max < 0) | 1657 | if (max < 0) |
118 | 1633 | return percentage_value; | 1658 | return percentage_value; |
119 | 1659 | min = MAX (min, max * min_percentage); | ||
120 | 1660 | max = max * max_percentage; | ||
121 | 1634 | step = BRIGHTNESS_STEP_AMOUNT (max - min + 1); | 1661 | step = BRIGHTNESS_STEP_AMOUNT (max - min + 1); |
123 | 1635 | discrete = MAX (now - step, 0); | 1662 | discrete = MAX (now - step, min); |
124 | 1636 | ret = backlight_helper_set_value ("set-brightness", | 1663 | ret = backlight_helper_set_value ("set-brightness", |
125 | 1637 | discrete, | 1664 | discrete, |
126 | 1638 | error); | 1665 | error); |
127 | 1639 | 1666 | ||
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 | 58 | int gsd_power_backlight_abs_to_percentage (int min, int max, int value); | 58 | int gsd_power_backlight_abs_to_percentage (int min, int max, int value); |
133 | 59 | gboolean backlight_available (GsdRRScreen *rr_screen); | 59 | gboolean backlight_available (GsdRRScreen *rr_screen); |
134 | 60 | int backlight_get_abs (GsdRRScreen *rr_screen, GError **error); | 60 | int backlight_get_abs (GsdRRScreen *rr_screen, GError **error); |
136 | 61 | int backlight_get_percentage (GsdRRScreen *rr_screen, GError **error); | 61 | int backlight_get_percentage (GsdRRScreen *rr_screen, |
137 | 62 | gfloat min_percentage, | ||
138 | 63 | gfloat max_percentage, | ||
139 | 64 | GError **error); | ||
140 | 62 | int backlight_get_min (GsdRRScreen *rr_screen); | 65 | int backlight_get_min (GsdRRScreen *rr_screen); |
141 | 63 | int backlight_get_max (GsdRRScreen *rr_screen, GError **error); | 66 | int backlight_get_max (GsdRRScreen *rr_screen, GError **error); |
142 | 64 | gboolean backlight_set_percentage (GsdRRScreen *rr_screen, | 67 | gboolean backlight_set_percentage (GsdRRScreen *rr_screen, |
143 | 65 | guint value, | 68 | guint value, |
147 | 66 | GError **error); | 69 | gfloat min_percentage, |
148 | 67 | int backlight_step_up (GsdRRScreen *rr_screen, GError **error); | 70 | gfloat max_percentage, |
149 | 68 | int backlight_step_down (GsdRRScreen *rr_screen, GError **error); | 71 | GError **error); |
150 | 72 | int backlight_step_up (GsdRRScreen *rr_screen, | ||
151 | 73 | gfloat min_percentage, | ||
152 | 74 | gfloat max_percentage, | ||
153 | 75 | GError **error); | ||
154 | 76 | int backlight_step_down (GsdRRScreen *rr_screen, | ||
155 | 77 | gfloat min_percentage, | ||
156 | 78 | gfloat max_percentage, | ||
157 | 79 | GError **error); | ||
158 | 69 | int backlight_set_abs (GsdRRScreen *rr_screen, | 80 | int backlight_set_abs (GsdRRScreen *rr_screen, |
159 | 70 | guint value, | 81 | guint value, |
160 | 71 | GError **error); | 82 | GError **error); |
161 | 72 | 83 | ||
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 | 189 | /* Brightness */ | 189 | /* Brightness */ |
167 | 190 | gboolean backlight_available; | 190 | gboolean backlight_available; |
168 | 191 | gint pre_dim_brightness; /* level, not percentage */ | 191 | gint pre_dim_brightness; /* level, not percentage */ |
169 | 192 | gfloat min_brightness_percentage; | ||
170 | 193 | gfloat max_brightness_percentage; | ||
171 | 192 | 194 | ||
172 | 193 | /* Keyboard */ | 195 | /* Keyboard */ |
173 | 194 | GDBusProxy *upower_kdb_proxy; | 196 | GDBusProxy *upower_kdb_proxy; |
174 | @@ -2626,6 +2628,15 @@ | |||
175 | 2626 | } | 2628 | } |
176 | 2627 | 2629 | ||
177 | 2628 | static void | 2630 | static void |
178 | 2631 | brightness_configure (GsdPowerManager *manager) | ||
179 | 2632 | { | ||
180 | 2633 | gint min = CLAMP (g_settings_get_int (manager->priv->settings, "min-brightness-percentage"), 0, 100); | ||
181 | 2634 | gint max = CLAMP (g_settings_get_int (manager->priv->settings, "max-brightness-percentage"), min, 100); | ||
182 | 2635 | manager->priv->min_brightness_percentage = min * 0.01; | ||
183 | 2636 | manager->priv->max_brightness_percentage = max * 0.01; | ||
184 | 2637 | } | ||
185 | 2638 | |||
186 | 2639 | static void | ||
187 | 2629 | main_battery_or_ups_low_changed (GsdPowerManager *manager, | 2640 | main_battery_or_ups_low_changed (GsdPowerManager *manager, |
188 | 2630 | gboolean is_low) | 2641 | gboolean is_low) |
189 | 2631 | { | 2642 | { |
190 | @@ -3013,6 +3024,11 @@ | |||
191 | 3013 | setup_lid_closed_action (manager); | 3024 | setup_lid_closed_action (manager); |
192 | 3014 | return; | 3025 | return; |
193 | 3015 | } | 3026 | } |
194 | 3027 | if (g_str_equal (key, "min-brightness-percentage") || | ||
195 | 3028 | g_str_equal (key, "max-brightness-percentage")) { | ||
196 | 3029 | brightness_configure (manager); | ||
197 | 3030 | return; | ||
198 | 3031 | } | ||
199 | 3016 | } | 3032 | } |
200 | 3017 | 3033 | ||
201 | 3018 | static void | 3034 | static void |
202 | @@ -3416,6 +3432,7 @@ | |||
203 | 3416 | /* coldplug the engine */ | 3432 | /* coldplug the engine */ |
204 | 3417 | engine_coldplug (manager); | 3433 | engine_coldplug (manager); |
205 | 3418 | idle_configure (manager); | 3434 | idle_configure (manager); |
206 | 3435 | brightness_configure (manager); | ||
207 | 3419 | 3436 | ||
208 | 3420 | manager->priv->xscreensaver_watchdog_timer_id = gsd_power_enable_screensaver_watchdog (); | 3437 | manager->priv->xscreensaver_watchdog_timer_id = gsd_power_enable_screensaver_watchdog (); |
209 | 3421 | 3438 | ||
210 | @@ -3590,12 +3607,21 @@ | |||
211 | 3590 | 3607 | ||
212 | 3591 | if (g_strcmp0 (method_name, "GetPercentage") == 0) { | 3608 | if (g_strcmp0 (method_name, "GetPercentage") == 0) { |
213 | 3592 | g_debug ("screen get percentage"); | 3609 | g_debug ("screen get percentage"); |
215 | 3593 | value = backlight_get_percentage (manager->priv->rr_screen, &error); | 3610 | value = backlight_get_percentage ( |
216 | 3611 | manager->priv->rr_screen, | ||
217 | 3612 | manager->priv->min_brightness_percentage, | ||
218 | 3613 | manager->priv->max_brightness_percentage, | ||
219 | 3614 | &error); | ||
220 | 3594 | 3615 | ||
221 | 3595 | } else if (g_strcmp0 (method_name, "SetPercentage") == 0) { | 3616 | } else if (g_strcmp0 (method_name, "SetPercentage") == 0) { |
222 | 3596 | g_debug ("screen set percentage"); | 3617 | g_debug ("screen set percentage"); |
223 | 3597 | g_variant_get (parameters, "(u)", &value_tmp); | 3618 | g_variant_get (parameters, "(u)", &value_tmp); |
225 | 3598 | ret = backlight_set_percentage (manager->priv->rr_screen, value_tmp, &error); | 3619 | ret = backlight_set_percentage ( |
226 | 3620 | manager->priv->rr_screen, | ||
227 | 3621 | value_tmp, | ||
228 | 3622 | manager->priv->min_brightness_percentage, | ||
229 | 3623 | manager->priv->max_brightness_percentage, | ||
230 | 3624 | &error); | ||
231 | 3599 | if (ret) { | 3625 | if (ret) { |
232 | 3600 | value = value_tmp; | 3626 | value = value_tmp; |
233 | 3601 | backlight_emit_changed (manager); | 3627 | backlight_emit_changed (manager); |
234 | @@ -3603,12 +3629,20 @@ | |||
235 | 3603 | 3629 | ||
236 | 3604 | } else if (g_strcmp0 (method_name, "StepUp") == 0) { | 3630 | } else if (g_strcmp0 (method_name, "StepUp") == 0) { |
237 | 3605 | g_debug ("screen step up"); | 3631 | g_debug ("screen step up"); |
239 | 3606 | value = backlight_step_up (manager->priv->rr_screen, &error); | 3632 | value = backlight_step_up ( |
240 | 3633 | manager->priv->rr_screen, | ||
241 | 3634 | manager->priv->min_brightness_percentage, | ||
242 | 3635 | manager->priv->max_brightness_percentage, | ||
243 | 3636 | &error); | ||
244 | 3607 | if (value != -1) | 3637 | if (value != -1) |
245 | 3608 | backlight_emit_changed (manager); | 3638 | backlight_emit_changed (manager); |
246 | 3609 | } else if (g_strcmp0 (method_name, "StepDown") == 0) { | 3639 | } else if (g_strcmp0 (method_name, "StepDown") == 0) { |
247 | 3610 | g_debug ("screen step down"); | 3640 | g_debug ("screen step down"); |
249 | 3611 | value = backlight_step_down (manager->priv->rr_screen, &error); | 3641 | value = backlight_step_down ( |
250 | 3642 | manager->priv->rr_screen, | ||
251 | 3643 | manager->priv->min_brightness_percentage, | ||
252 | 3644 | manager->priv->max_brightness_percentage, | ||
253 | 3645 | &error); | ||
254 | 3612 | if (value != -1) | 3646 | if (value != -1) |
255 | 3613 | backlight_emit_changed (manager); | 3647 | backlight_emit_changed (manager); |
256 | 3614 | } else { | 3648 | } else { |
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.