Merge lp:~compiz-team/compiz/compiz.fix_1063617.3 into lp:compiz/0.9.9

[Sam Spilsbury]

Refactored CCSSettingDefaultImpl construction - export a
ccsSettingDefaultImplNew which can be provided custom methods on how to
initialize setting default values and info (eg, XML, ProtoBuf or Mock).

Added a simple construction test to prove that it works.

NOTE: I have a branch here which merges all of these together without conflicts, so just don't merge this one directly

This is about preparing a fix for (LP: #1063617).

(LP: #1063617) is really complicated. The problem is that there is a feedback loop between integrated keys and normal keys which can cause the integrated keys to read from normal keys which will be out of date or incorrect because they were never updated.

A part of that problem is that we need to have more information in the backends as to why setting a key to a particular value failed or succeeded.

Refactored CCSSettingDefaultImpl construction - export a
ccsSettingDefaultImplNew which can be provided custom methods on how to
initialize setting default values and info (eg, XML, ProtoBuf or Mock).

Added a simple construction test to prove that it works.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3481
http://jenkins.qa.ubuntu.com/job/compiz-ci/266/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/266/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/266//rebuild/?

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Works for me.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
Approved revid is not set in launchpad (maybe a permission problem?).
http://jenkins.qa.ubuntu.com/job/compiz-mbs-autolanding/98/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-mbs-autolanding/./build=pbuilder,distribution=quantal,flavor=amd64/98/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-mbs-autolanding/./build=pbuilder,distribution=quantal,flavor=i386/98/console

review: Needs Fixing (continuous-integration)

[Sam Spilsbury (smspillaz)]

(Ignore jenkins)

[Sam Spilsbury (smspillaz)]

Hmm, I said don't merge these manually ...

In any case please don't merge the other ones. Wait till they're
approved and I'll drop the proposals and propose one with the
conflicts resolved.

On Tue, Nov 27, 2012 at 12:00 PM, <email address hidden> wrote:
> The proposal to merge lp:~compiz-team/compiz/compiz.fix_1063617.3 into lp:compiz has been updated.
>
> Status: Approved => Merged
>
> For more details, see:
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1063617.3/+merge/136037
> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1063617.3/+merge/136037
> Your team Compiz Maintainers is subscribed to branch lp:compiz.

--
Sam Spilsbury

[Daniel van Vugt (vanvugt)]

Sorry, I was looking for the do-not-merge messages but somehow missed this one.

[Sam Spilsbury (smspillaz)]

No problem, this one is probably resolvable

On Tue, Nov 27, 2012 at 1:07 PM, Daniel van Vugt
<email address hidden> wrote:
> Sorry, I was looking for the do-not-merge messages but somehow missed this one.
>
>
> --
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1063617.3/+merge/136037
> Your team Compiz Maintainers is subscribed to branch lp:compiz.

--
Sam Spilsbury

=== modified file 'compizconfig/libcompizconfig/include/ccs.h'
--- compizconfig/libcompizconfig/include/ccs.h	2012-10-03 10:50:07 +0000
+++ compizconfig/libcompizconfig/include/ccs.h	2012-11-25 02:36:21 +0000
@@ -647,6 +647,31 @@
     CCSObject object;
 };
 
+typedef void (*CCSSettingDefaultValueInitializerFunc) (CCSSettingType  type,
+						       CCSSettingInfo  *info,
+						       CCSSettingValue *value,
+						       void            *data);
+
+typedef void (*CCSSettingInfoInitializerFunc) (CCSSettingType  type,
+					       CCSSettingInfo  *info,
+					       void            *data);
+
+CCSSetting *
+ccsSettingDefaultImplNew (CCSPlugin                             *plugin,
+			  const char                            *name,
+			  CCSSettingType                        type,
+			  const char                            *shortDesc,
+			  const char                            *longDesc,
+			  const char                            *hints,
+			  const char                            *group,
+			  const char                            *subGroup,
+			  CCSSettingDefaultValueInitializerFunc valueInit,
+			  void                                  *valueInitData,
+			  CCSSettingInfoInitializerFunc         infoInit,
+			  void                                  *infoInitData,
+			  CCSObjectAllocationInterface          *ai,
+			  const CCSInterfaceTable               *interfaces);
+
 struct _CCSPluginCategory
 {
     const char *name;      /* plugin category name */

=== modified file 'compizconfig/libcompizconfig/src/compiz.cpp'
--- compizconfig/libcompizconfig/src/compiz.cpp	2012-08-15 10:19:02 +0000
+++ compizconfig/libcompizconfig/src/compiz.cpp	2012-11-25 02:36:21 +0000
@@ -547,178 +547,233 @@
 }
 
 static void
+ccsSettingInfoPBInitializer (CCSSettingType type,
+			     CCSSettingInfo *info,
+			     void           *data)
+{
+    const OptionMetadata &option (*((const OptionMetadata *) data));
+
+    switch (type)
+    {
+    case TypeInt:
+	initIntInfoPB (info, option);
+	break;
+    case TypeFloat:
+	initFloatInfoPB (info, option);
+	break;
+    case TypeString:
+	initStringInfoPB (info, option);
+	break;
+    case TypeList:
+	initListInfoPB (info, option);
+	break;
+    case TypeKey:
+    case TypeButton:
+    case TypeEdge:
+    case TypeBell:
+	initActionInfoPB (info, option);
+	break;
+    case TypeAction: // do nothing and fall through
+    default:
+	break;
+    }
+}
+
+static void
+ccsSettingDefaultValuePBInitializer (CCSSettingType  type,
+				     CCSSettingInfo  *info,
+				     CCSSettingValue *value,
+				     void            *data)
+{
+    const OptionMetadata &option (*((const OptionMetadata *) data));
+
+    switch (type)
+    {
+    case TypeInt:
+	initIntValuePB (value, info,
+			option.default_value (0));
+	break;
+    case TypeBool:
+	initBoolValuePB (value, option.default_value (0));
+	break;
+    case TypeFloat:
+	initFloatValuePB (value, info,
+			  option.default_value (0));
+	break;
+    case TypeString:
+	initStringValuePB (value, info,
+			   option.default_value (0));
+	break;
+    case TypeColor:
+	initColorValuePB (value, option.default_value (0));
+	break;
+    case TypeKey:
+	initKeyValuePB (value, info,
+			option.default_value (0));
+	break;
+    case TypeButton:
+	initButtonValuePB (value, info,
+			   option.default_value (0));
+	break;
+    case TypeEdge:
+	initEdgeValuePB (value, info,
+			 option.default_value (0));
+	break;
+    case TypeBell:
+	initBellValuePB (value, info,
+			 option.default_value (0));
+	break;
+    case TypeMatch:
+	initMatchValuePB (value,
+			  option.default_value (0));
+	break;
+    case TypeList:
+	initListValuePB (value, info,
+			 option);
+	break;
+    case TypeAction: // do nothing and fall through
+    default:
+	break;
+    }
+}
+
+static void
+ccsSettingDefaultValueEmptyInitializer (CCSSettingType  type,
+					CCSSettingInfo  *info,
+					CCSSettingValue *value,
+					void            *data)
+{
+    /* if we have no set defaults, we have at least to set
+       the string defaults to empty strings */
+    switch (type)
+    {
+    case TypeString:
+	value->value.asString = strdup ("");
+	break;
+    case TypeMatch:
+	value->value.asMatch = strdup ("");
+	break;
+    default:
+	break;
+    }
+}
+
+CCSSetting *
+ccsSettingDefaultImplNew (CCSPlugin                             *plugin,
+			  const char                            *name,
+			  CCSSettingType                        type,
+			  const char                            *shortDesc,
+			  const char                            *longDesc,
+			  const char                            *hints,
+			  const char                            *group,
+			  const char                            *subGroup,
+			  CCSSettingDefaultValueInitializerFunc valueInit,
+			  void                                  *valueInitData,
+			  CCSSettingInfoInitializerFunc         infoInit,
+			  void                                  *infoInitData,
+			  CCSObjectAllocationInterface          *ai,
+			  const CCSInterfaceTable               *interfaces)
+{
+    CCSSetting *setting = (CCSSetting *) calloc (1, sizeof (CCSSetting));
+
+    if (!setting)
+	return NULL;
+
+    ccsObjectInit (setting, ai);
+
+    CCSSettingPrivate *sPrivate = (CCSSettingPrivate *) (*ai->calloc_) (ai->allocator, 1, sizeof (CCSSettingPrivate));
+
+    if (!sPrivate)
+    {
+	free (setting);
+	return NULL;
+    }
+
+    ccsObjectSetPrivate (setting, (CCSPrivate *) sPrivate);
+    ccsObjectAddInterface (setting, (CCSInterface *) interfaces->settingInterface, GET_INTERFACE_TYPE (CCSSettingInterface));
+    ccsSettingRef (setting);
+
+    sPrivate->parent = plugin;
+    sPrivate->isDefault = TRUE;
+    sPrivate->name = strdup (name);
+
+    sPrivate->shortDesc = strdup (shortDesc);
+    sPrivate->longDesc  = strdup (longDesc);
+    sPrivate->group     = strdup (group);
+    sPrivate->hints     = strdup (hints);
+    sPrivate->subGroup  = strdup (subGroup);
+
+    sPrivate->type = type;
+    sPrivate->value = &sPrivate->defaultValue;
+    sPrivate->defaultValue.parent = setting;
+
+    (*infoInit) (type, &sPrivate->info, infoInitData);
+    (*valueInit) (type, &sPrivate->info, &sPrivate->defaultValue, valueInitData);
+
+    return setting;
+}
+
+static void
 addOptionForPluginPB (CCSPlugin * plugin,
 		      const char * name,
 		      const StringList & groups,
 		      const StringList & subgroups,
 		      const OptionMetadata & option)
 {
-    CCSSetting *setting;
-
     if (ccsFindSetting (plugin, name))
     {
 	ccsError ("Option \"%s\" already defined", name);
 	return;
     }
 
-    CCSContext *context = ccsPluginGetContext (plugin);
-    CCSContextPrivate *cPrivate = GET_PRIVATE (CCSContextPrivate, context);
-
-    setting = (CCSSetting *) calloc (1, sizeof (CCSSetting));
-
-    if (!setting)
-	return;
-
-    ccsObjectInit (setting, &ccsDefaultObjectAllocator);
-
-    CCSSettingPrivate *ccsPrivate = (CCSSettingPrivate *) calloc (1, sizeof (CCSSettingPrivate));
-
-    if (!ccsPrivate)
-    {
-	free (setting);
-	return;
-    }
-
-    ccsObjectSetPrivate (setting, (CCSPrivate *) ccsPrivate);
-    ccsObjectAddInterface (setting, (CCSInterface *) cPrivate->object_interfaces->settingInterface, GET_INTERFACE_TYPE (CCSSettingInterface));
-    ccsSettingRef (setting);
-
-    CCSSettingPrivate *sPrivate = GET_PRIVATE (CCSSettingPrivate, setting);
-
-    sPrivate->parent = plugin;
-    sPrivate->isDefault = TRUE;
-    sPrivate->name = strdup (name);
+    const char *shortDesc = name;
+    const char *longDesc  = "";
+    const char *group     = "";
+    const char *hints     = "";
+    const char *subGroup  = "";
+
+    CCSSettingType type = (CCSSettingType) option.type ();
 
     if (!basicMetadata)
     {
-	sPrivate->shortDesc =
-	    strdup (option.has_short_desc () ?
+	shortDesc = option.has_short_desc () ?
 		    option.short_desc ().c_str () :
-		    name);
-	sPrivate->longDesc =
-	    strdup (option.has_long_desc () ?
+		    name;
+	longDesc =  option.has_long_desc () ?
 		    option.long_desc ().c_str () :
-		    name);
-	sPrivate->hints = strdup (option.has_hints () ?
-				 option.hints ().c_str () :
-				 name);
-	sPrivate->group =
-	    strdup (option.group_id () >= 0 ?
-		    groups.Get (option.group_id ()).c_str () :
-		    "");
-	sPrivate->subGroup =
-	    strdup (option.subgroup_id () >= 0 ?
-		    subgroups.Get (option.subgroup_id ()).c_str () :
-		    "");
-    }
-    else
-    {
-	sPrivate->shortDesc = strdup (name);
-	sPrivate->longDesc  = strdup ("");
-	sPrivate->hints     = strdup ("");
-	sPrivate->group     = strdup ("");
-	sPrivate->subGroup  = strdup ("");
-    }
-
-    sPrivate->type = (CCSSettingType) option.type ();
-    sPrivate->value = &sPrivate->defaultValue;
-    sPrivate->defaultValue.parent = setting;
-
-    switch (sPrivate->type)
-    {
-    case TypeInt:
-	initIntInfoPB (&sPrivate->info, option);
-	break;
-    case TypeFloat:
-	initFloatInfoPB (&sPrivate->info, option);
-	break;
-    case TypeString:
-	initStringInfoPB (&sPrivate->info, option);
-	break;
-    case TypeList:
-	initListInfoPB (&sPrivate->info, option);
-	break;
-    case TypeKey:
-    case TypeButton:
-    case TypeEdge:
-    case TypeBell:
-	initActionInfoPB (&sPrivate->info, option);
-	break;
-    case TypeAction: // do nothing and fall through
-    default:
-	break;
-    }
-
-    if (option.default_value_size () > 0)
-    {
-	switch (sPrivate->type)
-	{
-	case TypeInt:
-	    initIntValuePB (&sPrivate->defaultValue, &sPrivate->info,
-			    option.default_value (0));
-	    break;
-	case TypeBool:
-	    initBoolValuePB (&sPrivate->defaultValue, option.default_value (0));
-	    break;
-	case TypeFloat:
-	    initFloatValuePB (&sPrivate->defaultValue, &sPrivate->info,
-			      option.default_value (0));
-	    break;
-	case TypeString:
-	    initStringValuePB (&sPrivate->defaultValue, &sPrivate->info,
-			       option.default_value (0));
-	    break;
-	case TypeColor:
-	    initColorValuePB (&sPrivate->defaultValue, option.default_value (0));
-	    break;
-	case TypeKey:
-	    initKeyValuePB (&sPrivate->defaultValue, &sPrivate->info,
-			    option.default_value (0));
-	    break;
-	case TypeButton:
-	    initButtonValuePB (&sPrivate->defaultValue, &sPrivate->info,
-			       option.default_value (0));
-	    break;
-	case TypeEdge:
-	    initEdgeValuePB (&sPrivate->defaultValue, &sPrivate->info,
-			     option.default_value (0));
-	    break;
-	case TypeBell:
-	    initBellValuePB (&sPrivate->defaultValue, &sPrivate->info,
-			     option.default_value (0));
-	    break;
-	case TypeMatch:
-	    initMatchValuePB (&sPrivate->defaultValue,
-			      option.default_value (0));
-	    break;
-	case TypeList:
-	    initListValuePB (&sPrivate->defaultValue, &sPrivate->info,
-			     option);
-	    break;
-	case TypeAction: // do nothing and fall through
-	default:
-	    break;
-	}
-    }
-    else
-    {
-	/* if we have no set defaults, we have at least to set
-	   the string defaults to empty strings */
-	switch (sPrivate->type)
-	{
-	case TypeString:
-	    sPrivate->defaultValue.value.asString = strdup ("");
-	    break;
-	case TypeMatch:
-	    sPrivate->defaultValue.value.asMatch = strdup ("");
-	    break;
-	default:
-	    break;
-	}
-    }
-
+		    name;
+	hints = option.has_hints () ?
+		option.hints ().c_str () :
+		name;
+	group = option.group_id () >= 0 ?
+		groups.Get (option.group_id ()).c_str () :
+		"";
+	subGroup = option.subgroup_id () >= 0 ?
+		   subgroups.Get (option.subgroup_id ()).c_str () :
+		   "";
+    }
+
+    CCSContext *context = ccsPluginGetContext (plugin);
+    CCSContextPrivate *cPrivate = GET_PRIVATE (CCSContextPrivate, context);
     CCSPluginPrivate *pPrivate = GET_PRIVATE (CCSPluginPrivate, plugin);
 
+    CCSSetting *setting = ccsSettingDefaultImplNew (plugin,
+						    name,
+						    type,
+						    shortDesc,
+						    longDesc,
+						    hints,
+						    group,
+						    subGroup,
+						    option.default_value_size () > 0 ?
+							ccsSettingDefaultValuePBInitializer :
+							ccsSettingDefaultValueEmptyInitializer,
+						    (void *) &option,
+						    ccsSettingInfoPBInitializer,
+						    (void *) &option,
+						    plugin->object.object_allocation,
+						    cPrivate->object_interfaces);
+
     pPrivate->settings = ccsSettingListAppend (pPrivate->settings, setting);
 }
 

=== modified file 'compizconfig/libcompizconfig/tests/compizconfig_test_ccs_setting.cpp'
--- compizconfig/libcompizconfig/tests/compizconfig_test_ccs_setting.cpp	2012-08-09 18:21:41 +0000
+++ compizconfig/libcompizconfig/tests/compizconfig_test_ccs_setting.cpp	2012-11-25 02:36:21 +0000
@@ -1,17 +1,39 @@
+/*
+ * Compiz configuration system library
+ *
+ * Copyright (C) 2012 Canonical Ltd
+ * Copyright (C) 2012 Sam Spilsbury <[e-mail]>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
+*/
+
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
 
+#include <gtest_shared_autodestroy.h>
+#include <boost/shared_ptr.hpp>
+#include <boost/make_shared.hpp>
+#include <boost/function.hpp>
+
 #include <ccs.h>
 
 #include "compizconfig_ccs_setting_mock.h"
+#include "compizconfig_ccs_plugin_mock.h"
 
 using ::testing::_;
 using ::testing::Return;
-
-class CCSSettingTest :
-    public ::testing::Test
-{
-};
+using ::testing::InSequence;
 
 TEST(CCSSettingTest, TestMock)
 {
@@ -109,3 +131,122 @@
 
     ccsSettingUnref (setting);
 }
+
+namespace
+{
+class MockInitializerFuncs
+{
+    public:
+
+	MOCK_METHOD3 (initializeInfo, void (CCSSettingType, CCSSettingInfo *, void *));
+	MOCK_METHOD4 (initializeDefaultValue, void (CCSSettingType, CCSSettingInfo *, CCSSettingValue *, void *));
+
+	static void wrapInitializeInfo (CCSSettingType type,
+					CCSSettingInfo *info,
+					void           *data)
+	{
+	    MockInitializerFuncs &funcs (*(reinterpret_cast <MockInitializerFuncs *> (data)));
+	    funcs.initializeInfo (type, info, data);
+	}
+
+	static void wrapInitializeValue (CCSSettingType  type,
+					 CCSSettingInfo  *info,
+					 CCSSettingValue *value,
+					 void            *data)
+	{
+	    MockInitializerFuncs &funcs (*(reinterpret_cast <MockInitializerFuncs *> (data)));
+	    funcs.initializeDefaultValue (type, info, value, data);
+	}
+};
+
+const std::string SETTING_NAME = "setting_name";
+const std::string SETTING_SHORT_DESC = "Short Description";
+const std::string SETTING_LONG_DESC = "Long Description";
+const std::string SETTING_GROUP = "Group";
+const std::string SETTING_SUBGROUP = "Sub Group";
+const std::string SETTING_HINTS = "Hints";
+const CCSSettingType SETTING_TYPE = TypeInt;
+}
+
+class CCSSettingDefaultImplTest :
+    public ::testing::Test
+{
+    public:
+
+	CCSSettingDefaultImplTest () :
+	    plugin (AutoDestroy (ccsMockPluginNew (),
+				 ccsPluginUnref)),
+	    gmockPlugin (reinterpret_cast <CCSPluginGMock *> (ccsObjectGetPrivate (plugin.get ())))
+	{
+	}
+
+	virtual void SetUpSetting (MockInitializerFuncs &funcs)
+	{
+	    void                 *vFuncs = reinterpret_cast <void *> (&funcs);
+
+	    /* Info must be initialized before the value */
+	    InSequence s;
+
+	    EXPECT_CALL (funcs, initializeInfo (GetSettingType (), _, vFuncs));
+	    EXPECT_CALL (funcs, initializeDefaultValue (GetSettingType (), _, _, vFuncs));
+
+	    setting = AutoDestroy (ccsSettingDefaultImplNew (plugin.get (),
+							     SETTING_NAME.c_str (),
+							     GetSettingType (),
+							     SETTING_SHORT_DESC.c_str (),
+							     SETTING_LONG_DESC.c_str (),
+							     SETTING_HINTS.c_str (),
+							     SETTING_GROUP.c_str (),
+							     SETTING_SUBGROUP.c_str (),
+							     MockInitializerFuncs::wrapInitializeValue,
+							     vFuncs,
+							     MockInitializerFuncs::wrapInitializeInfo,
+							     vFuncs,
+							     &ccsDefaultObjectAllocator,
+							     &ccsDefaultInterfaceTable),
+				   ccsSettingUnref);
+	}
+
+	virtual void SetUp ()
+	{
+	    MockInitializerFuncs funcs;
+
+	    SetUpSetting (funcs);
+
+	}
+
+	virtual CCSSettingType GetSettingType ()
+	{
+	    return SETTING_TYPE;
+	}
+
+	boost::shared_ptr <CCSPlugin>  plugin;
+	CCSPluginGMock                 *gmockPlugin;
+	boost::shared_ptr <CCSSetting> setting;
+};
+
+TEST_F (CCSSettingDefaultImplTest, Construction)
+{
+    EXPECT_EQ (SETTING_TYPE, ccsSettingGetType (setting.get ()));
+    EXPECT_EQ (SETTING_NAME, ccsSettingGetName (setting.get ()));
+    EXPECT_EQ (SETTING_SHORT_DESC, ccsSettingGetShortDesc (setting.get ()));
+    EXPECT_EQ (SETTING_LONG_DESC, ccsSettingGetLongDesc (setting.get ()));
+    EXPECT_EQ (SETTING_GROUP, ccsSettingGetGroup (setting.get ()));
+    EXPECT_EQ (SETTING_SUBGROUP, ccsSettingGetSubGroup (setting.get ()));
+    EXPECT_EQ (SETTING_HINTS, ccsSettingGetHints (setting.get ()));
+
+    /* Pointer Equality */
+    EXPECT_EQ (ccsSettingGetDefaultValue (setting.get ()),
+	       ccsSettingGetValue (setting.get ()));
+
+    /* Actual Equality */
+    EXPECT_TRUE (ccsCheckValueEq (ccsSettingGetDefaultValue (setting.get ()),
+				  ccsSettingGetType (setting.get ()),
+				  ccsSettingGetInfo (setting.get ()),
+				  ccsSettingGetValue (setting.get ()),
+				  ccsSettingGetType (setting.get ()),
+				  ccsSettingGetInfo (setting.get ())));
+
+    EXPECT_EQ (ccsSettingGetParent (setting.get ()),
+	       plugin.get ());
+}