ubuntu-touch-coreapps-reviewers team mailing list archive

[Bartosz Kosiorek <gang65@xxxxxxxxxxxxxx>]

Review: Needs Fixing

Some more inline remarks.

Diff comments:

> 
> === added file 'app/stopwatch/LapListView.qml'
> --- app/stopwatch/LapListView.qml	1970-01-01 00:00:00 +0000
> +++ app/stopwatch/LapListView.qml	2015-08-06 15:24:30 +0000
> @@ -0,0 +1,113 @@
> +/*
> + * 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
> +    }
> +
> +    header: ListItem {
> +        visible: count !== 0

What do you think about always displaying header and current time from stopwatch in the first row.
After you press "lap" the time will stopped, it will be scrolled down (as it is currently) and then new real time watch will be displayed.

?

> +        Row {
> +            anchors {
> +                left: parent.left
> +                right: parent.right
> +                verticalCenter: parent.verticalCenter
> +                margins: units.gu(2)
> +            }
> +
> +            Label {
> +                text: i18n.tr("Lap #")

It is really hard to translate "Lap #" to other languages. Can we change it to just "Lap"?
I will also be very useful comment for developers, that there is only few chars available in that Label.

> +                width: parent.width / 7
> +                font.weight: Font.DemiBold
> +                horizontalAlignment: Text.AlignHCenter
> +            }
> +
> +            Label {
> +                width: 3 * parent.width / 7
> +                text: i18n.tr("Lap Time")
> +                font.weight: Font.DemiBold
> +                horizontalAlignment: Text.AlignHCenter
> +            }
> +
> +            Label {
> +                width: 3 * parent.width / 7
> +                text: i18n.tr("Total Time")
> +                font.weight: Font.DemiBold
> +                horizontalAlignment: Text.AlignHCenter
> +            }
> +        }
> +    }
> +
> +    displaced: Transition {
> +        UbuntuNumberAnimation {
> +            property: "y"
> +            duration: UbuntuAnimation.BriskDuration
> +        }
> +    }
> +
> +    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)

We should use there .toLocaleString(Qt.locale(), "f", 0) function, to support different numbers, eg:
https://en.wikipedia.org/wiki/Eastern_Arabic_numerals

> +                width: parent.width / 7
> +                horizontalAlignment: Text.AlignHCenter
> +            }
> +            
> +            Label {
> +                width: 3 * parent.width / 7
> +                text: stopwatchUtils.millisToTimeString(model.laptime, true)
> +                horizontalAlignment: Text.AlignHCenter
> +            }
> +            
> +            Label {
> +                width: 3 * parent.width / 7
> +                text: stopwatchUtils.millisToTimeString(model.totaltime, true)
> +                horizontalAlignment: Text.AlignHCenter
> +            }
> +        }
> +    }
> +}
> 
> === added file 'app/stopwatch/StopwatchUtils.qml'
> --- app/stopwatch/StopwatchUtils.qml	1970-01-01 00:00:00 +0000
> +++ app/stopwatch/StopwatchUtils.qml	2015-08-06 15:24:30 +0000
> @@ -0,0 +1,59 @@
> +/*
> + * 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) {
> +        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 without milliseconds
> +        var timeString = ""
> +        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)

This function should be moved up, because it is commonly used

We should use there .toLocaleString(Qt.locale(), "f", 0) function, to support different numbers, eg:
https://en.wikipedia.org/wiki/Eastern_Arabic_numerals

> +    }
> +}


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