ubuntu-touch-coreapps-reviewers team mailing list archive

[David Planella <david.planella@xxxxxxxxxx>]

Review: Needs Fixing

Hi Bartosz,

Thanks so much for looking into this! As you asked me for a review, I'll try to be thorough:

1. On the commit message for r199, there is no need to duplicate the Debian changelog on the commit message itself. Commit messages should be concise and to the point (e.g. "Updated Debian changelog"), in particular when the exact information can be seen in the commit (debian/changelog diff). This is not something that can be fixed now, or even an issue, it's just a recommendation for future commits.

2. In general, for each merge proposal, please only limit the changes related to the fix. This fix should only be a few lines and self-contained. I see many changes on this one that are not related, which make it difficult to review and make it more prone to introduce other issues.

3. I'm not sure why the change in r200 was introduced: as far as I know, there is no calculator app binary, and it makes the tests in Jenkins fail.

4. The rest of comments I've added inline in the merge proposal.

Diff comments:

> === modified file 'CMakeLists.txt'
> --- CMakeLists.txt	2015-04-14 16:42:56 +0000
> +++ CMakeLists.txt	2015-06-23 23:33:27 +0000
> @@ -27,13 +27,12 @@
>  
>  include(GNUInstallDirs)
>  
> -set(APP_NAME                        ubuntu-calculator-app)
> +set(APP_NAME                        calculator-app)

Why is it needed to change the app name?

>  set(APP_HARDCODE                    ubuntu-calculator-app)
>  set(AUTOPILOT_DIR                   ubuntu_calculator_app)
>  set(MAIN_QML                        ${APP_HARDCODE}.qml)
> -set(ICON                            "${APP_HARDCODE}.png")
> +set(ICON_FILE                       "${APP_HARDCODE}.png")
>  set(DESKTOP_FILE                    "${APP_HARDCODE}.desktop")
> -set(AUTOPILOT_DIR                   ubuntu_calculator_app)

Why is it needed to remove the AUTOPILOT_DIR variable? Even if it's necessary to remove it, please do it as a separate merge proposal. As far as I can tell, this is not related to any of the bugs targetted in the merge proposal.

>  set(APP_VERSION                     2.0)
>  
>  if(CLICK_MODE)
> @@ -41,8 +40,7 @@
>    set(UBUNTU-CALCULATOR-APP_DIR    "${CMAKE_INSTALL_DATADIR}/qml")
>  
>    set(QT_IMPORTS_DIR "${CMAKE_INSTALL_LIBDIR}")
> -  set(EXEC "qmlscene $@ ${UBUNTU-CALCULATOR-APP_DIR}/${MAIN_QML}")
> -  set(MODULE_PATH ${QT_IMPORTS_DIR})
> +  set(EXEC "qmlscene -qt5 $@ ${UBUNTU-CALCULATOR-APP_DIR}/${MAIN_QML}")
>    if(NOT BZR_REVNO)
>      execute_process(
>        COMMAND bzr revno
> @@ -76,8 +74,14 @@
>    configure_file(${UBUNTU_MANIFEST_PATH} ${CMAKE_CURRENT_BINARY_DIR}/manifest.json)
>    install(FILES ${CMAKE_CURRENT_BINARY_DIR}/manifest.json DESTINATION ${CMAKE_INSTALL_PREFIX})
>    install(FILES "${APP_HARDCODE}.apparmor" DESTINATION ${CMAKE_INSTALL_PREFIX})
> +  set(ICON ${ICON_FILE})
>  else(CLICK_MODE)
> -  set(EXEC "qmlscene $@ -I ${MODULE_PATH}  ${CMAKE_INSTALL_PREFIX}/${UBUNTU-CALCULATOR-APP_DIR}/${MAIN_QML}")
> +  set(ICON ${CMAKE_INSTALL_PREFIX}/${UBUNTU-CALCULATOR-APP_DIR}/${ICON_FILE})
> +  set(EXEC ${APP_NAME})
> +  configure_file(app/${APP_NAME}.in

Why is a separate app/${APP_NAME}.in file required? Can't we just set EXEC in the same way we do for the click package and just have the .desktop file start the app? In any case, if we end up using a separate file, this ubuntu-calculator-app.in file needs to be added to the source code. Right now it's missing, so it will not work.

> +    ${CMAKE_CURRENT_BINARY_DIR}/${APP_NAME})
> +  install(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/${APP_NAME}
> +    DESTINATION ${CMAKE_INSTALL_BINDIR})

Depending on the outcome of the comment above, this might not be needed.

>  endif(CLICK_MODE)
>  
>  file(GLOB_RECURSE I18N_SRC_FILES
> 
> === modified file 'app/CMakeLists.txt'
> --- app/CMakeLists.txt	2015-04-14 16:42:56 +0000
> +++ app/CMakeLists.txt	2015-06-23 23:33:27 +0000
> @@ -3,12 +3,8 @@
>    add_custom_target(ubuntu-calculator-app_QMlFiles ALL SOURCES ${QML_JS_FILES})
>  endif(NOT "${CMAKE_CURRENT_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_BINARY_DIR}")
>  
> -if(CLICK_MODE)
> -  set(ICON ${ICON})
> -  install(FILES graphics/${ICON} DESTINATION ${CMAKE_INSTALL_PREFIX})
> -endif(CLICK_MODE)
> -
> -install(FILES ${MAIN_QML} DESTINATION ${UBUNTU-CALCULATOR-APP_DIR})
> +install(FILES graphics/${ICON_FILE} DESTINATION ${CMAKE_INSTALL_PREFIX}/${UBUNTU-CALCULATOR-APP_DIR})
> +install(FILES ${MAIN_QML} DESTINATION ${CMAKE_INSTALL_PREFIX}/${UBUNTU-CALCULATOR-APP_DIR})
>  
>  add_subdirectory(engine)
>  add_subdirectory(graphics)
> 
> === modified file 'app/graphics/CMakeLists.txt'
> --- app/graphics/CMakeLists.txt	2015-03-11 09:13:18 +0000
> +++ app/graphics/CMakeLists.txt	2015-06-23 23:33:27 +0000
> @@ -1,4 +1,7 @@
> -file(GLOB GRAPHICS_FILES *.svg)
> +set(GRAPHICS_FILES 
> +  select_none.svg  
> +  select.svg
> +)
>  

This is not related to this bug. If there is any cleanup required, please do it in a separate merge proposal.

>  install(FILES ${GRAPHICS_FILES} DESTINATION ${UBUNTU-CALCULATOR-APP_DIR}/graphics)
>  
> 
> === removed file 'app/graphics/toolbarIcon@xxxxx'
> Binary files app/graphics/toolbarIcon@xxxxx	2014-11-10 09:28:27 +0000 and app/graphics/toolbarIcon@xxxxx	1970-01-01 00:00:00 +0000 differ

This is not related to this bug. If there is any cleanup required, please do it in a separate merge proposal.

> === modified file 'debian/changelog'
> --- debian/changelog	2014-12-11 11:09:09 +0000
> +++ debian/changelog	2015-06-23 23:33:27 +0000
> @@ -1,3 +1,45 @@
> +ubuntu-calculator-app (2.0.197) vivid; urgency=medium
> +
> +  [ Bartosz Kosiorek ]
> +  * Do not allow swiping in Landscape mode. 
> +    Fixes: https://bugs.launchpad.net/bugs/1466635.
> +  * Fix keyboard shortcuts (eg. Ctrl+C). 
> +    Fixes: https://bugs.launchpad.net/bugs/1466634.
> +  * Add additional comments to logaritm and modulo 
> +    to avoid confusion during translation. 
> +    Fixes: https://bugs.launchpad.net/bugs/1465297.
> +  * Fix display issue. 
> +    Fixes: https://bugs.launchpad.net/bugs/1465996.
> +
> +  [ Nicholas Skaggs ]
> +  * Fix testability for screen components.
> +
> +  [ Niklas Wenzel ]
> +  * Use longer swipe in autopilot tests for revealing the scientific keyboard. 
> +    Fixes: https://bugs.launchpad.net/bugs/1464571.
> +  * Fix typing special characters on hardware keyboards. 
> +    Fixes: https://bugs.launchpad.net/bugs/1449037.
> +
> +  [ Victor Thompson ]
> +  * Change favorites translatable string. 
> +    Fixes: https://bugs.launchpad.net/bugs/1464463.
> +
> + -- Bartosz Kosiorek <gang65@xxxxxxxxxxxxxx>  Fri, 24 Jul 2015 12:00:05 +0100
> +
> +ubuntu-calculator-app (2.0.182) vivid; urgency=medium
> +  * Add haptic feedback to buttons
> +  * Update math.js library. http://pad.lv/1439846
> +  * Enable 'bug number' support to fix floating point math. 
> +    http://pad.lv/1445152
> +  * Modify splash screen. http://pad.lv/1377638
> +  * Fix keyboard focus and disappearing cursor bug. http://pad.lv/1431548
> +  * Fix directory layout for AP tests. http://pad.lv/1443680
> +  * Modified layout to fit design
> +  * Fix issue with building which corrupted translations
> +  * Updated translations
> +
> + -- Bartosz Kosiorek <gang65@xxxxxxxxxxxxxx>  Thu, 15 Jun 2015 12:00:05 +0100
> +
>  ubuntu-calculator-app (0.1) vivid; urgency=low
>  
>    * Initial release
> 


-- 
https://code.launchpad.net/~gang65/ubuntu-calculator-app/ubuntu-calculator-app-desktop-install-fix/+merge/262799
Your team Ubuntu Calculator Developers is subscribed to branch lp:ubuntu-calculator-app.