[Kde-hardware-devel] Screen Brightness

[Albert Astals Cid]

A Diumenge 20 Abril 2008, Thomas Gillespie va escriure:
> Hi again,
>
> Finished the code, the diff is attached to this email. Thought I better
> let the list check if before i commit, I've never committed to the core
> before. I also added a section in solidshell to test.
> Comments/improvements on the api are more than welcome.
>
> Thanks
>
> Tom

Random comments over code quality, not functionality

device == QString() -> bad, you should use isNull() or isEmpty() depending of 
what you want

foreach(QString device, control.keys() -> bad, you should use foreach(const 
QString &device to avoid unnecessary QString constructions. The same applies 
to your other foreach constructions

int getBrightness(QString device = QString())
I don't know much about Solid naming schemes, but in KDE/Qt in general with 
omit get for getters, so it should be 
int brightness(QString device = QString())
BAD, it should be 
int brightness(const QString &device = QString())
to avoid unnecessary QString construction calls

The same applies for the setter
bool setBrightness(int brightness, QString device = QString())
should be 
bool setBrightness(int brightness, const QString &device = QString())

There's qRound, you don't need your own round function.

Albert