Review Request 109792: Update 'dim display' algorithm.
sebas at kde.org
Thu Apr 11 18:09:57 BST 2013
On Thursday, April 11, 2013 17:59:03 Danny Baumann wrote:
> >> Having said that, my current plan is the following:
> >> - Drop the UI option
> >> - However, leave the configurability via kconfig intact. That way, users
> >> who are not happy with the defaults do at least have a remote chance of
> >> changing it without recompiling KDE (after they found it's configurable,
> >> of course - but there's a huge step between looking something up and
> >> compiling a whole DE to change it) - Additionally add a dimRelative
> >> kconfig setting. With dimRelative=true, dimRatio is interpreted as a
> >> factor
> >> (dimmedBrightness = origBrightness * dimRatio), with it being false it's
> >> interpreted as an absolute value (dimmedBrightness = 100f * dimRatio). -
> >> The defaults are set up as dimRatio = 0.05, dimRelative = false
> >> - Do the dimming in 2 steps, the first one being done after the
> >> configured
> >> time, the second one 10 seconds later
> >> Is that approach ok?
> > Nope, even a config key just adds maintainability and support burden. I've
> > been there, done that, and didn't like the ride.
> > The bug you are addressing is entirely fixable without an option, there is
> > simply not a good reason to make this configurable.
> So you say whoever is not happy with the proposed defaults (e.g. me) is
> suppposed to either recompile KDE or use another DE?
You can already change the defaults with the slider offered to set the
brightness. What you are proposing is an option for an option, that's one
layer of complexity too much, unnecessarily.
You can even define this setting differently for each activity.
> My patch is _not_
> addressing a bug only (although that ultimately was the trigger), but
> aims at improving the algorithm in the process.
Adding complexity for no good reason (but with good reasons against it) is not
improving the code, it's making it harder to understand, explain and maintain,
harder to support and easier to break and perform worse (though probably not
> (In case you're wondering, I'd like to make it dim to 30% of the
> original brightness to make it work seamlessly in the evening where the
> non-dimmed brightness is lower. I understand this may not be deemed a
> suitable default behaviour.)
I haven't thought about this use case. The idea of a dimming relative to
initial brightness is not bad. It would solve the case where you've set the
brightness lower (to, say 50%), the display dims in steps and as first step
cranks the brightness up, not down. I'm not sure this is still a bug, haven't
noticed it in a while, though.
A problem I can imagine with getting a relative setting to do what you expect
it to is that eyes and perception of brightness (relative to ambient light,
relative to what the user is used to) is not linear, but far from it. You
could certainly try making the dimming relative, that's entirely safe unless a
lower limit is hit. I haven't tried this, though, and as I said it's probably
very complex to get right -- maybe not. When I encounter the problem you're
trying to solve, it's usually in a dark environment where I switch the laptop
on, and the display is too bright. The dim-on-idle doesn't matter at all in my
case since I'm then actually using the machine and it won't go idle. When I'm
watching a movie, I usually switch to my media consumption activity, which has
it all set up (i.e. no dimming at all, switch off after a long time, etc.) and
the media in front of me.
It's good to try this, but I think this review request is growing quite out of
proportion, and it already addresses different things (making it harder to
pass review). How about we try to get the minimal solution in, and then back
to the drawing board to come up with a solution that satisfies your usecase
and is acceptable for everybody? That might sound more tedious, but in the end
the process is quicker and more satisfying.
http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
More information about the kde-core-devel