Adds support for keyboard backlight controls in powerdevil

Reviews
Diff

- Download Diff

Information
Submitter:	Michael Zanetti
Repository:	kde-workspace
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:
People:	afiestas, dafre

Description

This adds Keyboard backlight control support to powerdevil.

Testing Done

Issues

0
13
3
All Issues: 16

Description	From	Last Updated
use "@label:slider" here.	Lamarque Souza	Nov. 16, 2012, 6:08 p.m.
the parameter can be const here.	Lamarque Souza	Nov. 16, 2012, 6:32 p.m.
The translation team does not like things like this. You should use setText(i18nc("@info:textbox", "%1 %%", brightnessLevel)). More info about i18nc ...	Lamarque Souza	Nov. 16, 2012, 6:10 p.m.
Usually I would use Q_UNSED(rectF) here to prevent compiler warnings. Does just commenting the parameter name also does that trick?	Lamarque Souza	Nov. 16, 2012, 6:11 p.m.
I think you can use one of the font size from KDE configuration here.	Lamarque Souza	Nov. 15, 2012, 5:07 p.m.
prepend "Global shortcut" with "@action:inmenu". Do the same for the others below this line.	Lamarque Souza	Nov. 16, 2012, 6:27 p.m.
type can be const, brightness too but since your patch does not add it I do not know if you ...	Lamarque Souza	Nov. 15, 2012, 5:09 p.m.
Add space: if (…)	Kai Uwe Broulik	Nov. 16, 2012, 6:28 p.m.
Add space: } else if (…) {	Kai Uwe Broulik	Nov. 16, 2012, 6:29 p.m.
Add space: if (…)	Kai Uwe Broulik	Nov. 16, 2012, 6:29 p.m.
Add space: } else if (…) {	Kai Uwe Broulik	Nov. 16, 2012, 6:29 p.m.
I think duplicating all this code is not good, we already use a similar thing for KMix and having them ...	Kai Uwe Broulik	Nov. 16, 2012, 6:05 p.m.
I think we should use Keyboard instead of abbreviated Kbd? I often misspell such things (Valid for all other cases, ...	Kai Uwe Broulik	Nov. 16, 2012, 10:11 p.m.
Why that commented rectF?	Kai Uwe Broulik	Nov. 16, 2012, 6:07 p.m.
Patch doesn't apply: It misses the void onBatteryChargeStateChanged I added to allow a "Charge Complete" notification to be emitted. Please ...	Kai Uwe Broulik	Nov. 16, 2012, 10:11 p.m.
Maybe change that to "Keyboard Backlight", the term looks weird and too long in the KCM next to the "Display ...	Kai Uwe Broulik	Nov. 17, 2012, 9:07 p.m.

powerdevil/daemon/actions/bundled/kbdbrightnesscontrolconfig.cpp (Diff revision 1)

Show all issues

use "@label:slider" here.

powerdevil/daemon/kbdbrightnessosdwidget.h (Diff revision 1)

Show all issues

the parameter can be const here.

powerdevil/daemon/kbdbrightnessosdwidget.cpp (Diff revision 1)

Show all issues

The translation team does not like things like this. You should use setText(i18nc("@info:textbox", "%1 %%", brightnessLevel)). More info about i18nc in https://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics

powerdevil/daemon/kbdbrightnessosdwidget.cpp (Diff revision 1)

Show all issues

Usually I would use Q_UNSED(rectF) here to prevent compiler warnings. Does just commenting the parameter name also does that trick?

Michael Zanetti Nov. 15, 2012, 8:18 a.m.

Yes, this is what you do if you dont have Q_UNUSED (or any other macro that adds a (void*) cast). Disregarding compiler-optimisation, this would actually be the intended C++ way to do it.

However, I copied this from the other brighness OSD. As Kai Uwe pointed out I'm going to merge them into one anyways. Can change it into Q_UNUSED there if you want.

Lamarque Souza Nov. 15, 2012, 1:09 p.m.

I am not sure. I would like to hear from someone who knows better how to deal with this situation. I have always used Q_UNUSED, but if there is a better alternative, why not use it?

Michael Zanetti Nov. 15, 2012, 5:09 p.m.

Well, its not better. In the end the compiler optimizes it to be the same. Using Q_UNUSED() is the Qtish way. Removing the parameter name the plain c/c++ way. As we do everything Qt, I will probably change it t use Q_UNUSED. However, it is really just a matter of taste. And again, I just copied it from existing codebase without even noticing it.

powerdevil/daemon/kbdbrightnessosdwidget.cpp (Diff revision 1)

Show all issues

I think you can use one of the font size from KDE configuration here.

Kai Uwe Broulik Nov. 14, 2012, 10:37 p.m.

This is actually done that way in all OSD widgets (PowerDevil brightness OSD, as well as KMix OSD) - and is there a preconfigured size in that range at all? It adapts to the global dpi settings, so I don't see a reason to change that.

Michael Zanetti Nov. 15, 2012, 8:18 a.m.
```
I agree with Kai Uwe here.
```
Lamarque Souza Nov. 15, 2012, 1:09 p.m.
```
Ok then.
```

powerdevil/daemon/powerdevilcore.cpp (Diff revision 1)

Show all issues

prepend "Global shortcut" with "@action:inmenu". Do the same for the others below this line.

powerdevil/daemon/powerdevilcore.cpp (Diff revision 1)

Show all issues

type can be const, brightness too but since your patch does not add it I do not know if you should change it.

Michael Zanetti Nov. 15, 2012, 8:18 a.m.

> but since your patch does not add it I do not know if you should change it.
Yes, and also I don't really see the point. It's a copy, not a reference anyways. If the developer wants to change it inside the functions, why not.

Lamarque Souza Nov. 15, 2012, 1:09 p.m.

const is never required as far as I know (I wrote "can be" because of that), it is more like a hint to the compiler. const allows the compiler to do some optimizations that otherwise the compiler would not do. I am not sure if adding const here would change the generated binary code, so fell free to do this change or not.

Michael Zanetti Nov. 15, 2012, 5:09 p.m.

As it doesn't make a difference in this case (it's just an integer anyways) I will drop this because it makes signal/slot connections a pain (the other matching signals don't have the const).

I will of course fix all the other raised issues.

powerdevil/daemon/backends/hal/powerdevilhalbackend.cpp (Diff revision 1)

Show all issues

Add space: if (…)

powerdevil/daemon/backends/hal/powerdevilhalbackend.cpp (Diff revision 1)

Show all issues

Add space: } else if (…) {

powerdevil/daemon/backends/upower/powerdevilupowerbackend.cpp (Diff revision 1)

Show all issues

Add space: if (…)

powerdevil/daemon/backends/upower/powerdevilupowerbackend.cpp (Diff revision 1)

Show all issues

Add space: } else if (…) {

powerdevil/daemon/kbdbrightnessosdwidget.cpp (Diff revision 1)

Show all issues

I think duplicating all this code is not good, we already use a similar thing for KMix and having them all synchronized for changes is really a pain. Can't we somehow merge both OSDs and use a single one, at least for powerdevil?

Michael Zanetti Nov. 15, 2012, 8:18 a.m.

Full ack. Even though I thought of solving the problem for real with some sort of OSD framework, I can merge the 2 OSDs in Powerdevil for now.

powerdevil/daemon/kbdbrightnessosdwidget.cpp (Diff revision 1)

Show all issues

I think we should use Keyboard instead of abbreviated Kbd? I often misspell such things (Valid for all other cases, of course)

Michael Zanetti Nov. 15, 2012, 8:18 a.m.

Don't really have a strong opinion on that one. I used Kbd because that is a well known abbreviation Keyboard that shows up in a lot of places.

powerdevil/daemon/kbdbrightnessosdwidget.cpp (Diff revision 1)

Show all issues

Why that commented rectF?

Michael Zanetti Nov. 15, 2012, 8:18 a.m.

Old-school style Q_UNUSED(rectF) that is. Also copied from the other OSDs. Usually I prefer Q_UNUSED() in Qt applications too.

powerdevil/daemon/powerdevilcore.h (Diff revision 1)

Show all issues

Patch doesn't apply:
It misses the
void onBatteryChargeStateChanged
I added to allow a "Charge Complete" notification to be emitted. Please re-base your patch.

Michael Zanetti Nov. 15, 2012, 8:18 a.m.

As the review header says, it applies to KDE/4.9. I thought that would make it easier for people to test because I expect not all of us run git master all the time and I'm not sure about running a git master kded inside a 4.9 environment. If you guys prefer reviewing against master, I can of course rebase it.

Kai Uwe Broulik Nov. 15, 2012, 1:33 p.m.

Problem is: This is a new feature, and more importantly introduces new i18n strings, so this may only go to master, and also in master we are already in Hard Feature Freeze and String Freeze, so this will have to wait for 4.11 anyway, I guess.

Michael Zanetti Nov. 15, 2012, 5:12 p.m.

Of course I wouldn't push it to 4.9 in the end. Still hoped to get it into 4.10 though :/ But you're right... the deadline is gone...

Change Summary:

reworked patch according to review comments

Diff:

Revision 2 (+524 -63)

Show changes

	powerdevil/daemon/CMakeLists.txt
	powerdevil/daemon/brightnessosdwidget.h
	powerdevil/daemon/brightnessosdwidget.cpp
	powerdevil/daemon/org.kde.Solid.PowerManagement.xml
	powerdevil/daemon/powerdevilactionpool.cpp
	powerdevil/daemon/powerdevilbackendinterface.h
	powerdevil/daemon/powerdevilcore.h
	powerdevil/daemon/powerdevilcore.cpp
	12 more

Change Summary:

patch applies to master now

Branch:

-	KDE/4.9
+	master

I just installed the patch and I get the config option in the KCM but nothing happened. Neither pressing the associated keys, nor changing the slider in the KCM did change the brightness.
Also you should check for the maximum brightness available. My notebook only offers four steps (0 = off, 1, 2, 3), so having a 100% slider is counterproductive.

Michael Zanetti Nov. 17, 2012, 3:03 p.m.

Would you mind helping debug this? It works fine here and I think the problem is rather in the backends which are not part of my patch but already included in powerdevil. Reading through that code it does already check for max brightness and converts the 100% slider. Would be interesting to know what responses you get from D-Bus calls like:

qdbus --system org.freedesktop.UPower /org/freedesktop/UPower/KbdBacklight org.freedesktop.UPower.KbdBacklight.GetBrightness
qdbus --system org.freedesktop.UPower /org/freedesktop/UPower/KbdBacklight org.freedesktop.UPower.KbdBacklight.GetMaxBrightness

also please check if it you can set the backlight using UPower directly:

qdbus --system org.freedesktop.UPower /org/freedesktop/UPower/KbdBacklight org.freedesktop.UPower.KbdBacklight.SetBrightness 3

Please let me know which values work and which don't. kded's debug output should show you with which values it tries to set the brightness when you press the buttons on your keyboard.


Also, my keyboard offers 256 steps so it makes quite sense to have a 100% slider. I think the best splution to this would be to keep the 100% slider but set it to the closest available value for your hardware in the backend.

Kai Uwe Broulik Nov. 17, 2012, 4:17 p.m.

All of that works. GetBrightness gives me 1 (I usually have it at lowest setting), GetMaxBrightness gives me 3 which is correct, and using SetBrightness I can set the brightness.
Still, I think if the steps are below 100, we should only offer those to the user. When I set it to 40% on Battery and 60% on AC power, nothing will change I guess because it is higher than 33 but lower than 66 or so. In your case it is perfectly valid to have a 100% slider but when there are just a couple of values, the slider should reflect that.

Kai Uwe Broulik Nov. 17, 2012, 4:21 p.m.

Okay, now it works, I guess this is again a Project Neon issue where the DBus services are not properly registered/accessible. When I launch it from my current session everything works. And there again I notice that the 100% slider is bad :P I had to try a dozen times to find a proper setting, the left third of the slider is just backlight off.
What doesn't work so far:
 - Key to *increase* brightness, I can happily decrease the background brightness
 - The % sign in the OSD is shown twice ie. "0% %"

powerdevil/daemon/actions/bundled/powerdevilkeyboardbrightnesscontrolaction.desktop (Diff revision 2)

Show all issues

Maybe change that to "Keyboard Backlight", the term looks weird and too long in the KCM next to the "Display Brightness"

Change Summary:

Ok. I have found the reason why it didn't work for you:

As your hardware has only 4 steps, increasing by 10% isn't enough to go up to the next level because the number is truncated. I now round the result instead of truncate and increase/decrease by 30% in case there are less than 6 possible values. 30% works fine to increase/decrease exactly one step for keyboards with 2 (on,off), 3, 4, and 5 steps. For everything else the 10% are fine.

That said, this still doesn't change the slider in the kcm module, however, it distributes the values more naturally on it. Making the slider aware of the possible steps would require introducing some API functions in the daemon which don't really fit with the rest of the intended hardware abstraction.

Diff:

Revision 3 (+536 -66)

Show changes

	powerdevil/daemon/CMakeLists.txt
	powerdevil/daemon/brightnessosdwidget.h
	powerdevil/daemon/brightnessosdwidget.cpp
	powerdevil/daemon/org.kde.Solid.PowerManagement.xml
	powerdevil/daemon/powerdevilactionpool.cpp
	powerdevil/daemon/powerdevilbackendinterface.h
	powerdevil/daemon/powerdevilcore.h
	powerdevil/daemon/powerdevilcore.cpp
	12 more

Okay that makes sense :-)

Why would that need a change in the API? If the slider is aware of the possible range of values, then you can directly take the values the slider is set to and store them and use them for the backlight. (Btw I didn't say I like the way the display backlight slider works :P) Or does the API require a range of 0-100? That would be bad since the UPower dbus calls also use absolute numbers.

Michael Zanetti Nov. 17, 2012, 10:10 p.m.

Yes. The api is based on % values. The slider (or better: the whole kcm module) has no clue about anything happening in the daemon. The controls for brightness only store 1 value of the brighness percentage into a config file and send a signal to the daemon to reload the config. No communication watsoever the other direction.

Ship it!

Sorry for joining the party late. The patch is amazingly slick, so thanks for such a useful feature. Not sure if we'll make it for 4.10, for now I'd throw this to master, and right after bring attention to this on the release list. Let's coordinate on IRC for that. Thanks again, amazing job.

Lukáš Tinkl Jan. 16, 2013, 5 p.m.
```
Has this been submitted?
```

Michael Zanetti Jan. 17, 2013, 9:54 a.m.

No, not yet. As it was too late for the 4.10 feature freeze. It'll be submitted when trunk is open for 4.11 features.

Michael Zanetti Jan. 17, 2013, 9:54 a.m.

... which I just realized already happened. I'll submit today evening. Thanks for the reminder.

This review has been submitted with commit d9f08307177fd56d98b58dfe87cbea062a1516e0 by Michael Zanetti to branch master.

Jekyll Wu Jan. 23, 2013, 12:23 p.m.

Hi Michael, I think this commit introduces https://bugs.kde.org/show_bug.cgi?id=313708. Could you please take a look at it ?

You have a pending review.

Review Board 2.5.15

Screenshots

Files

Issues

Change Summary:

Diff:

Change Summary:

Branch:

Change Summary:

Diff:

Status: Closed (submitted)