Merge lp:~nik90/ubuntu-clock-app/correct-time-locale into lp:ubuntu-clock-app

[Nekhelesh Ramananthan]

Returns the clock time in the proper user locale.

Returns the clock time in the proper user locale.

By using Qt.formatTime(new Date()), we allow Qt to return the time in locale set by the user. So the clock app does not control whether it is returned in 12-hour or 24-hour, but rather the locale dictates that.

However the designers stated that they want the time to be centered in bold text while the "am/pm" text should be shown beneath it if the locale uses the 12-hour format. I used Qt.locale().pmText and Qt.locale().amText to detect if there are "am/pm" strings in the clock time and if they are present then format the time accordingly to show it as defined by the designers.

All this is done without affecting the locale set by the user.

@victor, one reason why we do is that during my discussing with david, he informed me that time in certain locales are shown as hh.mm in which cases using ":" to split the string will fail. As a result, I resorted to showing the time including the separator in one color and then showing the "am/pm" text below using the qt locale variable.

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:12
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~nik90/ubuntu-clock-app/correct-time-locale/+merge/225005/+edit-commit-message

http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-ci/5/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/724/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-utopic-amd64-ci/5/console

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-ci/5/rebuild

review: Needs Fixing (continuous-integration)

[Victor Thompson (vthompson)]

1. Currently the time does not show in the 12 hour locale. Please make the following change:

bzr diff
=== modified file 'app/clock/Clock.qml'
--- app/clock/Clock.qml 2014-06-30 13:39:47 +0000
+++ app/clock/Clock.qml 2014-06-30 22:42:42 +0000
@@ -95,11 +95,11 @@
                 }
                 else if(time.search(Qt.locale().amText) !== -1) {
                     // 12 hour format detected with the localised AM text
- return time.split(Qt.locale().amText)[0].slice(end-1)
+ return time.split(Qt.locale().amText)[0]
                 }
                 else {
                     // 12 hour format detected with the localised PM text
- return time.split(Qt.locale().pmText)[0].slice(end-1)
+ return time.split(Qt.locale().pmText)[0]
                 }
             }
         }

2. Each of places you check for amText/pmText would be simplified if you changed from doing this:

if (not AM and not PM)
else if (AM)
else

to be

if (AM)
else if (PM)
else // assumed to be 24 hour

3. Wouldn't it still be possible to parse the time separator and have it displayed in a different color?

review: Needs Fixing

[Nekhelesh Ramananthan (nik90)]

> 1. Currently the time does not show in the 12 hour locale. Please make the
> following change:
>
> bzr diff
> === modified file 'app/clock/Clock.qml'
> --- app/clock/Clock.qml 2014-06-30 13:39:47 +0000
> +++ app/clock/Clock.qml 2014-06-30 22:42:42 +0000
> @@ -95,11 +95,11 @@
> }
> else if(time.search(Qt.locale().amText) !== -1) {
> // 12 hour format detected with the localised AM text
> - return time.split(Qt.locale().amText)[0].slice(end-1)
> + return time.split(Qt.locale().amText)[0]
> }
> else {
> // 12 hour format detected with the localised PM text
> - return time.split(Qt.locale().pmText)[0].slice(end-1)
> + return time.split(Qt.locale().pmText)[0]
> }
> }
> }

Are you sure? The slice(end-1) is only there to remove the gap between "01:00" and "AM" in the string "01:00 AM" which causes the text to be off centered. So after splitting it and taking [0], the string is "01:00 " with a space at the end which I then slice of.

[Nekhelesh Ramananthan (nik90)]

> 2. Each of places you check for amText/pmText would be simplified if you
> changed from doing this:
>
> if (not AM and not PM)
> else if (AM)
> else
>
> to be
>
> if (AM)
> else if (PM)
> else // assumed to be 24 hour
>

Done

[Nekhelesh Ramananthan (nik90)]

>3. Wouldn't it still be possible to parse the time separator and have it displayed in a different >color?

I am unable to think of a logic which will work across all locales. Previously we used the colon ":" to do the splitting which fails in other locales. May be we can come up with a regex expression which doesn't depend on the time separator.

Somehow I feel this solution in the MP is more fail proof due to its simplicity while we stand on the shoulders of Qt locale(). Surprisingly Qt.locale() does not have a property for time separator. Otherwise I would have used that.

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:13
http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-ci/6/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/729/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-utopic-amd64-ci/6/console

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-ci/6/rebuild

review: Needs Fixing (continuous-integration)

[Victor Thompson (vthompson)]

> > 1. Currently the time does not show in the 12 hour locale. Please make the
> > following change:
> >
> > bzr diff
> > === modified file 'app/clock/Clock.qml'
> > --- app/clock/Clock.qml 2014-06-30 13:39:47 +0000
> > +++ app/clock/Clock.qml 2014-06-30 22:42:42 +0000
> > @@ -95,11 +95,11 @@
> > }
> > else if(time.search(Qt.locale().amText) !== -1) {
> > // 12 hour format detected with the localised AM text
> > - return time.split(Qt.locale().amText)[0].slice(end-1)
> > + return time.split(Qt.locale().amText)[0]
> > }
> > else {
> > // 12 hour format detected with the localised PM text
> > - return time.split(Qt.locale().pmText)[0].slice(end-1)
> > + return time.split(Qt.locale().pmText)[0]
> > }
> > }
> > }
>
> Are you sure? The slice(end-1) is only there to remove the gap between "01:00"
> and "AM" in the string "01:00 AM" which causes the text to be off centered. So
> after splitting it and taking [0], the string is "01:00 " with a space at the
> end which I then slice of.

Positive. "end" is not defined:

file:///home/victor/Development/correct-time-locale/app/clock/Clock.qml:93: ReferenceError: end is not defined

Could you simply do this to trim the whitespace:

bzr diff
=== modified file 'app/clock/Clock.qml'
--- app/clock/Clock.qml 2014-07-01 10:47:41 +0000
+++ app/clock/Clock.qml 2014-07-01 11:17:00 +0000
@@ -90,11 +90,11 @@
             text: {
                 if (time.search(Qt.locale().amText) !== -1) {
                     // 12 hour format detected with the localised AM text
- return time.split(Qt.locale().amText)[0].slice(end-1)
+ return time.split(Qt.locale().amText)[0].trim()
                 }
                 else if (time.search(Qt.locale().pmText) !== -1) {
                     // 12 hour format detected with the localised PM text
- return time.split(Qt.locale().pmText)[0].slice(end-1)
+ return time.split(Qt.locale().pmText)[0].trim()
                 }
                 else {
                     // 24-hour format detected, return full time string

[Nekhelesh Ramananthan (nik90)]

> > > 1. Currently the time does not show in the 12 hour locale. Please make the
> > > following change:
> > >
> > > bzr diff
> > > === modified file 'app/clock/Clock.qml'
> > > --- app/clock/Clock.qml 2014-06-30 13:39:47 +0000
> > > +++ app/clock/Clock.qml 2014-06-30 22:42:42 +0000
> > > @@ -95,11 +95,11 @@
> > > }
> > > else if(time.search(Qt.locale().amText) !== -1) {
> > > // 12 hour format detected with the localised AM text
> > > - return time.split(Qt.locale().amText)[0].slice(end-1)
> > > + return time.split(Qt.locale().amText)[0]
> > > }
> > > else {
> > > // 12 hour format detected with the localised PM text
> > > - return time.split(Qt.locale().pmText)[0].slice(end-1)
> > > + return time.split(Qt.locale().pmText)[0]
> > > }
> > > }
> > > }
> >
> > Are you sure? The slice(end-1) is only there to remove the gap between
> "01:00"
> > and "AM" in the string "01:00 AM" which causes the text to be off centered.
> So
> > after splitting it and taking [0], the string is "01:00 " with a space at
> the
> > end which I then slice of.
>
> Positive. "end" is not defined:
>
> file:///home/victor/Development/correct-time-locale/app/clock/Clock.qml:93:
> ReferenceError: end is not defined
>
> Could you simply do this to trim the whitespace:
>

Wasn't aware of the trim() function. Thnx. I did that in the latest rev

[Nekhelesh Ramananthan (nik90)]

One more thing, I reduced the time period font size to 12 dp which corresponds to small fontSize as specified in the design spec at https://docs.google.com/presentation/d/1Kkl7xT8BYo9mT8i3IXh6rPrDlNg6cBxTajahlRYIGlI/edit#slide=id.g35b3f34e6_00

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:14
http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-ci/7/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/730/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-utopic-amd64-ci/7/console

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-ci/7/rebuild

review: Needs Fixing (continuous-integration)

[Victor Thompson (vthompson)]

IMO 12 dp is far too small. It appears even smaller than the design spec: http://i.imgur.com/WyYBq3T.png

review: Approve

[Victor Thompson (vthompson)]

16 dp might be closer for the am/pm text, but other than that this looks good!

[Nekhelesh Ramananthan (nik90)]

> IMO 12 dp is far too small. It appears even smaller than the design spec:
> http://i.imgur.com/WyYBq3T.png

I agree that 16 dp would be much better. But if you look at the word "Location" in the screenshot, it has a font size "Medium". So the "am/pm" text should be appear smaller than that. Somehow the designers say that the font size will be correct when viewed on the device. I am not so sure if things will improve on the device.

However let's come back to the font sizes through out the app once we get clock app running on the device.

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-autolanding/1/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/811/console
    FAILURE: http://91.189.93.70:8080/job/ubuntu-clock-dev-ubuntu-clock-app-utopic-3.0-utopic-amd64-autolanding/1/console

review: Needs Fixing (continuous-integration)

=== modified file 'app/clock/Clock.qml'
--- app/clock/Clock.qml	2014-06-26 11:25:17 +0000
+++ app/clock/Clock.qml	2014-07-01 12:08:31 +0000
@@ -66,8 +66,8 @@
             }
             
             PropertyAnimation {
-                target: _digitalTimeRow
-                property: "digitalTimeFontPixelSize"
+                target: _digitalTime
+                property: "font.pixelSize"
                 to: units.dp(62)
                 duration: 900
             }
@@ -75,52 +75,57 @@
             PropertyAnimation {
                 target: _digitalTimePeriod
                 property: "font.pixelSize"
-                to: units.dp(24)
+                to: units.dp(12)
                 duration: 900
             }
         }
 
-        Row {
-            id: _digitalTimeRow
-            property real digitalTimeFontPixelSize: units.dp(1)
-            anchors {
-                centerIn: parent
-            }
-        
-            Label {
-                id: _digitalTimeHours
-                color: UbuntuColors.midAubergine
-                font.pixelSize: _digitalTimeRow.digitalTimeFontPixelSize
-                text: time.split(":")[0]
-            }
-
-            Label {
-                id: _digitalTimeDivider
-                color: UbuntuColors.coolGrey
-                font.pixelSize: _digitalTimeRow.digitalTimeFontPixelSize
-                text: ":"
-            }
-
-            Label {
-                id: _digitalTimeMinutes
-                color: UbuntuColors.midAubergine
-                font.pixelSize: _digitalTimeRow.digitalTimeFontPixelSize
-                text: time.split(":")[1].split(" ")[0]
+        Label {
+            id: _digitalTime
+
+            anchors.centerIn: parent
+
+            color: UbuntuColors.midAubergine
+            font.pixelSize: units.dp(1)
+            text: {
+                if (time.search(Qt.locale().amText) !== -1) {
+                    // 12 hour format detected with the localised AM text
+                    return time.split(Qt.locale().amText)[0].trim()
+                }
+                else if (time.search(Qt.locale().pmText) !== -1) {
+                    // 12 hour format detected with the localised PM text
+                    return time.split(Qt.locale().pmText)[0].trim()
+                }
+                else {
+                    // 24-hour format detected, return full time string
+                    return time
+                }
             }
         }
 
         Label {
             id: _digitalTimePeriod
-            property string period: time.split(":")[1].split(" ")[1] !== undefined
-                                    ? time.split(":")[1].split(" ")[1] : ""
-            anchors {
-                top: _digitalTimeRow.bottom
-                horizontalCenter: parent.horizontalCenter
-            }
+
+            anchors.top: _digitalTime.bottom
+            anchors.horizontalCenter: parent.horizontalCenter
+
             color: UbuntuColors.midAubergine
             font.pixelSize: units.dp(1)
-            visible: period !== ""
-            text: period
+            visible: text !== ""
+            text: {
+                if (time.search(Qt.locale().amText) !== -1) {
+                    // 12 hour format detected with the localised AM text
+                    return Qt.locale().amText
+                }
+                else if (time.search(Qt.locale().pmText) !== -1) {
+                    // 12 hour format detected with the localised PM text
+                    return Qt.locale().pmText
+                }
+                else {
+                    // 24-hour format detected
+                    return ""
+                }
+            }
         }
     }
 }