Merge lp:~smspillaz/compiz-core/compiz-core.vector-color-variant into lp:compiz-core/0.9.5

[Sam Spilsbury]

Makes color wrap around std::vector

[Alan Griffiths (alan-griffiths)]

1. Don't #include <iostream> where it isn't needed. (I don't think you even need #include <iosfwd>.)

2. As you're specifying the length of the array as 4 here:

    Value( unsigned short c[4] ) : mListType(TypeUnset),
    mValue (ColorVector (c, c + sizeof (c) / sizeof (unsigned short)))

it seems overly complex to compute it by "sizeof (c) / sizeof (unsigned short))" - (even a "length_of" template would make the intention clearer).

review: Needs Fixing

[lp:~smspillaz/compiz-core/compiz-core.vector-color-variant]

2967. By Sam Spilsbury on 2012-01-25

Remove #include <iostream>

[Alan Griffiths (alan-griffiths)]

Also as written c is an "unsigned short*" so the calculation doesn't work as intended.

[Alan Griffiths (alan-griffiths)]

I suspect that:
58 + void set( unsigned short *s ) {
59 + mValue = ColorVector (s, s + sizeof (s) / sizeof (unsigned short));
60 + }

should be:

template<size_t N>
void set( unsigned short (const &s)[N] ) {
  mValue = ColorVector (s, s + N);
}

review: Needs Fixing

[lp:~smspillaz/compiz-core/compiz-core.vector-color-variant]

2968. By Sam Spilsbury on 2012-01-25

Explicitly require an array of unsigned short[4]

[Alan Griffiths (alan-griffiths)]

Rather than "+ std::cout << "color: " << color[0] << color[1] << color[2] << color[3] << std::endl;" ASSERT the expected values.

review: Needs Fixing

[lp:~smspillaz/compiz-core/compiz-core.vector-color-variant]

2969. By Sam Spilsbury on 2012-01-25

const correctness

[Alan Griffiths (alan-griffiths)]

Running the test:
$ src/option/tests/compiz_option_test
Running main() from gtest_main.cc
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from CompOption
[ RUN ] CompOption.Value
/home/alan/Documents/programming/c++/compiz/compiz-core.vector-color-variant/src/option/tests/option.cpp:15: Failure
Value of: type
  Actual: 4
Expected: v.type()
Which is: 0
[ FAILED ] CompOption.Value (1 ms)
[ RUN ] CompOption.Color
color: 25502550
Segmentation fault (core dumped)

review: Needs Fixing

Unmerged revisions

=== modified file 'include/core/abiversion.h'
--- include/core/abiversion.h	2012-01-24 08:24:18 +0000
+++ include/core/abiversion.h	2012-01-25 14:36:23 +0000
@@ -5,6 +5,6 @@
 #  error Conflicting definitions of CORE_ABIVERSION
 #endif
 
-#define CORE_ABIVERSION 20120123
+#define CORE_ABIVERSION 20120125
 
 #endif // COMPIZ_ABIVERSION_H

=== modified file 'include/core/option.h'
--- include/core/option.h	2012-01-24 15:22:49 +0000
+++ include/core/option.h	2012-01-25 14:36:23 +0000
@@ -74,12 +74,14 @@
 	class Value {
 	    public:
 
+		typedef std::vector <unsigned short> ColorVector;
+
 	  typedef boost::variant<
 	      bool,
 	      int,
 	      float,
 	      CompString,
-	      unsigned short*,
+	      boost::recursive_wrapper<ColorVector>,
 	      boost::recursive_wrapper<CompAction>,
 	      boost::recursive_wrapper<CompMatch>,
 	      boost::recursive_wrapper<std::vector<Value> >
@@ -97,6 +99,16 @@
 		mValue(t)
 		{
 		}
+		
+		Value( unsigned short const (& color)[4]  ) : mListType(TypeUnset),
+		    mValue (ColorVector (color, color + 4))
+		{
+		}
+
+		Value( const char *s ) : mListType(TypeUnset),
+		mValue( CompString (s) )
+		{
+		}
 
 		~Value();
 
@@ -117,12 +129,17 @@
 		{
 		    mValue = t;
 		}
+		
+		void set( const char* c ) {
+			mValue = CompString(c);
+		}
+		
+		void set( unsigned short const (& color)[4]) {
+		    mValue = ColorVector (color, color + 4);
+		}
 
-		/* In order to be exception safe, this MUST
-		 * return a copy. Prefer to use a specific
-		 * member instead */
 		template<typename T>
-		const T & get (const T & defaultValue = T ()) const
+		const T & get () const
 		{
 		    try
 		    {

=== modified file 'src/option.cpp'
--- src/option.cpp	2012-01-24 14:16:08 +0000
+++ src/option.cpp	2012-01-25 14:36:23 +0000
@@ -138,7 +138,7 @@
 {
     try
     {
-	return boost::get<unsigned short*>(mValue);
+	return const_cast <unsigned short *> (&((boost::get<ColorVector>(mValue))[0]));
     }
     catch (...)
     {

=== modified file 'src/option/tests/option.cpp'
--- src/option/tests/option.cpp	2012-01-18 16:56:31 +0000
+++ src/option/tests/option.cpp	2012-01-25 14:36:23 +0000
@@ -15,6 +15,32 @@
 	ASSERT_EQ(v.type(),type);
 	ASSERT_EQ (v.get<T>(),value);
     }
+    
+    unsigned short testColor[4] = {255, 0, 255, 0};
+    unsigned short testColor2[4] = {0, 255, 0, 255};
+
+    template<typename T>
+    void
+    check_list_type(CompOption::Type listType, CompOption::Value::Vector &list)
+    {
+	CompOption::Value vl;
+	vl.set (list);
+
+	ASSERT_EQ (vl.type (), CompOption::TypeList);
+	ASSERT_EQ (vl.get <CompOption::Value::Vector> (), list);
+
+	for (CompOption::Value::Vector::const_iterator it = vl.get <CompOption::Value::Vector> ().begin ();
+	     it != vl.get <CompOption::Value::Vector> ().end ();
+	     it++)
+	{
+	    T inst;
+	    CompOption::Value value (inst);
+
+	    const CompOption::Value &v (*it);
+
+	    ASSERT_EQ (v.type (), value.type ());
+	}
+    }
 }
 
 TEST(CompOption,Value)
@@ -25,15 +51,50 @@
 
     check_type_value<int> (CompOption::TypeInt, 1);
     check_type_value<float> (CompOption::TypeFloat, 1.f);
-    check_type_value<CompString> (CompOption::TypeString, CompString("Check"));
-
+    check_type_value<CompString> (CompOption::TypeString, CompString ("Check"));
+    check_type_value<CompString> (CompOption::TypeString, "core");
+    
     check_type_value<CompAction> (CompOption::TypeAction, CompAction());
     check_type_value<CompMatch> (CompOption::TypeMatch, CompMatch());
 
+    check_type_value<unsigned short*> (CompOption::TypeColor, testColor);
+	
     check_type_value<CompOption::Value::Vector> (CompOption::TypeList, CompOption::Value::Vector(5));
 
     CompOption::Value v1, v2;
     ASSERT_EQ (v1,v2);
     v1.set (CompString("SomeString"));
     ASSERT_TRUE(v1 != v2);
+
+    CompOption::Value::Vector vec;
+    CompOption::Value v;
+
+    v.set (true);
+    vec.push_back (v);
+    vec.push_back (v);
+
+    check_list_type<bool> (CompOption::TypeBool, vec);
+
+    vec.clear ();
+    v.set (CompString ("foo"));
+    vec.push_back (v);
+    vec.push_back (v);
+
+    check_list_type<CompString> (CompOption::TypeString, vec);
+}
+
+TEST(CompOption,Color)
+{
+
+    CompOption::Value value(testColor);
+    unsigned short * color = value.c();
+    std::cout << "color: " << color[0] << color[1] << color[2] << color[3] << std::endl;
+    color = value.get<unsigned short*>();
+    std::cout << "color: " << color[0] << color[1] << color[2] << color[3] << std::endl;
+    value.set(testColor2);
+    color = value.c();
+    std::cout << "color: " << color[0] << color[1] << color[2] << color[3] << std::endl;
+    color = value.get<unsigned short*>();
+    std::cout << "color: " << color[0] << color[1] << color[2] << color[3] << std::endl;
+    
 }