Code review comment for lp:~gang65/ubuntu-clock-app/ubuntu-clock-mainclock-runtime-timezone-fix

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.localTimeString.split(":")[2],
- localTimeSource.localTimeString.split(":")[3]
- )
- return localTime.toLocaleString()
- }
-
+ subText: localTimeSource.localTimeString + " " + localTimeSource.localDateString

Please retain the FIXME comment since the upstream bug report is valid and does indeed still affect the clock app. We're moving to using the c++ QDateTime *because* the javascript Date object is terrible with timezones.

7. In the C++ code variable declartion below,

     // Private copies of the local time and date
+ QString m_localanalogtime;
     QString m_localtime;

Please differentiate variable names better by renaming them to m_localAnalogTime and m_localDigitalTime pls. Thnx