Review Request 109792: Update 'dim display' algorithm.

[Sebastian Kügler]

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 
noticeable).

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

Cheers,
-- 
sebas

http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9