ubuntu-touch-coreapps-reviewers team mailing list archive
-
ubuntu-touch-coreapps-reviewers team
-
Mailing list archive
-
Message #04010
Re: [Merge] lp:~ubuntu-clock-dev/ubuntu-clock-app/stopwatch-feature-staging into lp:ubuntu-clock-app
Review: Needs Fixing
Some additional inline comments
Diff comments:
>
> === added file 'app/MainPage.qml'
> --- app/MainPage.qml 1970-01-01 00:00:00 +0000
> +++ app/MainPage.qml 2015-08-06 00:59:41 +0000
> @@ -0,0 +1,82 @@
> +/*
> + * 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
> +import "upstreamcomponents"
> +import "alarm"
> +import "clock"
> +import "stopwatch"
> +
> +PageWithBottomEdge {
> + id: _mainPage
> + objectName: "mainPage"
> +
> + // Property to keep track of the clock time
> + property var clockTime: new Date()
> +
> + // Property to keep track of an app cold start status
> + property alias isColdStart: clockPage.isColdStart
> +
> + // Property to check if the clock page is currently visible
> + property bool isClockPage: listview.currentIndex === 0
> +
> + // Clock App Alarm Model Reference Variable
> + property var alarmModel
> +
> + flickable: null
> + bottomEdgeTitle: _mainPage.visible ? alarmUtils.set_bottom_edge_title(alarmModel, clockTime)
> + : i18n.tr("No Active Alarms")
> +
> + Component.onCompleted: {
> + console.log("[LOG]: Main Page loaded")
I think we don't need this log in official release
> + _mainPage.setBottomEdgePage(Qt.resolvedUrl("alarm/AlarmPage.qml"), {})
> + }
> +
> + AlarmUtils {
> + id: alarmUtils
> + }
> +
> + VisualItemModel {
> + id: navigationModel
> + ClockPage {
> + id: clockPage
> + clockTime: _mainPage.clockTime
> + width: clockApp.width
> + height: listview.height
> + }
> +
> + StopwatchPage {
> + id: stopwatchPage
> + width: clockApp.width
> + height: listview.height
> + }
> + }
> +
> + ListView {
> + id: listview
> +
> + anchors.fill: parent
> + model: navigationModel
> + orientation: ListView.Horizontal
> + snapMode: ListView.SnapOneItem
> + interactive: false
> + highlightMoveDuration: UbuntuAnimation.BriskDuration
> + highlightRangeMode: ListView.StrictlyEnforceRange
> + }
> +}
>
> === modified file 'app/clock/ClockPage.qml'
> --- app/clock/ClockPage.qml 2015-07-21 11:40:58 +0000
> +++ app/clock/ClockPage.qml 2015-08-06 00:59:41 +0000
> @@ -317,26 +284,34 @@
> id: otherElementsStartUpAnimation
>
> PropertyAnimation {
> - target: settingsIcon
> - property: "anchors.topMargin"
> - from: units.gu(6)
> - to: units.gu(2)
> - duration: 900
> - }
> -
> - PropertyAnimation {
> - target: settingsIcon
> - property: "opacity"
> - from: 0
> - to: 1
> - duration: 900
> - }
> -
> - PropertyAnimation {
> - target: date
> - property: "anchors.topMargin"
> - from: units.gu(36)
> - to: units.gu(40)
> + target: headerRow
> + property: "anchors.topMargin"
> + from: units.gu(4)
> + to: 0
> + duration: 900
Why we cannot use UbuntuAnimation.SleepyDuration instead (it is set to 1000ms)?
If we cannot, we should create some const for that.
> + }
> +
> + PropertyAnimation {
> + target: headerRow
> + property: "opacity"
> + from: 0
> + to: 1
> + duration: 900
Why we cannot use UbuntuAnimation.SleepyDuration instead (it is set to 1000ms)?
If we cannot, we should create some const for that.
> + }
> +
> + PropertyAnimation {
> + target: date
> + property: "opacity"
> + from: 0
> + to: 1
> + duration: 900
Why we cannot use UbuntuAnimation.SleepyDuration instead (it is set to 1000ms)?
If we cannot, we should create some const for that.
> + }
> +
> + PropertyAnimation {
> + target: date
> + property: "anchors.topMargin"
> + from: units.gu(40)
> + to: units.gu(44)
> duration: 900
Why we cannot use UbuntuAnimation.SleepyDuration instead (it is set to 1000ms)?
If we cannot, we should create some const for that.
> }
> }
>
> === added file 'app/components/HeaderNavigation.qml'
> --- app/components/HeaderNavigation.qml 1970-01-01 00:00:00 +0000
> +++ app/components/HeaderNavigation.qml 2015-08-06 00:59:41 +0000
> @@ -0,0 +1,84 @@
> +/*
> + * 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
> +
> +Item {
> + id: headerRow
> +
> + height: units.gu(7)
> +
> + Row {
> + id: iconContainer
> +
> + anchors.centerIn: parent
> +
> + ActionIcon {
> + width: units.gu(5.5)
> + height: units.gu(4)
> + iconName: "alarm-clock"
> + onClicked: listview.currentIndex = 0
> + }
> +
> + ActionIcon {
> + width: units.gu(5.5)
> + height: units.gu(4)
> + iconName: "stopwatch-lap"
> + onClicked: listview.currentIndex = 1
> + }
> + }
> +
> + Rectangle {
> + id: outerProgressRectangle
> + anchors {
> + left: iconContainer.left
> + right: iconContainer.right
> + top: iconContainer.bottom
> + }
> + height: units.gu(0.3)
> + color: UbuntuColors.lightGrey
> +
> + Rectangle {
> + height: parent.height
> + x: listview.currentIndex == 0 ? 0 : parent.width-width
please add spaces around operator
> + width: units.gu(5.5)
> + color: UbuntuColors.orange
> + Behavior on x {
> + UbuntuNumberAnimation {}
> + }
> + }
> + }
> +
> + ActionIcon {
> + id: settingsIcon
> + objectName: "settingsIcon"
> +
> + anchors {
> + verticalCenter: parent.verticalCenter
> + right: parent.right
> + rightMargin: units.gu(2)
> + }
> +
> + iconName: "settings"
> +
> + onClicked: {
> + mainStack.push(Qt.resolvedUrl("../alarm/AlarmSettingsPage.qml"))
> + }
> + }
> +}
>
> === added directory 'app/stopwatch'
> === added file 'app/stopwatch/CMakeLists.txt'
> --- app/stopwatch/CMakeLists.txt 1970-01-01 00:00:00 +0000
> +++ app/stopwatch/CMakeLists.txt 2015-08-06 00:59:41 +0000
> @@ -0,0 +1,6 @@
> +file(GLOB STOPWATCH_QML_JS_FILES *.qml *.js)
According to official cmake documentation, we shouldn't use glob for source code, because it could cause building issues:
http://www.cmake.org/cmake/help/v3.1/command/file.html
Use list of files instead.
> +
> +# make the files visible in the qtcreator tree
> +add_custom_target(ubuntu-clock-app_stopwatch_QMlFiles ALL SOURCES ${STOPWATCH_QML_JS_FILES})
> +
> +install(FILES ${STOPWATCH_QML_JS_FILES} DESTINATION ${UBUNTU-CLOCK_APP_DIR}/stopwatch)
>
> === added file 'app/stopwatch/LapListView.qml'
> --- app/stopwatch/LapListView.qml 1970-01-01 00:00:00 +0000
> +++ app/stopwatch/LapListView.qml 2015-08-06 00:59:41 +0000
> @@ -0,0 +1,77 @@
> +/*
> + * 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 QtQuick.Layouts 1.1
> +import Ubuntu.Components 1.2
> +
> +ListView {
> + id: lapListView
> +
> + interactive: false
> +
> + StopwatchUtils {
> + id: stopwatchUtils
> + }
> +
> + displaced: Transition {
> + UbuntuNumberAnimation { property: "y"; duration: UbuntuAnimation.BriskDuration }
Please add new lines, to keep common code style.
> + }
> +
> + delegate: ListItem {
> + divider.visible: true
> + leadingActions: ListItemActions {
> + actions: [
> + Action {
> + iconName: "delete"
> + onTriggered: {
> + lapsModel.remove(index, 1)
> + }
> + }
> + ]
> + }
> +
> + Row {
> + anchors {
> + left: parent.left
> + right: parent.right
> + verticalCenter: parent.verticalCenter
> + margins: units.gu(2)
> + }
> +
> + Label {
> + color: UbuntuColors.midAubergine
> + text: "#%1".arg(count - index)
> + width: parent.width/7
Please add spaces around operator
> + horizontalAlignment: Text.AlignHCenter
> + }
> +
> + Label {
> + width: 3*parent.width/7
Please add spaces around operators
> + text: stopwatchUtils.millisToTimeString(model.laptime, true)
> + horizontalAlignment: Text.AlignHCenter
> + }
> +
> + Label {
> + width: 3*parent.width/7
Please add spaces around operator
> + text: stopwatchUtils.millisToTimeString(model.totaltime, true)
> + horizontalAlignment: Text.AlignHCenter
> + }
> + }
> + }
> +}
>
> === added file 'app/stopwatch/StopwatchPage.qml'
> --- app/stopwatch/StopwatchPage.qml 1970-01-01 00:00:00 +0000
> +++ app/stopwatch/StopwatchPage.qml 2015-08-06 00:59:41 +0000
> @@ -0,0 +1,184 @@
> +/*
> + * 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 QtQuick.Layouts 1.1
> +import Ubuntu.Components 1.2
> +import "../components"
> +
> +Item {
> + id: _stopwatchPage
> + objectName: "stopwatchPage"
> +
> + property date startTime: new Date()
> + property date snapshot: startTime
> + property bool running: false
> +
> + property int timeDiff: snapshot - startTime
> + property int oldDiff: 0
> +
> + property int totalTimeDiff: timeDiff + oldDiff
> +
> + Component.onCompleted: {
> + console.log("[LOG]: Stopwatch Page Loaded")
Why do we need this log?
> + }
> +
> + function start() {
> + startTime = new Date()
> + snapshot = startTime
> + running = true
> + }
> +
> + function stop() {
> + oldDiff += timeDiff
> + startTime = new Date()
> + snapshot = startTime
> + running = false
> + }
> +
> + function update() {
> + snapshot = new Date()
> + }
> +
> + function clear() {
> + oldDiff = 0
> + startTime = new Date()
> + snapshot = startTime
> + lapsModel.clear()
> + }
> +
> + ListModel {
> + id: lapsModel
> + function addLap(totalTime) {
> + if (lapsModel.count === 0) {
> + append({"laptime": totalTime, "totaltime": totalTime})
> + } else {
> + insert(0, {"laptime": totalTime - lapsModel.get(0).totaltime, "totaltime": totalTime})
> + }
> + }
> + }
> +
> + Timer {
> + id: refreshTimer
> + interval: 45
> + repeat: true
> + running: _stopwatchPage.running
> + onTriggered: {
> + _stopwatchPage.update()
> + }
> + }
> +
> + Flickable {
> + id: _flickable
> +
> + onFlickStarted: {
> + forceActiveFocus()
> + }
> +
> + clip: true
> + anchors.fill: parent
> + interactive: contentHeight > height
> + contentWidth: parent.width
> + contentHeight: stopwatch.height + buttonRow.height + lapListViewLoader.height + units.gu(14)
> +
> + HeaderNavigation {
> + id: headerRow
> + anchors {
> + top: parent.top
> + left: parent.left
> + right: parent.right
> + topMargin: 0
> + }
> + }
> +
> + StopwatchFace {
> + id: stopwatch
> + objectName: "stopwatch"
> +
> + milliseconds: _stopwatchPage.totalTimeDiff
> +
> + anchors {
> + verticalCenter: parent.top
> + verticalCenterOffset: units.gu(25)
> + horizontalCenter: parent.horizontalCenter
> + }
> + }
> +
> + RowLayout {
> + id: buttonRow
> +
> + spacing: units.gu(2)
> + anchors {
> + top: parent.top
> + topMargin: units.gu(44)
> + left: parent.left
> + right: parent.right
> + margins: units.gu(2)
> + }
> +
> + Button {
> + id: stopButton
> + Layout.fillWidth: true
> + color: !_stopwatchPage.running ? UbuntuColors.green : UbuntuColors.red
> + text: _stopwatchPage.running ? i18n.tr("Stop") : oldDiff === 0 ? i18n.tr("Start") : i18n.tr("Resume")
> + onClicked: {
> + if (_stopwatchPage.running) {
> + _stopwatchPage.stop()
> + } else {
> + _stopwatchPage.start()
> + }
> + }
> + }
> +
> + Button {
> + id: lapButton
> + text: _stopwatchPage.running ? i18n.tr("Lap") : i18n.tr("Clear")
> + Layout.fillWidth: true
> + strokeColor: UbuntuColors.lightGrey
> + onClicked: {
> + if (_stopwatchPage.running) {
> + _stopwatchPage.update()
> + lapsModel.addLap(_stopwatchPage.totalTimeDiff)
> + } else {
> + _stopwatchPage.clear()
> + }
> + }
> + }
> + }
> +
> + Loader {
> + id: lapListViewLoader
> + anchors {
> + top: buttonRow.bottom
> + left: parent.left
> + right: parent.right
> + topMargin: units.gu(1)
> + }
> + height: units.gu(7) * lapsModel.count
> + sourceComponent: !_stopwatchPage.running && _stopwatchPage.totalTimeDiff == 0 ? undefined : lapListViewComponent
> + }
> +
> + Component {
> + id: lapListViewComponent
> + LapListView {
> + id: lapListView
> + model: lapsModel
> + }
> + }
> + }
> +}
>
> === added file 'app/stopwatch/StopwatchUtils.qml'
> --- app/stopwatch/StopwatchUtils.qml 1970-01-01 00:00:00 +0000
> +++ app/stopwatch/StopwatchUtils.qml 2015-08-06 00:59:41 +0000
> @@ -0,0 +1,67 @@
> +/*
> + * 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 zeroPadding(millis % 1000, 3)
> + }
> +
> + // Function to break down time (milliseconds) to hours, minutes and seconds
> + function millisToTimeString(millis, showMilliseconds) {
> + var hours, minutes, seconds, milliseconds
> +
> + // Break down total time (milliseconds) to hours, minutes, seconds and milliseconds
> + milliseconds = millis % 1000
> + seconds = Math.floor(millis / 1000) % 60;
> + minutes = Math.floor(millis / 1000 / 60) % 60
> + hours = Math.floor(millis / 1000 / 60 / 60)
> +
> + // Build the time string without milliseconds
> + var timeString = ""
> + timeString += zeroPadding(hours, 2) + ":"
> + timeString += zeroPadding(minutes, 2) + ":"
> + timeString += zeroPadding(seconds, 2)
> +
> + if (showMilliseconds) {
> + timeString += "." + zeroPadding(milliseconds, 3)
We should use "millisToString" function here to remove duplicated code.
> + }
> +
> + return timeString
> + }
> +
> + // Function to add zero prefix if necessary.
> + function zeroPadding (str, count) {
Maybe rename this function to "addZeroPrefix", and "count" to "totalLength"?
> + var string, tmp
> +
> + string = "" + str
> + tmp = ""
> + for (var i = 0; i < count; i++) {
we don't need this loop. We could set tmp to "00000000000" and then make ("000000000" + str).slice(-count):
http://stackoverflow.com/questions/3884632/how-to-get-the-last-character-of-a-string
to improve performance
> + tmp += "0"
> + }
> + return (tmp + str).substring(string.length)
> + }
> +}
--
https://code.launchpad.net/~ubuntu-clock-dev/ubuntu-clock-app/stopwatch-feature-staging/+merge/266718
Your team Ubuntu Clock Developers is subscribed to branch lp:ubuntu-clock-app.
References
