Merge lp:~renatofilho/compiz/compiz-lp1033531 into lp:compiz/0.9.8

[Renato Araujo Oliveira Filho]

Implemented single click exit from expo screen. (LP: #1033531)

Implemented single click exit from expo screen.

[Sam Spilsbury (smspillaz)]

Looks good, and as I thought, this was pretty simple anyways.

+#define DND_THRESHOLD 5

Can you make this a const unsigned int? eg const unsigned int DND_THRESHOLD 5 . Makes things more debuggable.

26 + ((abs (prevClickPoint.x () - event->xbutton.x) <= DND_THRESHOLD) &&
27 + (abs (prevClickPoint.y () - event->xbutton.y) <= DND_THRESHOLD)))

These need to be aligned with the initial bracked after the else, and not the else itself, eg,

else (foo () ||
      bar ())

not

else (foo () ||
bar ())

Major point:
This needs tests before it can go in. It should be fairly easy to get this code under test, since there's a single abstraction, eg

namespace compiz
{
namespace expo
{
bool clickMovementInThreshold (const CompPoint &previousPoint,
                               const CompPoint &currentPoint)
{
    CompPoint distance = currentPoint - previousPoint;
    distance.set (abs (distance.x ()), abs (distance.y ()));

    if (distance.x () < DND_THRESHOLD &&
        distance.y () < DND_THRESHOLD)
        return true;

    return false;
}
}
}

You can then put it in a different file and test it that way. There are examples around the codebase (eg, in decor, composite, wall) on how to do this. Testing == Awesome.

review: Needs Fixing

[lp:~renatofilho/compiz/compiz-lp1033531]

3308. By Renato Araujo Oliveira Filho on 2012-08-09

Declared DND_THRESHOLD as "static const unsigned int" instead of "define";

3309. By Renato Araujo Oliveira Filho on 2012-08-09

Created new file with ¨clickMovementInThreshold¨ function.
Created unit test for function ¨clickMovementInThreshold¨.

[Renato Araujo Oliveira Filho (renatofilho)]

@Sam Spilsbury (smspillaz)

All comments fixed.

Thanks

[Sam Spilsbury (smspillaz)]

Looks great. Thanks!

review: Approve

[Daniel van Vugt (vanvugt)]

Works well. Just a couple of small cleanups:

clickMovementInThreshold(const CompPoint &previousPoint, int x, int y)
^ This is inconsistent. It should either accept two CompPoints, or four ints. Not a mixture of both.

This:
    static const unsigned int DND_THRESHOLD = 5;
    if ((abs (previousPoint.x () - x) <= static_cast<int>(DND_THRESHOLD)
should just be:
    static const int DND_THRESHOLD = 5;
    if ((abs (previousPoint.x () - x) <= DND_THRESHOLD

Ideally CMakeLists should use lower case for reserved words like the docs do:
http://www.cmake.org/cmake/help/v2.8.8/cmake.html

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Also, to get the alignment right in all environments, make sure you use spaces on line 138, to align with line 137. Then tabs when actually indenting a block.

[Renato Araujo Oliveira Filho (renatofilho)]

> Also, to get the alignment right in all environments, make sure you use spaces
> on line 138, to align with line 137. Then tabs when actually indenting a
> block.

I might be wrong but what I understand from this code style is that I should use 'tab' for 2 levels of indentation.

ref: http://wiki.compiz.org/Development/CodingStyle

[Renato Araujo Oliveira Filho (renatofilho)]

> > Also, to get the alignment right in all environments, make sure you use
> spaces
> > on line 138, to align with line 137. Then tabs when actually indenting a
> > block.
>
> I might be wrong but what I understand from this code style is that I should
> use 'tab' for 2 levels of indentation.
>
> ref: http://wiki.compiz.org/Development/CodingStyle

And this was done automatically by vim because of the flag "set cindent" which they recommend to use in the vimrc.

[lp:~renatofilho/compiz/compiz-lp1033531]

3310. By Renato Araujo Oliveira Filho on 2012-08-10

Used lower case for reserved words in cmake files.

3311. By Renato Araujo Oliveira Filho on 2012-08-10

Changed "clickMovementInThreshold" signature to use int instead of CompPoint.

[Renato Araujo Oliveira Filho (renatofilho)]

> Works well. Just a couple of small cleanups:
>
> clickMovementInThreshold(const CompPoint &previousPoint, int x, int y)
> ^ This is inconsistent. It should either accept two CompPoints, or four ints.
> Not a mixture of both.
>
> This:
> static const unsigned int DND_THRESHOLD = 5;
> if ((abs (previousPoint.x () - x) <= static_cast<int>(DND_THRESHOLD)
> should just be:
> static const int DND_THRESHOLD = 5;
> if ((abs (previousPoint.x () - x) <= DND_THRESHOLD

Fixed on revision 3311

>
> Ideally CMakeLists should use lower case for reserved words like the docs do:
> http://www.cmake.org/cmake/help/v2.8.8/cmake.html

Fixed on revision 3310

[Daniel van Vugt (vanvugt)]

OK, thanks. Approved.

I noticed a bug with wobbly and using the "single click" sometimes got my windows stuck in a permanently deformed state. But I can't reproduce that bug now... !?

P.S. clickMovementInThreshold really should be a single line:
    return (abs (previousX - currentX) <= DND_THRESHOLD) &&
           (abs (previousY - currentY) <= DND_THRESHOLD);
And yes, that would mean libcompiz_expo_click_threshold.a would be a library containing only a single line function :)

P.P.S Namespaces should not be used repeatedly in a file:
    EXPECT_TRUE(compiz::expo::clickMovementInThreshold (10, 10, 13, 13));
    EXPECT_FALSE(compiz::expo::clickMovementInThreshold (10, 10, 1, 1));
Instead you should:
    using namespace compiz::expo;
    ...
    EXPECT_TRUE(clickMovementInThreshold (10, 10, 13, 13));
    EXPECT_FALSE(clickMovementInThreshold (10, 10, 1, 1));

review: Approve

[Daniel van Vugt (vanvugt)]

P.P.P.S. If text needs to be aligned, you should always use the same number of tabs (if any) as the previous line. Because you can't always assumed the source code will be viewed in the same editor as yours with the same settings. So ensuring the tabs at the start of the lines are identical guarantees correctly aligned text in all environments.

=== modified file 'plugins/expo/CMakeLists.txt'
--- plugins/expo/CMakeLists.txt	2012-05-16 17:40:58 +0000
+++ plugins/expo/CMakeLists.txt	2012-08-10 12:28:19 +0000
@@ -3,5 +3,9 @@
 include (CompizPlugin)
 
 if (OPENGL_GLU_FOUND)
-    compiz_plugin (expo PLUGINDEPS composite opengl LIBRARIES ${OPENGL_glu_LIBRARY} INCDIRS ${OPENGL_INCLUDE_DIR})
+    add_subdirectory (src/click_threshold)
+    include_directories (src/click_threshold/include)
+
+    compiz_plugin (expo PLUGINDEPS composite opengl LIBRARIES ${OPENGL_glu_LIBRARY} compiz_expo_click_threshold INCDIRS ${OPENGL_INCLUDE_DIR})
 endif (OPENGL_GLU_FOUND)
+

=== added directory 'plugins/expo/src/click_threshold'
=== added file 'plugins/expo/src/click_threshold/CMakeLists.txt'
--- plugins/expo/src/click_threshold/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ plugins/expo/src/click_threshold/CMakeLists.txt	2012-08-10 12:28:19 +0000
@@ -0,0 +1,33 @@
+include_directories (
+  ${CMAKE_CURRENT_SOURCE_DIR}/include
+  ${CMAKE_CURRENT_SOURCE_DIR}/src
+  ${Boost_INCLUDE_DIRS}
+  ${GLIBMM_INCLUDE_DIRS}
+)
+
+link_directories (${GLIBMM_LIBRARY_DIRS} ${COMPIZ_LIBRARY_DIRS})
+
+set (
+  PRIVATE_HEADERS
+  ${CMAKE_CURRENT_SOURCE_DIR}/include/click-threshold.h
+)
+
+set (
+  SRCS
+  ${CMAKE_CURRENT_SOURCE_DIR}/src/click-threshold.cpp
+)
+
+add_library (
+  compiz_expo_click_threshold STATIC
+  ${SRCS}
+  ${PRIVATE_HEADERS}
+)
+
+if (COMPIZ_BUILD_TESTING)
+  add_subdirectory ( ${CMAKE_CURRENT_SOURCE_DIR}/tests )
+endif (COMPIZ_BUILD_TESTING)
+
+target_link_libraries (
+  compiz_expo_click_threshold
+  compiz_point
+)

=== added directory 'plugins/expo/src/click_threshold/include'
=== added file 'plugins/expo/src/click_threshold/include/click-threshold.h'
--- plugins/expo/src/click_threshold/include/click-threshold.h	1970-01-01 00:00:00 +0000
+++ plugins/expo/src/click_threshold/include/click-threshold.h	2012-08-10 12:28:19 +0000
@@ -0,0 +1,38 @@
+/**
+ *
+ * Compiz Expo plugin
+ *
+ * click-threshold.h
+ *
+ * Copyright © 2012 Canonical Ltd.
+ *
+ * Authors:
+ * Renato Araujo Oliviera Filho <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program 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 General Public License for more details.
+ *
+ **/
+
+#ifndef _COMPIZ_EXPO_CLICK_THRESHOLD_H
+#define _COMPIZ_EXPO_CLICK_THRESHOLD_H
+
+#include <core/point.h>
+
+namespace compiz
+{
+    namespace expo
+    {
+	bool clickMovementInThreshold(int previousX, int previousY,
+				      int currentX, int currentY);
+    }
+}
+
+#endif

=== added directory 'plugins/expo/src/click_threshold/src'
=== added file 'plugins/expo/src/click_threshold/src/click-threshold.cpp'
--- plugins/expo/src/click_threshold/src/click-threshold.cpp	1970-01-01 00:00:00 +0000
+++ plugins/expo/src/click_threshold/src/click-threshold.cpp	2012-08-10 12:28:19 +0000
@@ -0,0 +1,39 @@
+/**
+ *
+ * Compiz Expo plugin
+ *
+ * click-threshold.cpp
+ *
+ * Copyright © 2012 Canonical Ltd.
+ *
+ * Authors:
+ * Renato Araujo Oliviera Filho <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program 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 General Public License for more details.
+ *
+ **/
+
+
+#include "click-threshold.h"
+#include <stdlib.h>
+
+static const int DND_THRESHOLD = 5;
+
+bool
+compiz::expo::clickMovementInThreshold(int previousX, int previousY,
+				       int currentX, int currentY)
+{
+    if ((abs (previousX - currentX) <= DND_THRESHOLD) &&
+	(abs (previousY - currentY) <= DND_THRESHOLD))
+	return true;
+    else
+	return false;
+}

=== added directory 'plugins/expo/src/click_threshold/tests'
=== added file 'plugins/expo/src/click_threshold/tests/CMakeLists.txt'
--- plugins/expo/src/click_threshold/tests/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ plugins/expo/src/click_threshold/tests/CMakeLists.txt	2012-08-10 12:28:19 +0000
@@ -0,0 +1,19 @@
+if (NOT GTEST_FOUND)
+  message ("Google Test not found - cannot build tests!")
+  set (COMPIZ_BUILD_TESTING OFF)
+endif (NOT GTEST_FOUND)
+
+include_directories (${GTEST_INCLUDE_DIRS})
+
+link_directories (${COMPIZ_LIBRARY_DIRS})
+
+add_executable (compiz_test_expo_click_threshold
+		${CMAKE_CURRENT_SOURCE_DIR}/test-expo-click-threshold.cpp)
+
+target_link_libraries (compiz_test_expo_click_threshold
+		       compiz_expo_click_threshold
+		       ${GTEST_BOTH_LIBRARIES}
+		       ${CMAKE_THREAD_LIBS_INIT} # Link in pthread.
+                       )
+
+compiz_discover_tests (compiz_test_expo_click_threshold COVERAGE compiz_expo_click_threshold)

=== added file 'plugins/expo/src/click_threshold/tests/test-expo-click-threshold.cpp'
--- plugins/expo/src/click_threshold/tests/test-expo-click-threshold.cpp	1970-01-01 00:00:00 +0000
+++ plugins/expo/src/click_threshold/tests/test-expo-click-threshold.cpp	2012-08-10 12:28:19 +0000
@@ -0,0 +1,79 @@
+/*
+ * Copyright © 2012 Canonical Ltd.
+ *
+ * Permission to use, copy, modify, distribute, and sell this software
+ * and its documentation for any purpose is hereby granted without
+ * fee, provided that the above copyright notice appear in all copies
+ * and that both that copyright notice and this permission notice
+ * appear in supporting documentation, and that the name of
+ * Canonical Ltd. not be used in advertising or publicity pertaining to
+ * distribution of the software without specific, written prior permission.
+ * Canonical Ltd. makes no representations about the suitability of this
+ * software for any purpose. It is provided "as is" without express or
+ * implied warranty.
+ *
+ * CANONICAL, LTD. DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN
+ * NO EVENT SHALL CANONICAL, LTD. BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
+ * OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT,
+ * NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION
+ * WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ *
+ * Authored by: Renato Araujo Oliveira Filho <[e-mail]>
+ */
+
+
+#include <gtest/gtest.h>
+
+#include "click-threshold.h"
+
+class ExpoClickThresholdTest :
+    public ::testing::Test
+{
+};
+
+TEST(ExpoClickThresholdTest, TestNotMove)
+{
+    EXPECT_TRUE(compiz::expo::clickMovementInThreshold (10, 10, 10, 10));
+}
+
+TEST(ExpoClickThresholdTest, TestMoveNearLeft)
+{
+    EXPECT_TRUE(compiz::expo::clickMovementInThreshold (10, 10, 8, 8));
+}
+
+TEST(ExpoClickThresholdTest, TestMoveNearRight)
+{
+    EXPECT_TRUE(compiz::expo::clickMovementInThreshold (10, 10, 13, 13));
+}
+
+TEST(ExpoClickThresholdTest, TestMoveFarLeft)
+{
+    EXPECT_FALSE(compiz::expo::clickMovementInThreshold (10, 10, 1, 1));
+}
+
+TEST(ExpoClickThresholdTest, TestMoveFarRight)
+{
+    EXPECT_FALSE(compiz::expo::clickMovementInThreshold (10, 10, 30, 30));
+}
+
+TEST(ExpoClickThresholdTest, TestMoveNearX)
+{
+    EXPECT_TRUE(compiz::expo::clickMovementInThreshold (10, 10, 13, 10));
+}
+
+TEST(ExpoClickThresholdTest, TestMoveNearY)
+{
+    EXPECT_TRUE(compiz::expo::clickMovementInThreshold (10, 10, 10, 13));
+}
+
+TEST(ExpoClickThresholdTest, TestMoveFarX)
+{
+    EXPECT_FALSE(compiz::expo::clickMovementInThreshold (10, 10, 30, 10));
+}
+
+TEST(ExpoClickThresholdTest, TestMoveFarY)
+{
+    EXPECT_FALSE(compiz::expo::clickMovementInThreshold (10, 10, 10, 30));
+}

=== modified file 'plugins/expo/src/expo.cpp'
--- plugins/expo/src/expo.cpp	2012-05-28 06:08:02 +0000
+++ plugins/expo/src/expo.cpp	2012-08-10 12:28:19 +0000
@@ -24,6 +24,7 @@
  **/
 
 #include "expo.h"
+#include "click-threshold.h"
 #include <math.h>
 #include <GL/glu.h>
 #include <X11/cursorfont.h>
@@ -354,6 +355,7 @@
 		doubleClick = false;
 	    }
 	    cScreen->damageScreen ();
+	    prevClickPoint = CompPoint(event->xbutton.x, event->xbutton.y);
 	}
 	break;
 
@@ -367,7 +369,11 @@
 		clickTime   = 0;
 		doubleClick = false;
 	    }
-	    else if (doubleClick)
+	    else if (doubleClick ||
+		     compiz::expo::clickMovementInThreshold(prevClickPoint.x (),
+							    prevClickPoint.y (),
+							    event->xbutton.x,
+							    event->xbutton.y))
 	    {
 		CompAction& action = optionGetExpoKey ();
 

=== modified file 'plugins/expo/src/expo.h'
--- plugins/expo/src/expo.h	2012-05-28 05:35:38 +0000
+++ plugins/expo/src/expo.h	2012-08-10 12:28:19 +0000
@@ -86,6 +86,7 @@
 
 	CompPoint prevCursor;
 	CompPoint newCursor;
+	CompPoint prevClickPoint;
 
 	CompPoint origVp;
 	CompPoint selectedVp;