Merge lp:~compiz-team/compiz/compiz.fix_1067246.1 into lp:compiz/0.9.10

[Sam Spilsbury]

Remove redundant src/logmessage/include/core/logmessage.h

(LP: #1067246)

Remove redundant src/logmessage/include/core/logmessage.h

(LP: #1067246)

[Micheal Hsu (mkhu)]

there is a lot of inconsistency in src/:

1. headers. where should we put public headers?
   some headers are in $(top_srcdir)/include, : e.g. plugin.h, point.h
   some are in $(top_srcdir)/src/<sub_directory>/include/: e.g. string.h
   my proposal: put all public headers in $(top_srcdir)/include
2. .cpp files. where should we put source files?
   some .cpp files are in $(top_srcdir)/src : e.g. plugin.cpp
   some .cpp files are in $(top_srcdir)/<sub_directory>/src/: e.g. point.cpp
   my proposal: put all .cpp files in $(top_srcdir)/src/
3. tests. where should we put tests?
   some tests are in $(top_srcdir)/src/<sub_directory>/tests: e.g. plugin, point
   some tests are in $(top_srcdir)/tests/
   my proposal: put all tests in $(top_srcdir)/src/tests
4. tests naming inconsistency:
   e.g. compiz_option_test, option.cpp for option/tests
        compiz_plugin_test, test-plugin.cpp for plugin/tests
        compiz_test_outputdevices, test_outputdevices.cpp for tests/
   my proposal: all test executables should be "compiz_test_xxxx"
                all test files should be "test-xxxx"

[Micheal Hsu (mkhu)]

I' like to know your opinion

[Sam Spilsbury (smspillaz)]

On Mon, May 20, 2013 at 11:16 AM, Hu Kang <email address hidden> wrote:
> there is a lot of inconsistency in src/:

You'd be right. There was a half-finished effort to split the compiz
core code into "modules" - although these days that model doesn't
really make sense.

>
> 1. headers. where should we put public headers?
> some headers are in $(top_srcdir)/include, : e.g. plugin.h, point.h
> some are in $(top_srcdir)/src/<sub_directory>/include/: e.g. string.h
> my proposal: put all public headers in $(top_srcdir)/include

Lets put all the public headers in include/.

> 2. .cpp files. where should we put source files?
> some .cpp files are in $(top_srcdir)/src : e.g. plugin.cpp
> some .cpp files are in $(top_srcdir)/<sub_directory>/src/: e.g. point.cpp
> my proposal: put all .cpp files in $(top_srcdir)/src/

They can probably all go in src/ - as long as they are still compiled
into independent static libraries.

> 3. tests. where should we put tests?
> some tests are in $(top_srcdir)/src/<sub_directory>/tests: e.g. plugin, point
> some tests are in $(top_srcdir)/tests/
> my proposal: put all tests in $(top_srcdir)/src/tests

Likewise.

The only places where (2) and (3) cannot apply is in the plugins,
because CompizPlugin.cmake will take all the files in src/*.cpp and
compile that into the plugin, which isn't really what we want for
testing purposes.

> 4. tests naming inconsistency:
> e.g. compiz_option_test, option.cpp for option/tests
> compiz_plugin_test, test-plugin.cpp for plugin/tests
> compiz_test_outputdevices, test_outputdevices.cpp for tests/
> my proposal: all test executables should be "compiz_test_xxxx"
> all test files should be "test-xxxx"
>

I think compiz_test_xxx for executables and compiz_test_xxx.cpp for
the test files really.

> --
> https://code.launchpad.net/~stevenhooke11/compiz/compiz.fix_1067246/+merge/164665
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~stevenhooke11/compiz/compiz.fix_1067246 into lp:compiz.

--
Sam Spilsbury

[Micheal Hsu (mkhu)]

Sam. thanks for your comment

some corrections for my previous comment:
(I am talking about files in $(top_srcdir)/src/)
////////////////////
2. .cpp files. where should we put source files?
   some .cpp files are in $(top_srcdir)/src : e.g. plugin.cpp
   some .cpp files are in $(top_srcdir)/src/<sub_directory>/src/: e.g. point.cpp
   my proposal: put all .cpp files in $(top_srcdir)/src/
3. tests. where should we put tests?
   some tests are in $(top_srcdir)/src/<sub_directory>/tests: e.g. point
   some tests are in $(top_srcdir)/src/tests/
   my proposal: put all tests in $(top_srcdir)/src/tests/
////////////////////

I'll push a new branch with 1, 2, 3, 4, applied to $(top_srcdir)/src/.

[MC Return (mc-return)]

Hu, great to see that you are helping with the cleanup/inconsistency-elimination !
This was one point on my TODO list. :) Thanks for taking care of it...

+1

[Sam Spilsbury (smspillaz)]

Just before this can be merged in, the install () command which installs whatever is in PUBLIC_HEADERS needs to be removed to (and that would effectively make the SET_TARGET_PROPERTIES call setting PUBLIC_HEADERS redundant in that directory too).

Right now, there are two install () statements that cover the same file.

(Sorry for the late review on that one, today's been kind of hectic, and the next week is going to be a bit all over the place too).

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

Hey Hu.

Were you going to get to this? I should be able to make the required changes myself - I just don't want to accidentally step on you! :)

[Micheal Hsu (mkhu)]

sorry, i've been a little busy recently. i do have a branch, but haven't tested throughly.
so i guess i won't have time to do that.

[MC Return (mc-return)]

> sorry, i've been a little busy recently. i do have a branch, but haven't
> tested throughly.
> so i guess i won't have time to do that.

Please take your time, but do not throw away work you've already done...
We will simply wait :)

[Sam Spilsbury (smspillaz)]

review: Approve

[Sam Spilsbury (smspillaz)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
Approved revid is not set in launchpad. This is most likely a launchpad issue and re-approve should fix it. There is also a chance (although a very small one) this is a permission problem of the ps-jenkins bot.
http://jenkins.qa.ubuntu.com/job/compiz-autolanding/127/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-autolanding/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-autolanding/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-autolanding/39

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3756
http://jenkins.qa.ubuntu.com/job/compiz-ci/265/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-ci/78
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-ci/78
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-ci/78

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/265/rebuild

review: Approve (continuous-integration)

=== modified file 'include/core/CMakeLists.txt'
--- include/core/CMakeLists.txt	2013-01-03 16:05:26 +0000
+++ include/core/CMakeLists.txt	2013-07-15 21:05:55 +0000
@@ -6,6 +6,7 @@
     countedlist.h
     global.h
     icon.h
+    logmessage.h
     match.h
     modifierhandler.h
     option.h

=== modified file 'src/logmessage/CMakeLists.txt'
--- src/logmessage/CMakeLists.txt	2012-01-23 17:46:28 +0000
+++ src/logmessage/CMakeLists.txt	2013-07-15 21:05:55 +0000
@@ -1,15 +1,9 @@
-
 INCLUDE_DIRECTORIES(  
-  ${CMAKE_CURRENT_SOURCE_DIR}/include
-  
   ${compiz_SOURCE_DIR}/include
     
   ${Boost_INCLUDE_DIRS}
 )
 
-SET( PUBLIC_HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/include/core/logmessage.h )
-SET( PRIVATE_HEADERS )
-
 SET( SRCS ${CMAKE_CURRENT_SOURCE_DIR}/src/logmessage.cpp )
 
 ADD_LIBRARY( 
@@ -24,10 +18,3 @@
 IF (COMPIZ_BUILD_TESTING)
 ADD_SUBDIRECTORY( ${CMAKE_CURRENT_SOURCE_DIR}/tests )
 ENDIF (COMPIZ_BUILD_TESTING)
-
-SET_TARGET_PROPERTIES(
-  compiz_logmessage PROPERTIES
-  PUBLIC_HEADER "${PUBLIC_HEADERS}"
-)
-
-install (FILES ${PUBLIC_HEADERS} DESTINATION ${COMPIZ_CORE_INCLUDE_DIR})

=== removed directory 'src/logmessage/include'
=== removed directory 'src/logmessage/include/core'
=== removed file 'src/logmessage/include/core/logmessage.h'
--- src/logmessage/include/core/logmessage.h	2011-10-31 13:51:00 +0000
+++ src/logmessage/include/core/logmessage.h	1970-01-01 00:00:00 +0000
@@ -1,51 +0,0 @@
-/*
- * Copyright © 2007 Novell, Inc.
- *
- * 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
- * Novell, Inc. not be used in advertising or publicity pertaining to
- * distribution of the software without specific, written prior permission.
- * Novell, Inc. makes no representations about the suitability of this
- * software for any purpose. It is provided "as is" without express or
- * implied warranty.
- *
- * NOVELL, INC. DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
- * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN
- * NO EVENT SHALL NOVELL, INC. 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.
- *
- * Author: David Reveman <[e-mail]>
- */
-
-#ifndef _COMPIZ_LOGMESSAGE_H
-#define _COMPIZ_LOGMESSAGE_H
-
-typedef enum {
-    CompLogLevelFatal = 0,
-    CompLogLevelError,
-    CompLogLevelWarn,
-    CompLogLevelInfo,
-    CompLogLevelDebug
-} CompLogLevel;
-
-void
-logMessage (const char   *componentName,
-	    CompLogLevel level,
-	    const char   *message);
-
-void
-compLogMessage (const char   *componentName,
-		CompLogLevel level,
-		const char   *format,
-		...);
-
-const char *
-logLevelToString (CompLogLevel level);
-
-#endif