Merge lp:~carlos-mazieri/ubuntu-filemanager-app/samba-ui-02 into lp:ubuntu-filemanager-app

[Carlos Jose Mazieri]

    1. Added Samba Location as Network
    2. Removed default locations from setting files, it allows adding new default locations in source code
    3. Kept user added and removed locations in settings file.
    4. Created a regression test for PlacesModel.

    User can remove a default Location and then add it again.

Added Samba location as Network

Improved settings file content to allow add/remove places without affecting default places which are handled in the source code.

Added a regression testing for PlacesModel.

Added "/media/<user>" as watched directory because "/etc/mtab" was failing to fire changes.

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

PASSED: Continuous integration, rev:460
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/543/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-vivid-amd64-ci/165

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

review: Approve (continuous-integration)

[Arto Jalkanen (ajalkane)]

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.

review: Needs Fixing

[Carlos Jose Mazieri (carlos-mazieri)]

I agree with your comments and I will modify that according to that.

Thanks.

[lp:~carlos-mazieri/ubuntu-filemanager-app/samba-ui-02]

461. By Carlos Jose Mazieri on 2015-09-19

PlacesModel::addLocationWithoutStoring() renamed to PlacesModel::addLocationNotRemovedWithoutStoring()
Documented both PlacesModel::addLocation() and PlacesModel::addLocationNotRemovedWithoutStoring()

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

PASSED: Continuous integration, rev:461
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/549/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-vivid-amd64-ci/171

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

review: Approve (continuous-integration)

[Arto Jalkanen (ajalkane)]

review: Approve

=== 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-19 22:49:08 +0000
@@ -15,6 +15,7 @@
  *
  * Author : David Planella <[e-mail]>
  *          Arto Jalkanen <[e-mail]>
+ *          Carlos Mazieri <[e-mail]>
  */
 
 #include "placesmodel.h"
@@ -25,14 +26,20 @@
 #include <QStandardPaths>
 #include <QDebug>
 
+namespace
+{
+  const QString userSavedLocationsName("userSavedLocations");
+  const QString userRemovedLocationsName("userRemovedLocations");
+}
+
 PlacesModel::PlacesModel(QObject *parent) :
-    QAbstractListModel(parent)
+    QAbstractListModel(parent) 
+  , 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 +47,41 @@
             + "/" + QCoreApplication::applicationName() + "/" + "places.conf";
     m_settings = new QSettings(settingsLocation, QSettings::IniFormat, this);
 
+    m_userSavedLocations   = m_settings->value(userSavedLocationsName).toStringList();
+    m_userRemovedLocations = m_settings->value(userRemovedLocationsName).toStringList();
+
+    //remove old key "storedLocations" which is no longer used
+    QLatin1String oldStoredLocations("storedLocations");
+    if (m_settings->contains(oldStoredLocations)) {
+        m_settings->remove(oldStoredLocations);
+    }
+
     // 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) {
+       addLocationNotRemovedWithoutStoring(userLocation);
     }
+    m_settings->sync();
 
     foreach (const QString &location, m_locations) {
         qDebug() << "Location: " << location;
     }
-
-    initNewUserMountsWatcher();
-    rescanMtab();
 }
 
 PlacesModel::~PlacesModel() {
@@ -73,17 +92,29 @@
 PlacesModel::initNewUserMountsWatcher() {
     m_newUserMountsWatcher = new QFileSystemWatcher(this);
 
-    qDebug() << Q_FUNC_INFO << "Start watching mtab file for new mounts" << m_mtabParser.path();
+    connect(m_newUserMountsWatcher, SIGNAL(fileChanged(QString)), this, SLOT(mtabChanged(QString)));
+    connect(m_newUserMountsWatcher, SIGNAL(directoryChanged(QString)), this, SLOT(mtabChanged(QString)));
 
     m_newUserMountsWatcher->addPath(m_mtabParser.path());
+    /*
+     it looks like QFileSystemWatcher does not work for /etc/mtab sometimes, lets use /media/<user> as well
+     See:
+        https://forum.qt.io/topic/8566/qfilesystemwatcher-not-working-with-etc-mtab
+        https://bugs.launchpad.net/ubuntu-filemanager-app/+bug/1444367
+    */
+    m_newUserMountsWatcher->addPath(m_userMountLocation);
 
-    connect(m_newUserMountsWatcher, &QFileSystemWatcher::fileChanged, this, &PlacesModel::mtabChanged);
+    qDebug() << Q_FUNC_INFO << "Start watching mtab file for new mounts, using:"
+             << m_newUserMountsWatcher->files() << "and" << m_newUserMountsWatcher->directories();
 }
 
 void
 PlacesModel::mtabChanged(const QString &path) {
     qDebug() << Q_FUNC_INFO << "file changed in " << path;
-    rescanMtab();
+    if (!m_going_to_rescanMtab) {
+        m_going_to_rescanMtab = true;
+        QTimer::singleShot(100, this, SLOT(rescanMtab()));
+    }
     // Since old mtab file is replaced with new contents, must readd filesystem watcher
     m_newUserMountsWatcher->removePath(path);
     m_newUserMountsWatcher->addPath(path);
@@ -91,6 +122,7 @@
 
 void
 PlacesModel::rescanMtab() {
+    m_going_to_rescanMtab = false;
     const QString& path = m_mtabParser.path();
     qDebug() << Q_FUNC_INFO << "rescanning mtab" << path;
 
@@ -113,7 +145,7 @@
 
     foreach (QString addedMount, addedMounts) {
         qDebug() << Q_FUNC_INFO << "user mount added: " << addedMount;
-        addLocationWithoutStoring(addedMount);
+        addLocationNotRemovedWithoutStoring(addedMount);
         emit userMountAdded(addedMount);
     }
 
@@ -203,6 +235,11 @@
     return standardLocation(QStandardPaths::MoviesLocation);
 }
 
+QString PlacesModel::locationSamba() const
+{
+    return QLatin1Literal("smb://");
+}
+
 int PlacesModel::rowCount(const QModelIndex &parent) const
 {
     Q_UNUSED(parent)
@@ -227,10 +264,30 @@
 
 void PlacesModel::removeItem(int indexToRemove)
 {
-    removeItemWithoutStoring(indexToRemove);
-
-    // Remove the location permanently
-    m_settings->setValue("storedLocations", m_locations);
+    if (indexToRemove >= 0 && indexToRemove < m_locations.count())
+    {
+        bool sync_settings = false;
+        const QString & location = m_locations.at(indexToRemove);
+        //check if the index belongs to a  user saved location
+        int index_user_location = m_userSavedLocations.indexOf(location);
+        if (index_user_location > -1)
+        {
+            // Remove the User saved location permanently
+            m_userSavedLocations.removeAt(index_user_location);
+            m_settings->setValue(userSavedLocationsName, m_userSavedLocations);
+            sync_settings = true;
+        }
+        //save it as removed location, even a default location can be removed
+        if (!m_userRemovedLocations.contains(location)) {
+            m_userRemovedLocations.append(location);
+            m_settings->setValue(userRemovedLocationsName, m_userRemovedLocations);
+            sync_settings = true;
+        }
+        removeItemWithoutStoring(indexToRemove);
+        if (sync_settings) {
+             m_settings->sync();
+        }
+    }
 }
 
 void PlacesModel::removeItemWithoutStoring(int indexToRemove)
@@ -250,18 +307,58 @@
     endRemoveRows();
 }
 
+/*!
+ * \brief PlacesModel::addLocation()
+ *
+ * Adds the location permanently in the settings file.
+ *
+ * If the location has already been deleted by the user it is first removed from the removed settings \a m_userRemovedLocations.
+ *
+ * The location is saved in settings file in \a m_userSavedLocations
+ *
+ * \param location
+ */
 void PlacesModel::addLocation(const QString &location)
-{
-    if (addLocationWithoutStoring(location)) {
-        // Store the location permanently
-        m_settings->setValue("storedLocations", m_locations);
+{   
+    bool sync_settings = false;
+    //verify it the user had deleted it before and now is inserting it again
+    int indexRemoved = m_userRemovedLocations.indexOf(location);
+    if (indexRemoved > -1) {
+        m_userRemovedLocations.removeAt(indexRemoved);
+        m_settings->setValue(userRemovedLocationsName, m_userRemovedLocations);
+        sync_settings = true;
+    }
+    if (addLocationNotRemovedWithoutStoring(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(userSavedLocationsName, m_userSavedLocations);
+            sync_settings = true;
+        }                
+    }
+    if (sync_settings) {
+        m_settings->sync();
     }
 }
 
-bool PlacesModel::addLocationWithoutStoring(const QString &location)
+/*!
+ * \brief PlacesModel::addLocationNotRemovedWithoutStoring()
+ *
+ *  Add that location only if it was not removed before by the user.
+ *
+ *  When the user removes a location from Places using \ref removeItem(int index) it is stored in settings file.
+ *  The user must use \ref addLocation(const QString &location) to add back an already removed location.
+ *
+ * \param location
+ *
+ * \return true when the location was added (not existent in \a m_locations nor in \a m_userRemovedLocations),
+ *         otherwise false
+ */
+bool PlacesModel::addLocationNotRemovedWithoutStoring(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)) {
         // 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
@@ -270,7 +367,6 @@
         // Append the actual location
         m_locations.append(location);
 
-
         // Tell Qt we're done with modifying the model so that
         // it can update the UI and everything else to reflect
         // the new state
@@ -279,3 +375,16 @@
     }
     return false;
 }
+
+void PlacesModel::addDefaultLocation(const QString &location)
+{
+    // a Default location can be removed by the user
+    if (addLocationNotRemovedWithoutStoring(location)) {
+         m_defaultLocations.append(location);
+    }
+}
+
+void PlacesModel::removeItem(const QString &location)
+{
+    removeItem(m_locations.indexOf(location));
+}

=== modified file 'src/plugin/placesmodel/placesmodel.h'
--- src/plugin/placesmodel/placesmodel.h	2015-01-28 20:18:07 +0000
+++ src/plugin/placesmodel/placesmodel.h	2015-09-19 22:49:08 +0000
@@ -40,6 +40,7 @@
     Q_PROPERTY(QString locationMusic READ locationMusic CONSTANT)
     Q_PROPERTY(QString locationPictures READ locationPictures CONSTANT)
     Q_PROPERTY(QString locationVideos READ locationVideos CONSTANT)
+    Q_PROPERTY(QString locationSamba READ locationSamba CONSTANT)
 
 public:
     explicit PlacesModel(QObject *parent = 0);
@@ -50,7 +51,8 @@
     QString locationMusic() const;
     QString locationPictures() const;
     QString locationVideos() const;
-    int rowCount(const QModelIndex &parent) const override;
+    QString locationSamba() const;
+    int rowCount(const QModelIndex &parent = QModelIndex() ) const override;
     QVariant data(const QModelIndex &index, int role) const override;
     QHash<int, QByteArray> roleNames() const override;
 
@@ -61,20 +63,28 @@
 public slots:
     void addLocation(const QString &location);
     void removeItem(int indexToRemove);
-    inline bool isUserMountDirectory(const QString location) {
+    inline bool isUserMountDirectory(const QString& location) {
         return m_userMounts.contains(location);
     }
+    bool isDefaultLocation(const QString& location) const {
+        return m_defaultLocations.contains(location);
+    }
+    inline int indexOfLocation(const QString& location) const {
+        return m_locations.indexOf(location);
+    }
 
 private slots:
     void mtabChanged(const QString &path);
     void rescanMtab();
 
 private:
-    void initNewUserMountsWatcher();
-    // Returns true if location was not known before, and false if it was known
-    bool addLocationWithoutStoring(const QString &location);
+    void initNewUserMountsWatcher();    
+    bool addLocationNotRemovedWithoutStoring(const QString &location);
     // Returns true if location was not known before, and false if it was known
     void removeItemWithoutStoring(int itemToRemove);
+    //just add into m_locations, does not emit any signal
+    void addDefaultLocation(const QString& location);   
+    void removeItem(const QString& location);
 
     QMtabParser m_mtabParser;
     QStringList m_runtimeLocations;
@@ -82,10 +92,17 @@
     bool isMtabEntryUserMount(const QMtabEntry &entry) const;
     bool isSubDirectory(const QString &dir, const QString &path) const;
     QString standardLocation(QStandardPaths::StandardLocation location) const;
-    QStringList m_locations;
+    QStringList    m_locations;  //<! m_locations = m_defaultLocations + m_userSavedLocations - m_userRemovedLocations
+    QStringList    m_defaultLocations;
+    QStringList    m_userSavedLocations;
+    QStringList    m_userRemovedLocations;   
     QSettings *m_settings;
     QFileSystemWatcher *m_newUserMountsWatcher;
     QSet<QString> m_userMounts;
+    bool          m_going_to_rescanMtab;
+#if defined(REGRESSION_TEST_PLACES_MODEL)
+    friend class PlacesmodelTest;
+#endif
 };
 
 #endif // PLACESMODEL_H

=== added directory 'src/plugin/test_placesmodel'
=== added file 'src/plugin/test_placesmodel/placesmodeltest.cpp'
--- src/plugin/test_placesmodel/placesmodeltest.cpp	1970-01-01 00:00:00 +0000
+++ src/plugin/test_placesmodel/placesmodeltest.cpp	2015-09-19 22:49:08 +0000
@@ -0,0 +1,193 @@
+#include "placesmodel.h"
+
+#include <QString>
+#include <QtTest>
+#include <QSettings>
+#include <QFile>
+#include <QFileInfo>
+#include <QTemporaryFile>
+#include <QTemporaryDir>
+
+
+class SaveSettings: public QTemporaryFile
+{
+public:
+       SaveSettings(QObject *parent = 0) : QTemporaryFile(parent) {}
+       bool openReadOnly()
+       {
+           return open(QFile::ReadOnly);
+       }
+};
+
+class PlacesmodelTest : public QObject
+{
+    Q_OBJECT
+
+public:
+    PlacesmodelTest();
+
+private Q_SLOTS:
+    void init();
+    void cleanup();
+    void cleanupTestCase();
+    void addUserPlace();
+    void removeUserPlace();
+    void addExistentDefaultPlace(); // should fail
+    void removeDefaultPlace();
+    void addRemovedDefaultPlace();
+private:
+    SaveSettings *m_saved_settings;
+};
+
+
+PlacesmodelTest::PlacesmodelTest() : m_saved_settings(0)
+{
+
+}
+
+void PlacesmodelTest::init()
+{
+    PlacesModel places;
+    QFileInfo saved(places.m_settings->fileName());
+    if (saved.exists())
+    {
+        QFile settings(saved.absoluteFilePath());
+        QCOMPARE(settings.open(QFile::ReadOnly), true);
+        m_saved_settings = new SaveSettings(this);
+        QCOMPARE(m_saved_settings->open(), true);
+        QByteArray settings_data = settings.readAll();
+        QCOMPARE(m_saved_settings->write(settings_data), (qint64)settings_data.size());
+        m_saved_settings->close();
+    }
+}
+
+void PlacesmodelTest::cleanupTestCase()
+{
+    if (m_saved_settings)
+    {
+        QCOMPARE(m_saved_settings->openReadOnly(), true);
+        PlacesModel places;
+        QFile  saved(places.m_settings->fileName());
+        QCOMPARE(saved.open(QFile::WriteOnly | QFile::Truncate), true);
+        QByteArray saved_data = m_saved_settings->readAll();
+        QCOMPARE(saved.write(saved_data), (qint64)saved_data.size());
+    }
+}
+
+void PlacesmodelTest::cleanup()
+{
+    cleanupTestCase();
+}
+
+void PlacesmodelTest::addUserPlace()
+{
+    QTemporaryDir tempDir;
+    PlacesModel   places;
+    int  locations_counter = places.rowCount();
+    QCOMPARE(places.indexOfLocation(tempDir.path()), -1);
+    places.addLocation(tempDir.path());
+    QTest::qWait(50);
+    QCOMPARE(places.rowCount(), locations_counter + 1);
+    QVERIFY(places.indexOfLocation(tempDir.path()) != -1);
+
+    //now try to add it again which must fail
+    places.addLocation(tempDir.path());
+    QTest::qWait(50);
+    QCOMPARE(places.rowCount(), locations_counter + 1);
+
+    //another model instance
+    PlacesModel places2;
+    QVERIFY(places2.indexOfLocation(tempDir.path()) != -1);
+    //added item must be in m_userSavedLocations
+    QVERIFY(places2.m_userSavedLocations.indexOf(tempDir.path()) != -1);
+    QCOMPARE(places2.rowCount(), locations_counter + 1);
+}
+
+void PlacesmodelTest::removeUserPlace()
+{
+    // first add a temporary place
+    QTemporaryDir tempDir;
+    PlacesModel   places;
+    int  locations_counter = places.rowCount();
+    QCOMPARE(places.indexOfLocation(tempDir.path()), -1);
+    places.addLocation(tempDir.path());
+    QTest::qWait(50);
+    QCOMPARE(places.rowCount(), locations_counter + 1);
+    QVERIFY(places.indexOfLocation(tempDir.path()) != -1);
+    //then remove it
+    places.removeItem(tempDir.path());
+    QTest::qWait(50);
+    QCOMPARE(places.rowCount(), locations_counter);
+    QCOMPARE(places.indexOfLocation(tempDir.path()), -1);
+
+    //another PlacesModel instance
+    PlacesModel places2;
+    QCOMPARE(places2.rowCount(), locations_counter);
+    //item removed is not in the m_locations
+    QCOMPARE(places2.indexOfLocation(tempDir.path()), -1);
+    //item removed is in m_userRemovedLocations
+    QVERIFY(places2.m_userRemovedLocations.indexOf(tempDir.path()) != -1);
+}
+
+void PlacesmodelTest::addExistentDefaultPlace()
+{
+    PlacesModel places;
+    int home_index = places.indexOfLocation(QDir::homePath());
+    QVERIFY(home_index != -1);
+    int places_counter = places.rowCount();
+    places.addLocation(QDir::homePath());
+    QTest::qWait(50);
+    //counter must be the same which indicates nothing was added
+    QCOMPARE(places.rowCount(), places_counter);
+}
+
+void PlacesmodelTest::removeDefaultPlace()
+{
+    PlacesModel places;
+    int home_index = places.indexOfLocation(QDir::homePath());
+    QVERIFY(home_index != -1);
+    int places_counter = places.rowCount();
+    places.removeItem(home_index);
+    QTest::qWait(50);
+    QCOMPARE(places.rowCount(), places_counter - 1);
+    QCOMPARE(places.indexOfLocation(QDir::homePath()), -1);
+
+    //use another instance to check
+    PlacesModel places2;
+    QCOMPARE(places2.rowCount(), places_counter - 1);
+    QCOMPARE(places2.indexOfLocation(QDir::homePath()), -1);
+    //default place is in m_userRemovedLocations
+    QVERIFY(places2.m_userRemovedLocations.indexOf(QDir::homePath()) != -1);
+}
+
+void PlacesmodelTest::addRemovedDefaultPlace()
+{
+    //first remove a default place
+    PlacesModel places;
+    int home_index = places.indexOfLocation(QDir::homePath());
+    QVERIFY(home_index != -1);
+    int places_counter = places.rowCount();
+    places.removeItem(home_index);
+    QTest::qWait(50);
+    QCOMPARE(places.rowCount(), places_counter - 1);
+    QCOMPARE(places.indexOfLocation(QDir::homePath()), -1);
+
+    //now add the default location back
+    places.addLocation(QDir::homePath());
+    QTest::qWait(50);
+    QCOMPARE(places.rowCount(), places_counter);
+    QVERIFY(places.indexOfLocation(QDir::homePath()) != -1);
+    //it must not exist neither on m_userSavedLocations nor m_userRemovedLocations
+    QCOMPARE(places.m_userSavedLocations.indexOf(QDir::homePath()),   -1);
+    QCOMPARE(places.m_userRemovedLocations.indexOf(QDir::homePath()), -1);
+
+    //check on a second instance
+    PlacesModel places2;
+    QVERIFY(places2.indexOfLocation(QDir::homePath()) != -1);
+    QCOMPARE(places2.m_userSavedLocations.indexOf(QDir::homePath()),   -1);
+    QCOMPARE(places2.m_userRemovedLocations.indexOf(QDir::homePath()), -1);
+}
+
+QTEST_MAIN(PlacesmodelTest)
+
+#include "placesmodeltest.moc"

=== added file 'src/plugin/test_placesmodel/test_placesmodel.pro'
--- src/plugin/test_placesmodel/test_placesmodel.pro	1970-01-01 00:00:00 +0000
+++ src/plugin/test_placesmodel/test_placesmodel.pro	2015-09-19 22:49:08 +0000
@@ -0,0 +1,35 @@
+#-------------------------------------------------
+#
+# Project created by QtCreator 2015-09-05T17:37:49
+#
+#-------------------------------------------------
+
+QT       += testlib
+
+QT       -= gui
+
+TARGET = tst_placesmodeltest
+CONFIG   += console
+CONFIG   += testcase
+CONFIG   -= app_bundle
+
+TEMPLATE = app
+
+DEFINES += REGRESSION_TEST_PLACES_MODEL
+
+DEFINES += SRCDIR=\\\"$$PWD/\\\"
+
+QMAKE_CXXFLAGS += -std=c++11
+
+
+SOURCES += placesmodeltest.cpp \
+    ../placesmodel/placesmodel.cpp \
+    ../placesmodel/qmtabparser.cpp
+
+
+HEADERS += \    
+    ../placesmodel/placesmodel.h \
+    ../placesmodel/qmtabparser.h
+
+
+INCLUDEPATH += ../placesmodel