Merge lp:~nwkj86/compiz/add-background-conf-to-switcher into lp:compiz/0.9.12

[Artur]

Allow to select background color for switcher and staticswitcher.

Allow user to set & select background color for switcher and staticswitcher.

Right now setBackground() function is duplicated. I'm looking for the good common place to put it there.

[MC Return (mc-return)]

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 ?

[Artur (nwkj86)]

Wow. Thank you a lot for your comment. I'll apply your suggestions and try to make this change in the right way. I'll push something soon.

About 6: I'm setting background_pixel = 0, because *switcher is doing that in original version. Yes, unchecking EnableBackground option resets it to be transparent (as before my changes).

[MC Return (mc-return)]

> About 6: I'm setting background_pixel = 0, because *switcher is doing that in
> original version. Yes, unchecking EnableBackground option resets it to be
> transparent (as before my changes).

Yeah, it was only my first test version, which did not update the useBackgroundColor bool correctly - I had to fix this by manually updating this bool shortly after creation of the popup window (hint if you experience the same problem).

Here the improved version for the updateBackground function, now also using some magic to factor in transparency into background_pixel, maybe you want to use it:

void
BaseSwitchScreen::updateBackground (bool useBackgroundColor,
        unsigned short backgroundColor [])
{
    if (!popupWindow)
 return;

    unsigned long background_pixel;

    if (useBackgroundColor)
 background_pixel = (unsigned long)((((backgroundColor [3] * backgroundColor [2]) >> 24) & 0x0000ff) |
        (((backgroundColor [3] * backgroundColor [1]) >> 16) & 0x00ff00) |
        (((backgroundColor [3] * backgroundColor [0]) >> 8) & 0xff0000) |
        (((backgroundColor [3] & 0xff00) << 16)));
    else
 background_pixel = 0ul;

    XSetWindowBackground (screen->dpy (), popupWindow, background_pixel);
}

[Artur (nwkj86)]

Hi,

I think that problem with not updating background settings is caused by:

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

optionGetBackgroundColor is binded once forever.

Consider:
#include <boost/bind.hpp>
#include <iostream>
using namespace std;
using namespace boost;
int getI()
{
  static int i;
  i++;
  cout << "getI() = " << i << endl;
  return i;
}
void printer(int i)
{
  cout << "printer(" << i << ")" << endl;
}
int main()
{
  auto p = bind(&printer, getI());
  p();
  p();
  p();
  return 0;
}

What should be printed?

BTW: is C++11 available in compiz?

[Stephen M. Webb (bregma)]

We recently switched to building with C++11 enabled because a dependency used by some plugins requires it. There is currently no C++11 code in Compiz itself but that may change in the near future.

[Artur (nwkj86)]

I don't see --std=c++11 (or similar) in main CMakeLists.txt, but I see it in compizconfig/ and in debian/rules.

I'm on bzr+ssh://bazaar.launchpad.net/~compiz-team/compiz/0.9.12/ branch.

I've added 'const auto &test = *this;' and I see

warning: ‘auto’ changes meaning in C++11; please remove it [-Wc++0x-compat]

This code compiles when I add '-DCMAKE_CXX_FLAGS' to cmake command.

Am I doing something wrong?

[Artur (nwkj86)]

I meant: '-DCMAKE_CXX_FLAGS="--std=c++11"'.

[Stephen M. Webb (bregma)]

No, you're not doing anything wrong. The CMake recipes have not been updated to force C++-11 mode, just the Debian packaging. There's an MP floating around to fix the packaging.

[lp:~nwkj86/compiz/add-background-conf-to-switcher]

3974. By Artur on 2015-08-22

Post review changes.

* change setBackground to updateBackground and move it to base class
* fix typo

[MC Return (mc-return)]

LGTM. +1

review: Approve

[Artur (nwkj86)]

Thanks.

=== modified file 'plugins/compiztoolbox/include/compiztoolbox/compiztoolbox.h'
--- plugins/compiztoolbox/include/compiztoolbox/compiztoolbox.h	2013-10-31 15:59:14 +0000
+++ plugins/compiztoolbox/include/compiztoolbox/compiztoolbox.h	2015-08-22 21:54:12 +0000
@@ -63,7 +63,7 @@
     AllViewports,
     Panels,
     Group
-} SwitchWindowSelection;	    
+} SwitchWindowSelection;
 
 class BaseSwitchScreen
 {
@@ -75,6 +75,7 @@
 	void setSelectedWindowHint (bool focus);
 	void activateEvent (bool activating);
 	void updateForegroundColor ();
+	void updateBackground (bool useBackgroundColor, unsigned short backgroundColor[]);
 
 	CompWindow *switchToWindow (bool toNext, bool autoChangeVPOption, bool focus);
 	static bool compareWindows (CompWindow *w1, CompWindow *w2);

=== modified file 'plugins/compiztoolbox/src/compiztoolbox.cpp'
--- plugins/compiztoolbox/src/compiztoolbox.cpp	2013-05-12 08:13:05 +0000
+++ plugins/compiztoolbox/src/compiztoolbox.cpp	2015-08-22 21:54:12 +0000
@@ -124,6 +124,24 @@
     return "";
 }
 
+void
+BaseSwitchScreen::updateBackground(bool           useBackgroundColor,
+				   unsigned short backgroundColor[])
+{
+    if (!popupWindow)
+	return;
+
+    unsigned long  background_pixel = 0ul;
+    if (useBackgroundColor)
+    {
+	background_pixel = ((((static_cast<unsigned long>(backgroundColor [3]) * backgroundColor [2]) >> 24) & 0x0000ff) |
+	(((backgroundColor [3] * backgroundColor [1]) >> 16) & 0x00ff00) |
+	(((backgroundColor [3] * backgroundColor [0]) >> 8) & 0xff0000) |
+	(((backgroundColor [3] & 0xff00) << 16)));
+    }
+
+    XSetWindowBackground (screen->dpy(), popupWindow, background_pixel);
+}
 
 void
 BaseSwitchScreen::setSelectedWindowHint (bool focus)

=== modified file 'plugins/staticswitcher/src/staticswitcher.cpp'
--- plugins/staticswitcher/src/staticswitcher.cpp	2013-06-11 04:45:57 +0000
+++ plugins/staticswitcher/src/staticswitcher.cpp	2015-08-22 21:54:12 +0000
@@ -286,6 +286,7 @@
 			 (unsigned char *) &Atoms::winTypeUtil, 1);
 
 	::screen->setWindowProp (popupWindow, Atoms::winDesktop, 0xffffffff);
+	updateBackground(optionGetUseBackgroundColor(), optionGetBackgroundColor());
 
 	setSelectedWindowHint (false);
 
@@ -1357,6 +1358,10 @@
     move (0),
     mouseSelect (false)
 {
+    auto bgUpdater = [=] (...){ this->updateBackground (this->optionGetUseBackgroundColor (), this->optionGetBackgroundColor ());};
+    optionSetUseBackgroundColorNotify (bgUpdater);
+    optionSetBackgroundColorNotify (bgUpdater);
+
 #define SWITCHBIND(a,b,c) boost::bind (switchInitiateCommon, _1, _2, _3, a, b, c)
 
     optionSetNextButtonInitiate (SWITCHBIND (CurrentViewport, true, true));

=== modified file 'plugins/staticswitcher/staticswitcher.xml.in'
--- plugins/staticswitcher/staticswitcher.xml.in	2012-10-15 10:31:51 +0000
+++ plugins/staticswitcher/staticswitcher.xml.in	2015-08-22 21:54:12 +0000
@@ -293,6 +293,24 @@
 			</default>
 		    </option>
 		</subgroup>
+		<subgroup>
+		    <_short>Background</_short>
+		    <option name="use_background_color" type="bool">
+			<_short>Set background color</_short>
+			<_long>Set background color</_long>
+			<default>false</default>
+		    </option>
+		    <option name="background_color" type="color">
+			<_short>Background Color</_short>
+			<_long>Background color of the switcher window.</_long>
+			<default>
+			    <red>0x3333</red>
+			    <green>0x3333</green>
+			    <blue>0x3333</blue>
+			    <alpha>0xd998</alpha>
+			</default>
+		    </option>
+		</subgroup>
 	    </group>
 	</options>
     </plugin>

=== modified file 'plugins/switcher/src/switcher.cpp'
--- plugins/switcher/src/switcher.cpp	2015-07-25 20:01:51 +0000
+++ plugins/switcher/src/switcher.cpp	2015-08-22 21:54:12 +0000
@@ -66,7 +66,6 @@
      (WIDTH >> 1), HEIGHT - BOX_WIDTH, 0.0f,
 };
 
-
 void
 SwitchScreen::updateWindowList (int count)
 {
@@ -300,6 +299,7 @@
 			 (unsigned char *) &Atoms::winTypeUtil, 1);
 
 	screen->setWindowProp (popupWindow, Atoms::winDesktop, 0xffffffff);
+        updateBackground (optionGetUseBackgroundColor (), optionGetBackgroundColor ());
 
 	setSelectedWindowHint (false);
     }
@@ -1123,6 +1123,10 @@
 
     optionSetZoomNotify (boost::bind (&SwitchScreen::setZoom, this));
 
+    auto bgUpdater = [=] (...){ this->updateBackground (this->optionGetUseBackgroundColor (), this->optionGetBackgroundColor ());};
+    optionSetUseBackgroundColorNotify (bgUpdater);
+    optionSetBackgroundColorNotify (bgUpdater);
+
 #define SWITCHBIND(a,b,c) boost::bind (switchInitiateCommon, _1, _2, _3, a, b, c)
 
     optionSetNextButtonInitiate (SWITCHBIND (CurrentViewport, true, true));

=== modified file 'plugins/switcher/switcher.xml.in'
--- plugins/switcher/switcher.xml.in	2012-10-15 10:31:51 +0000
+++ plugins/switcher/switcher.xml.in	2015-08-22 21:54:12 +0000
@@ -166,6 +166,24 @@
 		<_long>Rotate to the selected window while switching</_long>
 		<default>false</default>
 	    </option>
+	    <subgroup>
+		<_short>Background</_short>
+		<option name="use_background_color" type="bool">
+		    <_short>Set background color</_short>
+		    <_long>Set background color</_long>
+		    <default>false</default>
+		</option>
+		<option name="background_color" type="color">
+		    <_short>Background Color</_short>
+		    <_long>Background color of the switcher window.</_long>
+		    <default>
+			<red>0x3333</red>
+			<green>0x3333</green>
+			<blue>0x3333</blue>
+			<alpha>0xd998</alpha>
+		    </default>
+		</option>
+	    </subgroup>
 	</options>
     </plugin>
 </compiz>