ubuntu-touch-coreapps-reviewers team mailing list archive

[Nekhelesh Ramananthan <krnekhelesh@xxxxxxxxx>]

Nekhelesh Ramananthan has proposed merging lp:~nik90/ubuntu-clock-app/migrate-stopwatch-utils-c++ into lp:ubuntu-clock-app.

Commit message:
Converted StopwatchUtils.qml functions to C++ functions to shave loading times and improve performance by a tiny bit.

Requested reviews:
  Ubuntu Clock Developers (ubuntu-clock-dev)

For more details, see:
https://code.launchpad.net/~nik90/ubuntu-clock-app/migrate-stopwatch-utils-c++/+merge/269051

This MP converts the StopwatchUtils.qml functions to C++ functions for the following reasons,

When I ran stopwatch using Qt Profiler, I noticed that StopwatchUtils{} takes around 60 ms to load, and each function call within takes an average of 200 microsecond. On converting this to c++, the load time reduces to 500 microseconds, so from 60ms->500microsecond! And each function call takes half the time (100 microsecond)

While the performance shaving are quite small, it raises the question as to why not do this change? I see no disadvantages. Nothing in the qml side needs to be changed, unit tests pass. Regression free!
-- 
Your team Ubuntu Clock Developers is requested to review the proposed merge of lp:~nik90/ubuntu-clock-app/migrate-stopwatch-utils-c++ into lp:ubuntu-clock-app.

=== modified file 'app/stopwatch/CMakeLists.txt'
--- app/stopwatch/CMakeLists.txt	2015-08-09 20:59:10 +0000
+++ app/stopwatch/CMakeLists.txt	2015-08-25 12:34:12 +0000
@@ -2,7 +2,6 @@
     LapListView.qml
     StopwatchFace.qml
     StopwatchPage.qml
-    StopwatchUtils.qml
 )
 
 # make the files visible in the qtcreator tree

=== modified file 'app/stopwatch/LapListView.qml'
--- app/stopwatch/LapListView.qml	2015-08-20 19:59:21 +0000
+++ app/stopwatch/LapListView.qml	2015-08-25 12:34:12 +0000
@@ -19,6 +19,7 @@
 import QtQuick 2.4
 import QtQuick.Layouts 1.1
 import Ubuntu.Components 1.2
+import Stopwatch 1.0
 
 ListView {
     id: lapListView

=== modified file 'app/stopwatch/StopwatchFace.qml'
--- app/stopwatch/StopwatchFace.qml	2015-08-17 20:27:12 +0000
+++ app/stopwatch/StopwatchFace.qml	2015-08-25 12:34:12 +0000
@@ -17,6 +17,7 @@
  */
 
 import QtQuick 2.4
+import Stopwatch 1.0
 import Ubuntu.Components 1.2
 import "../components"
 
@@ -46,7 +47,7 @@
         anchors.centerIn: parent
 
         Label {
-            text: stopwatchUtils.millisToTimeString(milliseconds, false, true)
+            text: stopwatchUtils.millisToTimeString(milliseconds, true)
             font.pixelSize: units.dp(34)
             color: UbuntuColors.midAubergine
         }

=== modified file 'app/stopwatch/StopwatchPage.qml'
--- app/stopwatch/StopwatchPage.qml	2015-08-18 13:13:32 +0000
+++ app/stopwatch/StopwatchPage.qml	2015-08-25 12:34:12 +0000
@@ -17,8 +17,8 @@
  */
 
 import QtQuick 2.4
+import Stopwatch 1.0
 import Ubuntu.Components 1.2
-import Stopwatch.LapHistory 1.0
 
 Item {
     id: _stopwatchPage

=== removed file 'app/stopwatch/StopwatchUtils.qml'
--- app/stopwatch/StopwatchUtils.qml	2015-08-17 20:27:12 +0000
+++ app/stopwatch/StopwatchUtils.qml	1970-01-01 00:00:00 +0000
@@ -1,73 +0,0 @@
-/*
- * Copyright (C) 2015 Canonical Ltd
- *
- * This file is part of Ubuntu Clock App
- *
- * Ubuntu Clock App is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 3 as
- * published by the Free Software Foundation.
- *
- * Ubuntu Clock App 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.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-import QtQuick 2.4
-import Ubuntu.Components 1.2
-
-/*
-  Qt Object containing a collection of useful stopwatch functions
-*/
-QtObject {
-    id: stopwatchUtils
-
-    // Function to return only the milliseconds
-    function millisToString(millis) {
-        return addZeroPrefix(millis % 1000, 3)
-    }
-
-    // Function to break down time (milliseconds) to hours, minutes and seconds
-    function millisToTimeString(millis, showMilliseconds, showHours) {
-        var hours, minutes, seconds
-
-        // Break down total time (milliseconds) to hours, minutes and seconds
-        seconds = Math.floor(millis / 1000) % 60;
-        minutes = Math.floor(millis / 1000 / 60) % 60
-        hours = Math.floor(millis / 1000 / 60 / 60)
-
-        // Build the time string
-        var timeString = ""
-
-        if (showHours) {
-            timeString += addZeroPrefix(hours, 2) + ":"
-        }
-
-        timeString += addZeroPrefix(minutes, 2) + ":"
-        timeString += addZeroPrefix(seconds, 2)
-
-        if (showMilliseconds) {
-            timeString += "." + millisToString(millis)
-        }
-
-        return timeString
-    }
-
-    // Function to add zero prefix if necessary.
-    function addZeroPrefix (str, totalLength) {
-        return ("00000" + str).slice(-totalLength)
-    }
-
-    // Function to return lap time as a string
-    function lapTimeToString(millis) {
-        var hours = hours = Math.floor(millis / 1000 / 60 / 60)
-        if (hours > 0) {
-            return millisToTimeString(millis, false, true)
-        } else {
-            return millisToTimeString(millis, false, false)
-        }
-    }
-}

=== modified file 'backend/CMakeLists.txt'
--- backend/CMakeLists.txt	2015-08-24 15:08:21 +0000
+++ backend/CMakeLists.txt	2015-08-25 12:34:12 +0000
@@ -41,9 +41,10 @@
 )
 
 set(
-    stopwatchlaphistory_SRCS
-    modules/Stopwatch/LapHistory/backend.cpp
-    modules/Stopwatch/LapHistory/history.cpp
+    stopwatch_SRCS
+    modules/Stopwatch/backend.cpp
+    modules/Stopwatch/history.cpp
+    modules/Stopwatch/utils.cpp
 )
 
 add_library(timezone MODULE
@@ -66,8 +67,8 @@
     ${clockutility_SRCS}
 )
 
-add_library(stopwatchlaphistory MODULE
-    ${stopwatchlaphistory_SRCS}
+add_library(stopwatch MODULE
+    ${stopwatch_SRCS}
 )
 
 set_target_properties(timezone PROPERTIES
@@ -90,8 +91,8 @@
     LIBRARY_OUTPUT_DIRECTORY Clock/Utility
 )
 
-set_target_properties(stopwatchlaphistory PROPERTIES
-    LIBRARY_OUTPUT_DIRECTORY Stopwatch/LapHistory
+set_target_properties(stopwatch PROPERTIES
+    LIBRARY_OUTPUT_DIRECTORY Stopwatch/
 )
 
 qt5_use_modules(datetime Gui Qml Quick)
@@ -99,7 +100,7 @@
 qt5_use_modules(alarmsettings Gui Qml Quick DBus)
 qt5_use_modules(geolocation Gui Qml Quick)
 qt5_use_modules(clockutility Qml)
-qt5_use_modules(stopwatchlaphistory Qml)
+qt5_use_modules(stopwatch Qml)
 
 # Copy qmldir file to build dir for running in QtCreator
 add_custom_target(timezone-qmldir ALL
@@ -127,8 +128,8 @@
     DEPENDS ${QMLFILES}
 )
 
-add_custom_target(stopwatchlaphistory-qmldir ALL
-    COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_SOURCE_DIR}/modules/Stopwatch/LapHistory/qmldir ${CMAKE_CURRENT_BINARY_DIR}/Stopwatch/LapHistory
+add_custom_target(stopwatch-qmldir ALL
+    COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_SOURCE_DIR}/modules/Stopwatch/qmldir ${CMAKE_CURRENT_BINARY_DIR}/Stopwatch
     DEPENDS ${QMLFILES}
 )
 
@@ -148,6 +149,6 @@
 install(TARGETS clockutility DESTINATION ${MODULE_PATH}/Clock/Utility/)
 install(FILES   modules/Clock/Utility/qmldir DESTINATION ${MODULE_PATH}/Clock/Utility/)
 
-install(TARGETS stopwatchlaphistory DESTINATION ${MODULE_PATH}/Stopwatch/LapHistory/)
-install(FILES   modules/Stopwatch/LapHistory/qmldir DESTINATION ${MODULE_PATH}/Stopwatch/LapHistory/)
+install(TARGETS stopwatch DESTINATION ${MODULE_PATH}/Stopwatch/)
+install(FILES   modules/Stopwatch/qmldir DESTINATION ${MODULE_PATH}/Stopwatch/)
 

=== removed directory 'backend/modules/Stopwatch/LapHistory'
=== renamed file 'backend/modules/Stopwatch/LapHistory/backend.cpp' => 'backend/modules/Stopwatch/backend.cpp'
--- backend/modules/Stopwatch/LapHistory/backend.cpp	2015-08-18 05:03:59 +0000
+++ backend/modules/Stopwatch/backend.cpp	2015-08-25 12:34:12 +0000
@@ -20,12 +20,14 @@
 #include <QtQml/QQmlContext>
 #include "backend.h"
 #include "history.h"
+#include "utils.h"
 
 void BackendPlugin::registerTypes(const char *uri)
 {
-    Q_ASSERT(uri == QLatin1String("Stopwatch.LapHistory"));
+    Q_ASSERT(uri == QLatin1String("Stopwatch"));
 
     qmlRegisterType<LapHistory>(uri, 1, 0, "LapHistory");
+    qmlRegisterType<StopwatchUtils>(uri, 1, 0, "StopwatchUtils");
 }
 
 void BackendPlugin::initializeEngine(QQmlEngine *engine, const char *uri)

=== renamed file 'backend/modules/Stopwatch/LapHistory/backend.h' => 'backend/modules/Stopwatch/backend.h'
=== renamed file 'backend/modules/Stopwatch/LapHistory/history.cpp' => 'backend/modules/Stopwatch/history.cpp'
=== renamed file 'backend/modules/Stopwatch/LapHistory/history.h' => 'backend/modules/Stopwatch/history.h'
=== renamed file 'backend/modules/Stopwatch/LapHistory/qmldir' => 'backend/modules/Stopwatch/qmldir'
--- backend/modules/Stopwatch/LapHistory/qmldir	2015-08-18 05:03:59 +0000
+++ backend/modules/Stopwatch/qmldir	2015-08-25 12:34:12 +0000
@@ -1,2 +1,2 @@
-module Stopwatch.LapHistory
-plugin stopwatchlaphistory
+module Stopwatch
+plugin stopwatch

=== added file 'backend/modules/Stopwatch/utils.cpp'
--- backend/modules/Stopwatch/utils.cpp	1970-01-01 00:00:00 +0000
+++ backend/modules/Stopwatch/utils.cpp	2015-08-25 12:34:12 +0000
@@ -0,0 +1,71 @@
+/*
+ * Copyright (C) 2015 Canonical Ltd
+ *
+ * This file is part of Ubuntu Clock App
+ *
+ * Ubuntu Clock App is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Ubuntu Clock App 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "utils.h"
+
+#include <QtMath>
+
+StopwatchUtils::StopwatchUtils(QObject *parent):
+    QObject(parent)
+{
+}
+
+QString StopwatchUtils::millisToString(int millis) const
+{
+    return addZeroPrefix(QString::number(millis), 3);
+}
+
+QString StopwatchUtils::millisToTimeString(int millis, bool showHours) const
+{
+    int hours, minutes, seconds;
+
+    seconds = qFloor(millis / 1000) % 60;
+    minutes = qFloor(millis / 1000 / 60) % 60;
+    hours = qFloor(millis / 1000 / 60 / 60);
+
+    QString timeString("");
+
+    if (showHours)
+    {
+        timeString += addZeroPrefix(QString::number(hours), 2) + ":";
+    }
+
+    timeString += addZeroPrefix(QString::number(minutes), 2) + ":";
+    timeString += addZeroPrefix(QString::number(seconds), 2);
+
+    return timeString;
+}
+
+QString StopwatchUtils::addZeroPrefix(QString str, int totalLength) const
+{
+    return QString("00000" + str).remove(0, 5 + str.length() - totalLength);
+}
+
+QString StopwatchUtils::lapTimeToString(int millis) const
+{
+    int hours = qFloor(millis / 1000 / 60 / 60);
+
+    if (hours > 0)
+    {
+        return millisToTimeString(millis, true);
+    }
+
+    else {
+        return millisToTimeString(millis, false);
+    }
+}

=== added file 'backend/modules/Stopwatch/utils.h'
--- backend/modules/Stopwatch/utils.h	1970-01-01 00:00:00 +0000
+++ backend/modules/Stopwatch/utils.h	2015-08-25 12:34:12 +0000
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) 2015 Canonical Ltd
+ *
+ * This file is part of Ubuntu Clock App
+ *
+ * Ubuntu Clock App is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Ubuntu Clock App 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef UTILS_H
+#define UTILS_H
+
+#include <QObject>
+
+class StopwatchUtils: public QObject
+{
+    Q_OBJECT
+
+public:
+    StopwatchUtils(QObject *parent=0);
+
+public slots:
+    QString millisToString(int millis) const;
+    QString millisToTimeString(int millis, bool showHours) const;
+    QString addZeroPrefix(QString str, int totalLength) const;
+    QString lapTimeToString(int millis) const;
+};
+
+#endif

=== modified file 'tests/unit/tst_stopwatchUtils.qml'
--- tests/unit/tst_stopwatchUtils.qml	2015-08-17 20:27:12 +0000
+++ tests/unit/tst_stopwatchUtils.qml	2015-08-25 12:34:12 +0000
@@ -18,6 +18,7 @@
 
 import QtQuick 2.4
 import QtTest 1.0
+import Stopwatch 1.0
 import Ubuntu.Components 1.2
 import "../../app/stopwatch"
 
@@ -46,9 +47,11 @@
      correctly.
     */
     function test_convertTimeInMillisecondsToString() {
-        var timeInMilliseconds = 1123000
-        var result = stopwatchUtils.millisToTimeString(timeInMilliseconds, false, true)
+        var timeInMilliseconds = 1123000, result
+        result = stopwatchUtils.millisToTimeString(timeInMilliseconds, true)
         compare(result, "00:18:43", "Time not properly converted from milliseconds to hh:mm:ss")
+        result = stopwatchUtils.millisToTimeString(timeInMilliseconds, false)
+        compare(result, "18:43", "Time not properly converted from milliseconds to mm:ss")
     }
 
     /*