<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/109792/">http://git.reviewboard.kde.org/r/109792/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 31st, 2013, 4:14 p.m. UTC, <b>Àlex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">To be honest I don't like adding yet another configuration option by default, a configuration option that apparently is needed only in some systems.
There is no other alternative than this? </pre>
</blockquote>
<p>On March 31st, 2013, 4:48 p.m. UTC, <b>Danny Baumann</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well, I don't see any. I tried to approach this at the kernel level first, but the kernel people rejected the idea. But I don't see it as a big problem either: E.g. Windows (which isn't an example of overconfigurability) has a dimmed brightness setting as well. While it's required for my laptop, I think it can be useful for other people as well. When I hit the problem, at first I couldn't believe this isn't configurable either ;)
BTW, one thing I'm not yet certain about is whether the option should be 'dim to' or 'dim by'. Currently it's the former, but the latter might it make more obvious that the value is relative to the non-dimmed brightness.</pre>
</blockquote>
<p>On April 2nd, 2013, 9:16 a.m. UTC, <b>Sebastian Kügler</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Just set the minimum brightness to 1 instead of 0. We're generally not adding config options for this kind of things.</pre>
</blockquote>
<p>On April 2nd, 2013, 9:44 a.m. UTC, <b>Danny Baumann</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">That doesn't help though, as with intel-backlight 1 is still way too low to be visible. I think the value corresponding to acpi value 0 is about 5%. At this point, I think, I would alter the acpi behaviour again. Also, if the user had set the brightness to 0 before for some reason (e.g. because he's working in the evening), 'dim display' would suddenly make the display more bright.
In general, the problem is the different backlight interfaces just behave differently, which is why I think this problem can't be solved with a hardcoded value as long as it's not possible to detect the used sysfs file (which isn't the case as long as the used X driver provides a backlight interface via Xrandr).
</pre>
</blockquote>
<p>On April 2nd, 2013, 10:01 a.m. UTC, <b>Sebastian Kügler</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The "dim can brighten up" is a different problem that needs a different solution, it's unrelated here.
I don't see how a config option solves this problem. Even with a config option, we still don't know what the default should be. Also, users are not going to look for such an option (which maybe says enough about the option itself). Users are going to hit the brightness slider when the brightness is too low, so 1 actually does make sense.</pre>
</blockquote>
<p>On April 2nd, 2013, 11:07 a.m. UTC, <b>Danny Baumann</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">A sane value for the default would be 'retain current behaviour' (that is: dim by 100%).
Is there evidence for the 'users are not going to look for such an option' assertion? I know for a fact that I _did_ search for that option (until I looked up in the source that it's nowhere to be found). Surely this one sample doesn't represent the KDE user base - but do you have any representative data, especially given that the KDE user base in general doesn't have a problem with a slightly-higher-than-usual amount of settings (which is also evident in the very dialog we're talking about: are there that many users who look for e.g. a highly configurable 'run script on power status change' option?)
I'm also not sure what you mean by 'Users are going to hit the brightness slider when the brightness is too low', as we're talking about the dimmed-down case after all, where the user was idle for the configured amount of time. But as I wrote above, neither 0 nor 1 works for any user who is not using the acpi_video backlight controls for whatever reason (e.g. because it's buggy, or simply not present at all). I'm open for suggestions on how to fix this problem without config option (even if I think the option makes sense), but just raising the hardcoded value from 0 to 1 definitely does not help.</pre>
</blockquote>
<p>On April 2nd, 2013, 11:31 a.m. UTC, <b>Sebastian Kügler</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What would probably make sense it to use 10% or 1 (whichever is more) as lowest value. We do know the max value, so we can make sure it's low, but not zero. This should be tried.
How does raising it from 0 to 1 not help, btw?</pre>
</blockquote>
<p>On April 2nd, 2013, 11:42 a.m. UTC, <b>Danny Baumann</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Raising to 10% would help in my case, but how do you suggest handling cases where either
- 10% is considered 'too bright' (given that it was darker before - people might complain about 'more power usage than necessary') and
- 10% is brighter than the brightness before dimming?
The answer to 'why does raising to 1 not help' is a few comments above ;)
"That doesn't help though, as with intel-backlight 1 is still way too low to be visible. I think the value corresponding to acpi value 0 is about 5%"
But can you please explain where exactly you see the problem of this option compared to e.g. the way more sophisticated 'run script' options?</pre>
</blockquote>
<p>On April 2nd, 2013, 12:06 p.m. UTC, <b>Sebastian Kügler</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The option does not solve the problem. We have to make sure the system is usable without changing an option. qMax(5%, 1) is also fine with me.
In general, we can't expect users to look into options to fix obvious breakage. The fact that *you* would look into it doesn't say anything valuable, since we're not developing a system for people familiar with power management internals, but for people who want to get their job done. UI options come at certain costs, so we're only adding those where they really add value. For cases of breakage, we have to find solutions that are transparant to the user.</pre>
</blockquote>
<p>On April 2nd, 2013, 12:34 p.m. UTC, <b>Danny Baumann</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well, the issue is that 5% solve the problem for me, but other people might need other values (glossy panels, less bright backlight, etc.). The 5% figure (BTW, all numbers dimdisplay.cpp deals with are in the 0..100 range) is an _empiric_ value for _my_ system. I don't have any kind of data for other systems, so I simply don't know at all whether there's any value that would work for all users.
To throw in some random data point I just found: Gnome dims to 30% of the maximum value (as opposed to 30% of the currently set value, what my patch is currently doing). That value is not mapped into their GUI, but it's not hardcoded either: It's configurable via dconf-editor. Maybe an approach like this (make it configurable, but don't map into the UI) would work?</pre>
</blockquote>
<p>On April 2nd, 2013, 1:51 p.m. UTC, <b>Àlex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What would work.
All we can do here (as we do with brightness) is having sensible defaults that work best with for all hardware. Then we have to make it configurable (we don't have it in brightness yet) so advanced users and in the future retailers can make use of it.
You should wait until Dario or Oliver review this (added them in review)</pre>
</blockquote>
<p>On April 2nd, 2013, 3:15 p.m. UTC, <b>Oliver Henshaw</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't have particularly strong opinions about the existence of an extra configuration option, so I'll leave that matter to those who do, although I think you would need very good justification to put it in the configuration UI[1].
I understand from the intel-gfx links, and others in that thread, that you can't rely on any guarantees of the visibility of brightness 0 in intel_backlight or even the acpi backend. But have you explored whether it's possible to smooth over the difference between backends (or hash out some agreed semantics) at xrandr level?
[1] If this is added to the UI, perhaps it could be presented on another (or the) brightness slider? We'd need someone who's actually an UI expert to agree that's a good idea, of course.</pre>
</blockquote>
<p>On April 2nd, 2013, 3:30 p.m. UTC, <b>Danny Baumann</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I tried to avoid making this a slider because of the relationship to the current brightness: Expressing the setting as a slider IMHO implies that the value to be set is an absolute value, which imposes corner cases (e.g. what to do if current brightness is higher than dimmed brightness?). Using a relative value avoids those and makes sure the display is always dimmed as long as the hardware allows it. I couldn't think of a good way of expressing the relative value with a slider, which is why I chose the spin box. I'm open for suggestions, though :)
As for the Xrandr question: No, I didn't (yet? - this already became quite a work-intensive thing for me given that it's only a DSDT problem I'm dealing with ;) ), because I'm not sure how it'll help: As KDE (as well as Gnome, as well as probably the other DEs) ships a fallback for the non-Xrandr case anyway, doesn't it make more sense to define the behaviour in the common piece of code? Otherwise, one would get behavioral differences depending on what backend happens to be in use; I'm not sure whether this is a desired effect?</pre>
</blockquote>
<p>On April 2nd, 2013, 9:11 p.m. UTC, <b>Sebastian Kügler</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">There's no need for a slider, as there won't be any UI option for this. That's my personal opinion, but you can ask anybody in the Plasma team, and the answer will be the exact same. Making it configurable doesn't solve the problem, it reduces clarity in the UI, and it's the kind of option we've been successfully removing from our UI, with good reasons, for years. It just would be a huge step back.
With that out of the way, I still don't see what's wrong with setting it to 5% or 1, whichever is lower. The user, after all, configured the system to go to minimum brightness after a grace period, as long as the display is lit, we're doing the expected thing.
I do agree that dimming is not switching off, so we should make sure that the value never reaches 0, but other than "the display switched off instead of dimming", and of course the "dims after half the grace time", there is no bug.
>From my experience, values for brightness are often either between 0 and 10 or between 0 and 100.
One thing that should not get lost in the discussion about implementation details: I really appreciate your work on this. :)</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Sebastian, honestly, why are you ignoring what I wrote multiple times? :(
"That doesn't help though, as with intel-backlight 1 is still way too low to be visible. I think the value corresponding to acpi value 0 is about 5%"
In other words: While with intel-backlight 1% may not be 'backlight off' technically speaking, the visual appearance of 'display off' and 'display at 1%' is almost exactly the same.</pre>
<br />
<p>- Danny</p>
<br />
<p>On April 2nd, 2013, 1:52 p.m. UTC, Danny Baumann wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kde-workspace, Solid, Dario Freddi, and Oliver Henshaw.</div>
<div>By Danny Baumann.</div>
<p style="color: grey;"><i>Updated April 2, 2013, 1:52 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This change does two things:
- No longer dim display before the dim time set by the user elapses.
This fixes bug #304696
- No longer assume that 0% display brightness produces a visible result.
This doesn't work with the intel-backlight backlight interface (as it
turns off the backlight at 0% brightness). According to the kernel
developers (see [1] and [2]), this isn't a safe assumption to make for
other backlight interfaces either. Instead of always dimming to 0%,
make the amount of dimming configurable.
[1]
http://lists.freedesktop.org/archives/intel-gfx/2013-March/026152.html
[2]
http://lists.freedesktop.org/archives/intel-gfx/2013-March/026140.html</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=304696">304696</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>powerdevil/daemon/actions/bundled/dimdisplay.h <span style="color: grey">(426640ca7a2a2d23044052710fdfb4b1f65617d1)</span></li>
<li>powerdevil/daemon/actions/bundled/dimdisplay.cpp <span style="color: grey">(11554e3ba5d2f67d4d1de9d61c744c6c40a32d27)</span></li>
<li>powerdevil/daemon/actions/bundled/dimdisplayconfig.h <span style="color: grey">(14b787937249a512cf958a31fd3bd71d6051540d)</span></li>
<li>powerdevil/daemon/actions/bundled/dimdisplayconfig.cpp <span style="color: grey">(bc116d6216c4624cbf14489d739d9d226fb70ff3)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109792/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>