ubuntu-touch-coreapps-reviewers team mailing list archive

[Nekhelesh Ramananthan <krnekhelesh@xxxxxxxxx>]

Review: Needs Fixing

Code looks good except for some really really minor quibbles that I have pointed out. Most of them cant be ignored if you wish to do so, however there were some spelling mistakes and confusion in the function names. Please fix them. Thanks.

Diff comments:

> === modified file 'app/alarm/AlarmModelComponent.qml'
> --- app/alarm/AlarmModelComponent.qml	2015-07-15 22:31:52 +0000
> +++ app/alarm/AlarmModelComponent.qml	2015-08-14 05:38:13 +0000
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2014 Canonical Ltd
> + * Copyright (C) 2014-2015 Canonical Ltd
>   *

Let's keep our code changes specific to the task at hand. This MP is about the world city bug, and so updating the copyright year of only those files which were changed in this MP would be recommended. Now that this is done, it is fine ;)

>   * This file is part of Ubuntu Clock App
>   *
> 
> === modified file 'app/worldclock/WorldCityList.qml'
> --- app/worldclock/WorldCityList.qml	2015-07-24 13:16:28 +0000
> +++ app/worldclock/WorldCityList.qml	2015-08-14 05:38:13 +0000
> @@ -266,24 +266,27 @@
>          id: cityList
>          objectName: "cityList"
>  
> -        function addWorldCity(city, country, timezone) {
> -            console.log("[LOG]: Adding city to U1db Database")
> +        function addWorldCity(cityId, countryName, timezone) {
> +            console.log("[LOG]: Adding " + cityId.toString() + " city to U1db Database")
>              clockDB.putDoc
>                      (
>                          {
>                              "worldlocation":
>                              {
> -                                "city": city,
> -                                "country": country,
> +                                "city": cityId,
> +                                "country": countryName.replace("'"," "),
>                                  "timezone": timezone
>                              }
>                          },
> -                        encodeURIComponent(city + "_" + country)
> +                        // Apostrophes are forbidden by database, so we replace it with spaces.
> +                        // It will be replaced/translated next time after read from database.
> +                        // Country field is used only by jsonTimeZoneModel (lp: #1473074).
> +                        encodeURIComponent(cityId + "_" + countryName.replace("'"," "))
>                          )

The only thing I am worried about here is if there are other symbols like ^ (used in french) that are also forbidden by the database which might trigger the bug. During my testing, I didnt notice any issue. But a more generalistic solution to fixing this solution would be recommended.

>          }
>  
>          function getSectionText(index) {
> -            return sortedTimeZoneModel.get(index).city.substring(0,1)
> +            return sortedTimeZoneModel.get(index).cityName.substring(0,1)
>          }
>  
>          onFlickStarted: {
> 
> === modified file 'backend/modules/Timezone/generictimezonemodel.cpp'
> --- backend/modules/Timezone/generictimezonemodel.cpp	2014-08-10 13:06:37 +0000
> +++ backend/modules/Timezone/generictimezonemodel.cpp	2015-08-14 05:38:13 +0000
> @@ -57,22 +60,35 @@
>  
>      m_timeZones.clear();
>  
> -    TimeZone tz;
> +    CityData cityData;
>  
>      /*
>       Cycle through the u1db query model results and transfer them to the
>       TimeZone list.
>      */
> -    for (int i=0; i < m_results.size(); i++) {
> +    StaticTimeZoneModel timeZonesData;
> +    for (int i=0; i < m_results.size(); i++)
> +    {
>          // Map query model results to timezone tz
> -        tz.cityName = m_results.value(i).toMap().value("city").toString();
> -        tz.country = m_results.value(i).toMap().value("country").toString();
> -        tz.timeZone = QTimeZone(m_results.value(i).toMap().value("timezone").toString().toLatin1());
> -
> -        m_timeZones.append(tz);
> +        cityData.cityId = m_results.value(i).toMap().value("city").toString();
> +
> +        TimeZoneModel::CityData trandslatedCityData = timeZonesData.getTranslatedCityData(cityData.cityId);

trandslatedCityData is spelled incorrectly. It should be translatedCityData

> +        if (trandslatedCityData.cityId == "")

Incorrect spelling tradslatedCityData

> +        {
> +            cityData.cityName = cityData.cityId;
> +            cityData.countryName = m_results.value(i).toMap().value("country").toString();
> +        }
> +        else
> +        {
> +            cityData.cityName = trandslatedCityData.cityName;
> +            cityData.countryName = trandslatedCityData.countryName;

Incorrect spelling.

> +        }
> +        cityData.timeZone = QTimeZone(m_results.value(i).toMap().value("timezone").toString().toLatin1());
> +
> +        m_timeZones.append(cityData);
>  
>          // Clear tz before next iteration
> -        tz = TimeZone();
> +        cityData = CityData();
>      }
>  
>  
> 
> === modified file 'backend/modules/Timezone/statictimezonemodel.h'
> --- backend/modules/Timezone/statictimezonemodel.h	2015-02-27 12:14:50 +0000
> +++ backend/modules/Timezone/statictimezonemodel.h	2015-08-14 05:38:13 +0000
> @@ -31,12 +31,15 @@
>  public:
>      StaticTimeZoneModel(QObject *parent = 0);
>  
> +    // Function to get translated translated city and country name
> +    Q_INVOKABLE TimeZoneModel::CityData getTranslatedCityData(const QString &cityId);

Could we change this to TimeZoneModel:CityData getTranslatedCityDate(const QString &cityId); essentially removing the Q_INVOKABLE to maintain code uniformity with other slots defined in other classes? (Just a minor thing)

> +
>  private:
>      // Function to define the default city list
>      void loadDefaultCityList();
>  
>      // Function to append city list item into m_timeZones object
> -    void addCity(const QString &city, const QString &timezone, const QString &country);
> +    void addCity(const QString &cityId, const QString &cityName, const QString &timezone, const QString &countryName);
>  };
>  
>  #endif // STATICTIMEZONEMODEL_H
> 
> === modified file 'backend/modules/Timezone/timezonemodel.h'
> --- backend/modules/Timezone/timezonemodel.h	2014-09-02 14:31:44 +0000
> +++ backend/modules/Timezone/timezonemodel.h	2015-08-14 05:38:13 +0000
> @@ -113,14 +114,15 @@
>  
>  protected:
>      // Create a simple container class to hold our information
> -    struct TimeZone{
> +    struct CityData {
> +        QString cityId;
>          QString cityName;
> -        QString country;
> +        QString countryName;
>          QTimeZone timeZone;
>      };
>  
>      // Keep a list of TimeZone objects, holding all our timeZones.
> -    QList<TimeZone> m_timeZones;
> +    QList<CityData> m_timeZones;

This change of variable type name from TimeZone to CityData is actually a bit confusing. I understand the reason why it was done, but it conflicts with the class name which is called timezonemodel :P. As a result, in other places we import the timezonemodel class but instantiate a cityData variable type. If its not too much trouble, can you improve this pls?

>  
>      // Private property to keep track of the status of the timezonemodel
>      Status m_status;


-- 
https://code.launchpad.net/~gang65/ubuntu-clock-app/ubuntu-clock-city-name-fix/+merge/266153
Your team Ubuntu Clock Developers is subscribed to branch lp:ubuntu-clock-app.