ubuntu-touch-coreapps-reviewers team mailing list archive

[Bartosz Kosiorek <gang65@xxxxxxxxxxxxxx>]

Review: Needs Fixing

Please take a look at inline comments

Diff comments:

> === modified file 'CMakeLists.txt'
> --- CMakeLists.txt	2015-02-26 21:16:54 +0000
> +++ CMakeLists.txt	2015-08-08 18:46:09 +0000
> @@ -109,6 +109,13 @@
>  add_subdirectory(po)
>  add_subdirectory(tests)
>  
> +# make the qml files visible on qtcreator
> +file(GLOB QML_JS_FILES

Wrong variable name. It should be README_FILES.

> +     RELATIVE ${CMAKE_CURRENT_SOURCE_DIR}
> +     *.qml *.js README* *.txt)

In the main directory there is no *.js and *qml files. Also I think it was not your intention to expose CMakeLists.txt files (as I understand only README)

My proposal is explicetely set all README files which you would like to add do QTcreator. 
So it will be clearly visible for all, what kind of files are truely added to project.

Example:

set(README_FILES
    README
    README.autopilot
    README.developers
    README.translations
    README.unittest    
)

Benefits of explicetely set instead of globbing:
1. You could take a look at CMakeLists.txt to check what exactly is imported into project
2. Performance improvement
3. You are sure that nothing else outside of this list will be included

> +
> +add_custom_target(com_ubuntu_clock_QMLFiles DEPENDS ubuntu-clock-app.qml SOURCES ${QML_JS_FILES})
> +
>  add_custom_target("autopilot" chmod +x ${CMAKE_SOURCE_DIR}/tests/autopilot/run
>                      COMMAND ${CMAKE_SOURCE_DIR}/tests/autopilot/run
>                      DEPENDS timezone timezone-qmldir alarmsettings alarmsettings-qmldir datetime datetime-qmldir geolocation geolocation-qmldir


-- 
https://code.launchpad.net/~vthompson/ubuntu-clock-app/clock-show-all-READMEs/+merge/267439
Your team Ubuntu Clock Developers is subscribed to branch lp:ubuntu-clock-app.