Merge lp://qastaging/~gang65/ubuntu-clock-app/ubuntu-clock-mainclock-runtime-timezone-fix into lp://qastaging/ubuntu-clock-app

Preliminary Code Review
-----------------------

1. This MP changes some core stuff of the clock app and has potential to introduce regressions. And so we need to make sure that the changes in this MP are the ones which are absolutely necessary and only fixes one issue. With that in mind, please revert changes made to Stopwatch_placeholder.svg, worldclock_placeholder.svg. This way if we have to revert this MP after it lands in trunk, we don't potentially lose out other improvements that were bundled along.

2. I see two variables clockTimeInMain and clockAnalogTimeInMain. It would be better if you could rename it as analogClockTime and digitalClockTime and drop the "Main" postfix.

3. Why is there a need for 2 variable names clockAnalogTimeInMain and clockAnalogTimeinClockPage? We're essentially passing clockAnalogTimeInMain as an argument to the clock page. Perhaps the variable in the clock page can be named as analogTime and digitalTime. The "InMain" and "InClockPage" seems a bit messy in my opinion.

4. As mentioned during our chat, I would like to see a comment above rotation: (parseInt(analogTime.split(":")[1]) * 6) + (parseInt(analogTime.split(":")[2]) / 10) explaining the input string and how it is split up into mulitple strings for better understandability.

5. I am bit afraid that we're losing out on important code below,

- text: {
- if (time.search(Qt.locale().amText) !== -1) {
- // 12 hour format detected with the localised AM text
- return time.replace(Qt.locale().amText, "").trim()
- }
- else if (time.search(Qt.locale().pmText) !== -1) {
- // 12 hour format detected with the localised PM text
- return time.replace(Qt.locale().pmText, "").trim()
- }
- else {
- // 24-hour format detected, return full time string
- return time
- }
- }
+ text: localizedTimeString.split(" ")[0]

The replace() function was used to fix bugs like https://bugs.launchpad.net/ubuntu-clock-app/+bug/1458808. Please again provide a comment above + text: localizedTimeString.split(" ")[0] with the input string format and what is being split. And please test this in the chineese locale to see if it works correctly.

6. In the following code below,

- subText: {
- /*
- FIXME: When the upstream QT bug at
- https://bugreports.qt-project.org/browse/QTBUG-40275 is fixed
- it will be possible to receive a datetime object directly
- instead of using this hack.
- */
- var localTime = new Date
- (
- localTimeSource.localDateString.split(":")[0],
- localTimeSource.localDateString.split(":")[1]-1,
- localTimeSource.localDateString.split(":")[2],
- localTimeSource.localTimeString.split(":")[0],
- localTimeSource.localTimeString.split(":")[1],
- localTimeSource.loca...

Also requires manual tests as we are fixing multiple critical bugs. Please write manual tests for *each* critical bug fixed. Ideally you should start with writing tests before fixing the bug as we follow Test Driven Development (TDD) ;)

PASSED: Continuous integration, rev:409
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/845/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-ci/190

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/845/rebuild

PASSED: Continuous integration, rev:410
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/846/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-ci/191

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/846/rebuild

PASSED: Continuous integration, rev:411
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/847/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-ci/192

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/847/rebuild

A couples of fixed needed,

1. In the Settings Page, the time & date are shown as undefined.

2. The alarm function breaks (like in trunk) when editing alarms. But I guess we can tackle this in more detail in the next MP. So that's fine for now.

3. I would like a dedicated localized datetime property to be used here instead of localTimeString + localDateString analogy. Can you introduce a property in the c++ class called localizedDateTimeString and use that instead.

- subText: {
- /*
- FIXME: When the upstream QT bug at
- https://bugreports.qt-project.org/browse/QTBUG-40275 is fixed
- it will be possible to receive a datetime object directly
- instead of using this hack.
- */
- var localTime = new Date
- (
- localTimeSource.localDateString.split(":")[0],
- localTimeSource.localDateString.split(":")[1]-1,
- localTimeSource.localDateString.split(":")[2],
- localTimeSource.localTimeString.split(":")[0],
- localTimeSource.localTimeString.split(":")[1],
- localTimeSource.localTimeString.split(":")[2],
- localTimeSource.localTimeString.split(":")[3]
- )
- return localTime.toLocaleString()
- }
-
+ subText: localTimeSource.localTimeString + " " + localTimeSource.localDateString

4. This code has a logical flaw. For instance let's say the local user date time (in Poland) is 17th September 2015, 00:30. If I change to London, the local date time should be 16th September 2015, 23:30. Notice that the date has changed from 17th to 16th september. However in the code below, the date is obtained using the javascript date object which doesn't work well with timezones and may not be aware of this change leading to bugs in the date.

- localTime: clockTime
+ localTime: {
+ var date = new Date();
+ return new Date
+ (
+ date.getFullYear(),
+ date.getMonth(),
+ date.getDate(),
+ notLocalizedTimeString.split(":")[0],
+ notLocalizedTimeString.split(":")[1],
+ notLocalizedTimeString.split(":")[2],
+ 0
+ )
+ }

5. Same issue as issue #4.

+ var date = new Date();
+ var clockTime = new Date
+ (
+ date.getFullYear(),
+ date.getMonth(),
+ date.getDate(),
+ clockTimeString.split(":")[0],
+ clockTimeString.split(":")[1],
+ clockTimeString.split(":")[2],
+ 0
+ )

6. Same issue as issue #4

+ var date = new Date();
                     return new Date
                             (
- currentTime.localDateString.split(":")[0],
- ...

The following code changes are not necessary. Let me explain the reasoning why,

These code changes would add additional roles to *every* world city delegate in the c++ side and cost us performance. Instead, let's stick with the original code that returns the correct timezone info (hh:mm) as expected and just pass the current local time of the user as a property to the worldcitydelegate.qml. So we save one additional role in the QAbsractModel.

Also do note that you added the extra role in the TimeZone Model class which is the base class. This role will be inherited unnecessarily by StaticTimeZoneModel and JsonTimeZoneModel class where this role is useless as we just show the time at that city.

If you want, instead of the hard code "hh:mm" format, you could return the time in Qt::DefaultLocaleShortDate format.

+ RoleTimezoneId,
+ RoleNotLocalizedTimeString,
+ RoleLocalizedTimeString,

- case RoleTimeString:
+ case RoleNotLocalizedTimeString:
         /*
          FIXME: Until https://bugreports.qt-project.org/browse/QTBUG-40275
          is fixed, we will have to return a string.
         */
- return worldCityTime.toString("hh:mm");
+ return worldCityTime.toString("hh:mm:ss");
+ case RoleLocalizedTimeString:
+ return worldCityTime.time().toString(Qt::DefaultLocaleShortDate);

FAILED: Continuous integration, rev:415
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/848/
Executed test runs:
    FAILURE: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-ci/193/console

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/848/rebuild

PASSED: Continuous integration, rev:416
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/849/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-ci/194

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/849/rebuild

I checked geolocation and after 40 minutes it was autorefreshed automatically:

qml: [LOG]: Starting geolocation update service at UTC time: Mon Nov 16 22:25:17 2015 GMT+0100
UbuntuClipboard - Got invalid serialized mime data. Ignoring it.
qml: [LOG]: Searching online for user location at http://api.geonames.org/findNearbyPlaceNameJSON?lat=51.746&lng=19.5053&username=krnekhelesh&style=full
qml: [LOG]: Stopping geolocation update service

PASSED: Continuous integration, rev:417
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/850/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-clock-app-vivid-amd64-ci/195

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/850/rebuild

lgtm! Nice Work!