ubuntu-touch-coreapps-reviewers team mailing list archive
-
ubuntu-touch-coreapps-reviewers team
-
Mailing list archive
-
Message #04801
[Merge] lp:~carlos-mazieri/ubuntu-filemanager-app/samba-ui-02 into lp:ubuntu-filemanager-app
Carlos Jose Mazieri has proposed merging lp:~carlos-mazieri/ubuntu-filemanager-app/samba-ui-02 into lp:ubuntu-filemanager-app with lp:~carlos-mazieri/ubuntu-filemanager-app/samba-ui-01 as a prerequisite.
Commit message:
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.
Requested reviews:
Ubuntu File Manager Developers (ubuntu-filemanager-dev)
For more details, see:
https://code.launchpad.net/~carlos-mazieri/ubuntu-filemanager-app/samba-ui-02/+merge/270335
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.
--
Your team Ubuntu File Manager Developers is requested to review the proposed merge of lp:~carlos-mazieri/ubuntu-filemanager-app/samba-ui-02 into lp:ubuntu-filemanager-app.
=== 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
@@ -15,6 +15,7 @@
*
* Author : David Planella <david.planella@xxxxxxxxxx>
* Arto Jalkanen <ajalkane@xxxxxxxxx>
+ * Carlos Mazieri <carlos.mazieri@xxxxxxxxx>
*/
#include "placesmodel.h"
@@ -27,12 +28,14 @@
PlacesModel::PlacesModel(QObject *parent) :
QAbstractListModel(parent)
+ , m_userSavedLocationsName("userSavedLocations")
+ , 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) {
+ 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() {
@@ -73,17 +90,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 +120,7 @@
void
PlacesModel::rescanMtab() {
+ m_going_to_rescanMtab = false;
const QString& path = m_mtabParser.path();
qDebug() << Q_FUNC_INFO << "rescanning mtab" << path;
@@ -203,6 +233,11 @@
return standardLocation(QStandardPaths::MoviesLocation);
}
+QString PlacesModel::locationSamba() const
+{
+ return QLatin1Literal("smb://");
+}
+
int PlacesModel::rowCount(const QModelIndex &parent) const
{
Q_UNUSED(parent)
@@ -227,10 +262,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(m_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(m_userRemovedLocationsName, m_userRemovedLocations);
+ sync_settings = true;
+ }
+ removeItemWithoutStoring(indexToRemove);
+ if (sync_settings) {
+ m_settings->sync();
+ }
+ }
}
void PlacesModel::removeItemWithoutStoring(int indexToRemove)
@@ -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;
+ //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(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)) {
// 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
@@ -279,3 +351,16 @@
}
return false;
}
+
+void PlacesModel::addDefaultLocation(const QString &location)
+{
+ // a Default location can be removed by the user
+ if (addLocationWithoutStoring(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-07 21:27:29 +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,9 +63,15 @@
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);
@@ -75,6 +83,9 @@
bool addLocationWithoutStoring(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 +93,19 @@
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;
+ QLatin1String m_userSavedLocationsName;
+ QLatin1String m_userRemovedLocationsName;
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-07 21:27:29 +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-07 21:27:29 +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
Follow ups
