← Back to team overview

ubuntu-touch-coreapps-reviewers team mailing list archive

Re: [Merge] lp:~ubuntu-clock-dev/ubuntu-clock-app/stopwatch-feature-staging into lp:ubuntu-clock-app

 

@Bartosz I addressed the inline comments in the latest commit. Fixed many of them and explained the reasoning for the rest.

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")

It helps debugging user logs to know what's happening in the background and help spot timing issues. I would rather keep it there.

> +        _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

If you look at the motion design specs described at https://drive.google.com/drive/folders/0B-I2gOjaTc7dNHNOMU5rNEJxRlk, you will notice that the duration was set to 900ms by the designers. As for creating const, I rather not do it since it is set to 900 in just 4 places and is easier to execute this rather than check the const value. Agree?

> +            }
> +
> +            PropertyAnimation {
> +                target: headerRow
> +                property: "opacity"
> +                from: 0
> +                to: 1
> +                duration: 900

If you look at the motion design specs described at https://drive.google.com/drive/folders/0B-I2gOjaTc7dNHNOMU5rNEJxRlk, you will notice that the duration was set to 900ms by the designers. As for creating const, I rather not do it since it is set to 900 in just 4 places and is easier to execute this rather than check the const value. Agree?

> +            }
> +
> +            PropertyAnimation {
> +                target: date
> +                property: "opacity"
> +                from: 0
> +                to: 1
> +                duration: 900

If you look at the motion design specs described at https://drive.google.com/drive/folders/0B-I2gOjaTc7dNHNOMU5rNEJxRlk, you will notice that the duration was set to 900ms by the designers. As for creating const, I rather not do it since it is set to 900 in just 4 places and is easier to execute this rather than check the const value. Agree?

> +            }
> +
> +            PropertyAnimation {
> +                target: date
> +                property: "anchors.topMargin"
> +                from: units.gu(40)
> +                to: units.gu(44)
>                  duration: 900

If you look at the motion design specs described at https://drive.google.com/drive/folders/0B-I2gOjaTc7dNHNOMU5rNEJxRlk, you will notice that the duration was set to 900ms by the designers. As for creating const, I rather not do it since it is set to 900 in just 4 places and is easier to execute this rather than check the const value. Agree?

>              }
>          }
> 
> === 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

Done

> +            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 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 }

Done. But just so you know, the official QML code style recommends grouping similar properties in the same line. But since we haven't done it anywhere else in the clock app, I agree. May be a bite-size task for the future ;).

> +    }
> +
> +    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

Done

> +                horizontalAlignment: Text.AlignHCenter
> +            }
> +            
> +            Label {
> +                width: 3*parent.width/7

Done

> +                text: stopwatchUtils.millisToTimeString(model.laptime, true)
> +                horizontalAlignment: Text.AlignHCenter
> +            }
> +            
> +            Label {
> +                width: 3*parent.width/7

Done

> +                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")

As part of performance I intend to load/unload the stopwatch page when not used. The stopwatch loaded/unloaded log statement will help improve debugging in the future in case of issues.

> +    }
> +
> +    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)

Done

> +        }
> +
> +        return timeString
> +    }
> +
> +    // Function to add zero prefix if necessary.
> +    function zeroPadding (str, count) {

Indeed good suggestion. Meaningful function and variable names improve code readability.

> +        var string, tmp
> +
> +        string  = "" + str
> +        tmp = ""
> +        for (var i = 0; i < count; i++) {

That's actually smart. I was able to reduce the whole addZeroPrefix() function to just return ("00000" + str).slice(-totalLength) which is much simpler and performant. Thnx!

> +            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