Merge lp:~compiz-team/compiz/compiz.fix_1080989 into lp:compiz/0.9.9

[Sam Spilsbury]

Fix various test failures caused by not controlling the environment variable
COMPIZ_CONFIG_PROFILE. (LP: #1070817) (LP: #1077787)

If the user overrides COMPIZ_CONFIG_PROFILE with an empty string, it probably
means that they meant to force the use of the general profile, since it
isn't meant to be set otherwise. Add tests for the new behaviour.

(LP: #1080989)

[PS Jenkins bot (ps-jenkins)]

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

I think this is unreadable:
TEST_F (CCSUtilProfileSelectionTest, TestOverrideCompizConfigProfileWithNameReturnsGeneralUnderscoreName)

Also note the gtest TEST macros insert the word Test so using "Test" in the parameters is probably redundant. For example:
    TEST (Hello, World)
generates:
    Hello_World_Test

[Daniel van Vugt (vanvugt)]

I can't reproduce anything like bug 1080989 to verify. But I can see that this seems to fix bug 1070817. Can we make it bug 1070817 instead or are they separate?

Also, the copyright header on new files should be one of your own :)

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

I think this fixes bug 1077787 too.

[Sam Spilsbury (smspillaz)]

Ah. Run it in a gnome shell session to reproduce. In any case I can change
the name of the tests and the branches, we should probably rename the tests
to stop using the word "Test" for the reason you specify.
On 20/11/2012 5:34 PM, "Daniel van Vugt" <email address hidden>
wrote:

> Review: Needs Fixing
>
> I can't reproduce anything like bug 1080989 to verify. But I can see that
> this seems to fix bug 1070817. Can we make it bug 1070817 instead or are
> they separate?
>
> Also, the copyright header on new files should be one of your own :)
> --
>
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1080989/+merge/135065
> Your team Compiz Maintainers is subscribed to branch lp:compiz.
>

[Sam Spilsbury (smspillaz)]

As for some other things:

Copyrights: no significant change, just extracting a header. Not worth it
IMO.

Test name: any ideas?

Separate bug: I think this is separate from the segfaults. The segfaults
are an issue with the gsettings back end, this one is about the global
profile selection being broken.
On 20/11/2012 5:34 PM, "Daniel van Vugt" <email address hidden>
wrote:

> Review: Needs Fixing
>
> I can't reproduce anything like bug 1080989 to verify. But I can see that
> this seems to fix bug 1070817. Can we make it bug 1070817 instead or are
> they separate?
>
> Also, the copyright header on new files should be one of your own :)
> --
>
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1080989/+merge/135065
> Your team Compiz Maintainers is subscribed to branch lp:compiz.
>

[Sam Spilsbury (smspillaz)]

I've linked the bugs you suggested and simplified the test names. Copyright in the header is unchanged, as its effectively not my code.

[PS Jenkins bot (ps-jenkins)]

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

OK, all good. Maybe if you're reorganizing other peoples' code it would be best to add your own copyright below the existing ones. Because clearly the structure of the code is no longer the work of the original author(s).

Also I personally have a pet hate of:
    if (strlen (profile))
because it's just an inefficient way of writing:
    if (profile[0])
But that's just me.

review: Approve

=== added file 'compizconfig/libcompizconfig/src/ccs-config-private.h'
--- compizconfig/libcompizconfig/src/ccs-config-private.h	1970-01-01 00:00:00 +0000
+++ compizconfig/libcompizconfig/src/ccs-config-private.h	2012-11-20 14:53:24 +0000
@@ -0,0 +1,32 @@
+/*
+ * Compiz configuration system library
+ *
+ * Copyright (C) 2007  Dennis Kasprzyk <[e-mail]>
+ * Copyright (C) 2007  Danny Baumann <[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
+ */
+#ifndef COMPIZCONFIG_CCS_CONFIG_PRIVATE_H
+#define COMPIZCONFIG_CCS_CONFIG_PRIVATE_H
+
+#include <ccs-defs.h>
+
+COMPIZCONFIG_BEGIN_DECLS
+
+char * getSectionName (void);
+
+COMPIZCONFIG_END_DECLS
+
+#endif

=== modified file 'compizconfig/libcompizconfig/src/config.c'
--- compizconfig/libcompizconfig/src/config.c	2012-05-21 09:36:37 +0000
+++ compizconfig/libcompizconfig/src/config.c	2012-11-20 14:53:24 +0000
@@ -26,6 +26,7 @@
 #include <string.h>
 
 #include "ccs-private.h"
+#include "ccs-config-private.h"
 
 #define SETTINGPATH "compiz-1/compizconfig"
 
@@ -56,17 +57,24 @@
     return NULL;
 }
 
-static char*
+char *
 getSectionName (void)
 {
     char *profile;
     char *section;
 
     profile = getenv ("COMPIZ_CONFIG_PROFILE");
-    if (profile && strlen (profile))
+    if (profile)
     {
-	if (asprintf (&section, "general_%s", profile) == -1)
-	    section = NULL;
+	if (strlen (profile))
+	{
+	    if (asprintf (&section, "general_%s", profile) == -1)
+		section = NULL;
+	}
+	else
+	{
+	    section = strdup ("general");
+	}
 
 	return section;
     }

=== modified file 'compizconfig/libcompizconfig/tests/compizconfig_test_ccs_util.cpp'
--- compizconfig/libcompizconfig/tests/compizconfig_test_ccs_util.cpp	2012-10-03 09:00:57 +0000
+++ compizconfig/libcompizconfig/tests/compizconfig_test_ccs_util.cpp	2012-11-20 14:53:24 +0000
@@ -19,11 +19,18 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include <iostream>
+
+#include <cstdlib>
+
 #include <gtest/gtest.h>
 #include <gtest_shared_characterwrapper.h>
 
+#include <boost/noncopyable.hpp>
+
 #include <ccs.h>
 #include <ccs-private.h>
+#include <ccs-config-private.h>
 #include <ccs-modifier-list-inl.h>
 
 using ::testing::WithParamInterface;
@@ -236,3 +243,133 @@
 
     EXPECT_EQ (expectedModifierMask, modifierMask);
 }
+
+namespace
+{
+static const std::string emptyProfileName ("");
+static const std::string globalTestProfileName ("test");
+static const std::string globalGeneralProfileSection ("general");
+static const std::string globalGeneralProfileSectionUnderscore ("general_");
+static const std::string globalTestGeneralProfileSection (globalGeneralProfileSectionUnderscore + globalTestProfileName);
+static const std::string globalGNOMEProfileSection ("gnome_session");
+static const std::string globalKDE4ProfileSection ("kde4_session");
+static const std::string globalKDE3ProfileSection ("kde_session");
+static const std::string four ("4");
+static const std::string trueStr ("true");
+
+class TmpEnv :
+    boost::noncopyable
+{
+    public:
+
+	explicit TmpEnv (const char *env, const char *val) :
+	    envRestore (env),
+	    envRestoreVal (getenv (env))
+	{
+	    if (!val)
+		unsetenv (env);
+	    else
+		setenv (env, val, TRUE);
+	}
+
+	~TmpEnv ()
+	{
+	    if (envRestoreVal)
+		setenv (envRestore, envRestoreVal, TRUE);
+	    else
+		unsetenv (envRestore);
+	}
+
+    private:
+
+	const char *envRestore;
+	const char *envRestoreVal;
+};
+
+const char * getConstCharFromCharacterWrapper (const CharacterWrapper &wrap)
+{
+    return wrap;
+}
+
+}
+
+std::ostream &
+operator<< (std::ostream &os, const CharacterWrapper &wrap)
+{
+    return os << getConstCharFromCharacterWrapper (wrap);
+}
+
+bool
+operator== (const std::string &str, const CharacterWrapper &wrap)
+{
+    return str == getConstCharFromCharacterWrapper (wrap);
+}
+
+class CCSUtilProfileSelection :
+    public ::testing::Test
+{
+    public:
+
+	CCSUtilProfileSelection () :
+	    ccsEnv ("COMPIZ_CONFIG_PROFILE", NULL),
+	    gnomeEnv ("GNOME_DESKTOP_SESSION_ID", NULL),
+	    kde4Env ("KDE_SESSION_VERSION", NULL),
+	    kdeEnv ("KDE_FULL_SESSION", NULL)
+	{
+	}
+
+    private:
+
+	TmpEnv ccsEnv;
+	TmpEnv gnomeEnv;
+	TmpEnv kde4Env;
+	TmpEnv kdeEnv;
+};
+
+TEST_F (CCSUtilProfileSelection, OverrideWithName)
+{
+    TmpEnv env ("COMPIZ_CONFIG_PROFILE", globalTestProfileName.c_str ());
+    CharacterWrapper sName (getSectionName ());
+
+    EXPECT_EQ (globalTestGeneralProfileSection, sName);
+}
+
+TEST_F (CCSUtilProfileSelection, OverrideWithNoName)
+{
+    TmpEnv env ("COMPIZ_CONFIG_PROFILE", emptyProfileName.c_str ());
+    CharacterWrapper sName (getSectionName ());
+
+    EXPECT_EQ (globalGeneralProfileSection, sName);
+}
+
+TEST_F (CCSUtilProfileSelection, NoOverrideInGNOME)
+{
+    TmpEnv env ("GNOME_DESKTOP_SESSION_ID", globalTestProfileName.c_str ());
+    CharacterWrapper sName (getSectionName ());
+
+    EXPECT_EQ (globalGNOMEProfileSection, sName);
+}
+
+TEST_F (CCSUtilProfileSelection, NoOverrideInKDE4)
+{
+    TmpEnv env ("KDE_SESSION_VERSION", four.c_str ());
+    CharacterWrapper sName (getSectionName ());
+
+    EXPECT_EQ (globalKDE4ProfileSection, sName);
+}
+
+TEST_F (CCSUtilProfileSelection, NoOverrideInKDE3)
+{
+    TmpEnv env ("KDE_FULL_SESSION", trueStr.c_str ());
+    CharacterWrapper sName (getSectionName ());
+
+    EXPECT_EQ (globalKDE3ProfileSection, sName);
+}
+
+TEST_F (CCSUtilProfileSelection, Fallback)
+{
+    CharacterWrapper sName (getSectionName ());
+
+    EXPECT_EQ (globalGeneralProfileSection, sName);
+}
+