ubuntu-touch-coreapps-reviewers team mailing list archive

[Arto Jalkanen <ajalkane@xxxxxxxxx>]

Review: Needs Fixing

Several inlined comments. The one about removing all keys but the new ones introduced in this change is the one that needs attention, others are more minor.

Diff comments:

> === modified file 'src/plugin/placesmodel/placesmodel.cpp'
> --- src/plugin/placesmodel/placesmodel.cpp	2015-02-11 19:22:08 +0000
> +++ src/plugin/placesmodel/placesmodel.cpp	2015-09-07 21:27:29 +0000
> @@ -27,12 +28,14 @@
>  
>  PlacesModel::PlacesModel(QObject *parent) :
>      QAbstractListModel(parent)
> +  , m_userSavedLocationsName("userSavedLocations")

There's no reason to have userSavedLocationsName nor userRemovedLocationsName as object's instance variables. They can not be set per instance nor they can be modified after object creation, nor is there any reason why the should be.

Perhaps better to make them constants like:

namespace {
   const char *userSavedLocationsName = "userSavedLocationsName";
   ...
}

or something like that

> +  , m_userRemovedLocationsName("userRemovedLocations")
> +  , m_going_to_rescanMtab(false)
>  {
>      m_userMountLocation = "/media/" + qgetenv("USER");
>      // For example /run/user/1000
>      m_runtimeLocations = QStandardPaths::standardLocations(QStandardPaths::RuntimeLocation);
>  
> -    QStringList defaultLocations;
>      // Set the storage location to a path that works well
>      // with app isolation
>      QString settingsLocation =
> @@ -40,29 +43,43 @@
>              + "/" + QCoreApplication::applicationName() + "/" + "places.conf";
>      m_settings = new QSettings(settingsLocation, QSettings::IniFormat, this);
>  
> +    m_userSavedLocations   = m_settings->value(m_userSavedLocationsName).toStringList();
> +    m_userRemovedLocations = m_settings->value(m_userRemovedLocationsName).toStringList();
> +
> +    //remove old key "storedLocations" or others no longer used
> +    QStringList settingsKeys  = m_settings->allKeys();
> +    foreach (const QString& key, settingsKeys) {
> +       if (key != m_userSavedLocationsName && key != m_userRemovedLocationsName) {

This is error prone. No one will remember this piece of code when adding new key to the settings. Instead  it should only remove "storedLocations" if that is the deprecated piece of code. Preferrably a TODO to remove the block all together at some reasonable point at time (one year from now or whatever?)

> +           m_settings->remove(key);
> +       }
> +    }
> +
>      // Prepopulate the model with the user locations
>      // for the first time it's used
> -    defaultLocations.append(locationHome());
> -    defaultLocations.append(locationDocuments());
> -    defaultLocations.append(locationDownloads());
> -    defaultLocations.append(locationMusic());
> -    defaultLocations.append(locationPictures());
> -    defaultLocations.append(locationVideos());
> -    // Add root also
> -    defaultLocations.append("/");
> -
> -    if (!m_settings->contains("storedLocations")) {
> -        m_locations.append(defaultLocations);
> -    } else {
> -        m_locations = m_settings->value("storedLocations").toStringList();
> +    addDefaultLocation(locationHome());
> +    addDefaultLocation(locationDocuments());
> +    addDefaultLocation(locationDownloads());
> +    addDefaultLocation(locationMusic());
> +    addDefaultLocation(locationPictures());
> +    addDefaultLocation(locationVideos());    
> +
> +    //Network locations
> +    addDefaultLocation(locationSamba());
> +
> +    //mounted locations
> +    addDefaultLocation("/");
> +    initNewUserMountsWatcher();
> +    rescanMtab();
> +
> +    //other user saved locations
> +    foreach (const QString& userLocation, m_userSavedLocations) {
> +       addLocationWithoutStoring(userLocation);
>      }
> +    m_settings->sync();
>  
>      foreach (const QString &location, m_locations) {
>          qDebug() << "Location: " << location;
>      }
> -
> -    initNewUserMountsWatcher();
> -    rescanMtab();
>  }
>  
>  PlacesModel::~PlacesModel() {
> @@ -250,18 +305,35 @@
>      endRemoveRows();
>  }
>  
> +
>  void PlacesModel::addLocation(const QString &location)
> -{
> -    if (addLocationWithoutStoring(location)) {
> -        // Store the location permanently
> -        m_settings->setValue("storedLocations", m_locations);
> +{   
> +    bool sync_seettings = false;

sync_seettings -> sync_settings

> +    //verify it the user had deleted it before and now is inserting it again
> +    int indexRemoved = m_userRemovedLocations.indexOf(location);
> +    if(indexRemoved > -1) {

Spaces: if (indexRemoved > -1) {

> +        m_userRemovedLocations.removeAt(indexRemoved);
> +        m_settings->setValue(m_userRemovedLocationsName, m_userRemovedLocations);
> +        sync_seettings = true;
> +    }
> +    if (addLocationWithoutStoring(location)) {     
> +        // Store the location permanently if it is not default location
> +        if (!isDefaultLocation(location) && !m_userSavedLocations.contains(location))
> +        {
> +            m_userSavedLocations.append(location);
> +            m_settings->setValue(m_userSavedLocationsName, m_userSavedLocations);
> +            sync_seettings = true;
> +        }                
> +    }
> +    if (sync_seettings) {
> +        m_settings->sync();
>      }
>  }
>  
>  bool PlacesModel::addLocationWithoutStoring(const QString &location)
>  {
> -    // Do not allow for duplicates
> -    if (!m_locations.contains(location)) {
> +    // Do not allow for duplicates and look for removed locations from settings
> +    if (!m_locations.contains(location) && !m_userRemovedLocations.contains(location)) {

This changes the functionality of this function critically. So I would recommend renaming the function to something that reflects what it does. Perhaps addLocationIfNotRemovedWithoutStoring() or something like that (shorter name would be better of course :)). The fact that explicitly removed locations are not added is quite important change here.

>          // Tell Qt that we're going to be changing the model
>          // There's no tree-parent, first new item will be at
>          // m_locations.count(), and the last one too


-- 
https://code.launchpad.net/~carlos-mazieri/ubuntu-filemanager-app/samba-ui-02/+merge/270335
Your team Ubuntu File Manager Developers is subscribed to branch lp:ubuntu-filemanager-app.