Code review comment for lp:~nwkj86/compiz/add-background-conf-to-switcher

Hey Artur,
Thanks for your contribution.

Here a review:

1. You need to click "Set commit message" on this page above and enter a commit message there, otherwise this won't merge correctly.

2. A good common place to put setBackground () to would be the BaseSwitchScreen class in compiztoolbox.cpp to avoid code duplication.
You could then use the optionGet () variables as function arguments in your setBackground () function and pass those via update notification to your function like this:

    optionSetUseBackgroundColorNotify (boost::bind (&BaseSwitchScreen::updateBackground, this, optionGetUseBackgroundColor (), optionGetBackgroundColor ()));
    optionSetBackgroundColorNotify (boost::bind (&BaseSwitchScreen::updateBackground, this, optionGetUseBackgroundColor (), optionGetBackgroundColor ()));

The function in BaseSwitchScreen could then look like this:
void
BaseSwitchScreen::updateBackground (bool useBackgoundColor,
        unsigned short backgroundColor [])

3. unsigned short alpha seems to be unused. I know XColor does not care about transparency, but it would be nice if the alpha value would not be ignored, so semi-transparency of the background would also be possible...

4. Please insert a whitespace between function name and brackets as this is the style Compiz (mostly ;)) uses.

5. Typo here: 87 + <_short>Bacground Color</_short>

6. It seems that once the background color has been enabled, it cannot be disabled anymore - setting unsigned long background_pixel = 0ul; seems not to be enough to reset the background back...
Also you should not set background_pixel to 0ul in any case as this value gets overwritten before usage anyway in the case the background color gets used...
Does disabling the background color work for you ?