Merge lp:~carlos-mazieri/ubuntu-filemanager-app/app-devel-pre2 into lp:ubuntu-filemanager-app

[Carlos Jose Mazieri]

this is the second part of the https://code.launchpad.net/~carlos-mazieri/ubuntu-filemanager-app/app-devel/+merge/216409

summary: class ExternalFSWatcher changed to allow watching more than a path.
reason: It is intended to watch external changes in the Trash and Trash can be compounded for more than one path.

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

PASSED: Continuous integration, rev:170
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/192/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2345
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-raring-amd64-ci/192
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-saucy-amd64-ci/192
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-trusty-amd64-ci/142

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

review: Approve (continuous-integration)

[Alan Pope 🍺🐧🐱 🦄 (popey)]

I've asked Michael Spencer to take a look at this and the other related branches and review. If he has difficulty with that I've asked him to get back to me so we can find another reviewer.

[Arto Jalkanen (ajalkane)]

Function setCurrentPath(const QString &curPath) seems potentially confusing. ExternalFSWatcher allows watching several directories, and if that's the case and you try setting it to watch only one directory with setCurrentPath() then nothing happens (setCurrentPath does only changes if no paths watched or _only_ one path is watched, with several paths watched it seems to ignore the request).

m_setPath should probably be m_setPaths as it is a list of paths.

There might be something I don't properly understand about ExternalFSWatcher code. You can give it several paths to watch. But if there are changes happening at the same time at more than one path, the code seems to only send a notification of the change to the first. This can't be the intention?

externalfswatcher.h: The comments for the class still talk like it handles only one path. That should corrected.

review: Needs Information

[lp:~carlos-mazieri/ubuntu-filemanager-app/app-devel-pre2]

171. By Carlos Jose Mazieri on 2014-05-10

ExternalFSWatcher:
   * improved comments
   * m_setPath changed to m_setPaths
   * fixed bug about restoring paths in the slotFireChanges()

[Carlos Jose Mazieri (carlos-mazieri)]

This class used to watch just one path (the current on File Manager) that is why setPath() is a slot and it is called every time file manager changes its current path and emits DirModel::pathChanged(). For the normal path in the file system there will be always one path only. The idea here was to avoid notifications when the user changes the current path in the file system and lots of notifications when there is an Action going on which is causing lots of file system modifications.

It was changed to support a list of paths because of the Trash, again: Trash can show files from more than one Disk Trash, each mounted file system can have its own Disk Trash; all them (if more than one) are supposed to be watched.

The idea with more than one path is to notify the LAST (if more than one path was modified) not the first, see "m_lastChangedIndex". In the current implementation for the Trash it does not matter which Trash Disk path was modified, it gathers all them in a new DirItemInfoList and compares the content with the current content, see ExternalFileSystemChangesWorker::compareItems().

It should have a bug which I tried to fix, it used to not restore the entire path list. Now there is a comment in the ExternalFSWatcher::slotDirChanged() which may help a better understanding.

ExternalFSWatcher is used a little different for Trash, setCurrentPaths() will be called just once when entering in the Trash root, for sub-folders inside the Trash it is not used in the current implementation, TrashLocation::fetchItems() calls DiskLocation::stoptExternalFsWatcher() when it is not browsing Trash root items.

Current implementation keeps just one ExternalFSWatcher alive either for Normal File System or for Trash, when there will be support for other Locations like Network there will not be any instance of ExternalFSWatcher alive, see Location::startWorking() and Location::stopWorking() in the next MP.

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

FAILED: Continuous integration, rev:171
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/217/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2586
        deb: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2586/artifact/work/output/*zip*/output.zip
    FAILURE: http://91.189.93.70:8080/job/generic-mediumtests-utopic/125/console
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/125/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-trusty-amd64-ci/167
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-utopic-amd64-ci/15

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

review: Needs Fixing (continuous-integration)

[Arto Jalkanen (ajalkane)]

Thanks,

I probably (hope so) now understand it better. But still this setCurrentPath function perplexes me:

72 + if (m_setPaths.count() == 1 && m_setPaths.at(0) != curPath)
73 + {
74 + m_setPaths.removeFirst();
75 + }
76 + if (m_setPaths.count() == 0)
77 + {
78 + m_setPaths.append(curPath);
79 + QFileSystemWatcher::addPath(curPath);
80 + }

It seems like a bug? If a path has been set, and then directory is changed resulting in another path being set. It does only m_setPaths.removeFirst(). It does not call removing the path from QFileSystemWatcher. Then it adds the new path to be watched, so now it's watching two paths?

If setCurrentPaths() does not really notify of all the paths put there, but only of the last one changed, I think the intention should be documented a bit more. I think I understand the use case and it works as intended for it, but anyone looking only this piece of code would have hard time realizing this subtlety. I wonder if setCurrentPaths() could have a more descriptive function name. Or could the intention be better communicated by renaming the signal from pathModified(QString) to something like "atLeastOneOfPathsModified()"? Is the path argument needed in the signal? Quickly perusing the code I didn't see it being needed.

Let me know what you think, I might have not understood all the details here.

[Arto Jalkanen (ajalkane)]

Also, as how setCurrentPath currently works it seems one should not use setCurrentPath on an object where setCurrentPaths has been used. So it should at least be documented that only one or the other should be used on a created object.

Although I don't quite understand why setCurrentPath is that complex, couldn't you make it much simpler by doing for example only:

    clearPaths();
    m_setPaths.clear()
    m_setPaths.append(curPath);
    QFileSystemWatcher::addPath(curPath);

[Carlos Jose Mazieri (carlos-mazieri)]

> Also, as how setCurrentPath currently works it seems one should not use
> setCurrentPath on an object where setCurrentPaths has been used. So it should
> at least be documented that only one or the other should be used on a created
> object.
>
> Although I don't quite understand why setCurrentPath is that complex, couldn't
> you make it much simpler by doing for example only:
>
> clearPaths();
> m_setPaths.clear()
> m_setPaths.append(curPath);
> QFileSystemWatcher::addPath(curPath);

You're right it works when using it separated (I have a couple of tests for normal folders), when mixing setCurrentPath() and setCurrentPaths() in the same object it will not work.

I will improve that code as you suggested.
Thanks.

[lp:~carlos-mazieri/ubuntu-filemanager-app/app-devel-pre2]

172. By Carlos Jose Mazieri on 2014-05-13

* ExternalFSWatcher::setCurrentPath() improved/simplified
* created documentation for ExternalFSWatcher::slotDirChanged()
* improved documentation of ExternalFSWatcher::slotFireChanges()
* improved documentation of ExternalFSWatcher class in externalfswatcher.h

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

PASSED: Continuous integration, rev:172
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/223/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2623
        deb: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2623/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/160
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/160/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-trusty-amd64-ci/173
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-utopic-amd64-ci/21

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

review: Approve (continuous-integration)

[Carlos Jose Mazieri (carlos-mazieri)]

Some improvement provided.

[Arto Jalkanen (ajalkane)]

Looks good to me now, thanks!

review: Approve

=== modified file 'src/plugin/folderlistmodel/dirmodel.cpp'
--- src/plugin/folderlistmodel/dirmodel.cpp	2014-05-01 17:38:23 +0000
+++ src/plugin/folderlistmodel/dirmodel.cpp	2014-05-13 23:29:05 +0000
@@ -1189,8 +1189,8 @@
         connect(this,          SIGNAL(pathChanged(QString)),
                 mExtFSWatcher, SLOT(setCurrentPath(QString)));
 
-        connect(mExtFSWatcher, SIGNAL(pathModified()),
-                this,          SLOT(onThereAreExternalChanges()));
+        connect(mExtFSWatcher, SIGNAL(pathModified(QString)),
+                this,          SLOT(onThereAreExternalChanges(QString)));
 
        //setCurrentPath() checks for empty paths
        mExtFSWatcher->setCurrentPath(mCurrentDir);
@@ -1216,13 +1216,15 @@
 }
 
 
-void DirModel::onThereAreExternalChanges()
+void DirModel::onThereAreExternalChanges(const QString& pathModifiedOutside)
 {
     if ( IS_FILE_MANAGER_IDLE() )
     {
 #if DEBUG_EXT_FS_WATCHER
         qDebug() << "[extFsWatcher]" << QDateTime::currentDateTime().toString("hh:mm:ss.zzz")
-                 << Q_FUNC_INFO << this << "File System modified";
+                 << Q_FUNC_INFO << this << "File System modified" << pathModifiedOutside;
+#else
+  Q_UNUSED(pathModifiedOutside);
 #endif
         DirListWorker *w =
                 createWorkerRequest(IORequest::DirListExternalFSChanges,

=== modified file 'src/plugin/folderlistmodel/dirmodel.h'
--- src/plugin/folderlistmodel/dirmodel.h	2014-05-01 17:38:23 +0000
+++ src/plugin/folderlistmodel/dirmodel.h	2014-05-13 23:29:05 +0000
@@ -392,7 +392,7 @@
     void          onItemAddedOutsideFm(const DirItemInfo&fi);
     void          onItemRemovedOutSideFm(const DirItemInfo&);
     void          onItemChangedOutSideFm(const DirItemInfo&fi);
-    void          onThereAreExternalChanges();
+    void          onThereAreExternalChanges(const QString &);
     void          onExternalFsWorkerFinished(int);
 
 

=== modified file 'src/plugin/folderlistmodel/externalfswatcher.cpp'
--- src/plugin/folderlistmodel/externalfswatcher.cpp	2013-11-17 14:27:10 +0000
+++ src/plugin/folderlistmodel/externalfswatcher.cpp	2014-05-13 23:29:05 +0000
@@ -28,7 +28,7 @@
 #if DEBUG_EXT_FS_WATCHER
 # define DEBUG_FSWATCHER()    \
     qDebug() << "[extFsWatcher]" << QDateTime::currentDateTime().toString("hh:mm:ss.zzz") \
-             << Q_FUNC_INFO << "m_setPath:" << m_setPath \
+             << Q_FUNC_INFO << "m_setPath:" << m_setPaths \
              << "m_changedPath:" << m_changedPath        \
              << "m_waitingEmit:" << m_waitingEmitCounter
 #else
@@ -40,6 +40,7 @@
     QFileSystemWatcher(parent)
   , m_waitingEmitCounter(0)
   , m_msWaitTime(DEFAULT_NOTICATION_PERIOD)
+  , m_lastChangedIndex(-1)
 {
     connect(this,   SIGNAL(directoryChanged(QString)),
             this,   SLOT(slotDirChanged(QString)));
@@ -48,32 +49,59 @@
 
 void ExternalFSWatcher::setCurrentPath(const QString &curPath)
 {
-    if (!curPath.isEmpty())
+    if (!curPath.isEmpty() && (m_setPaths.count() != 1 || m_setPaths.at(0) != curPath))
     {
-        if (m_setPath != curPath)
-        {
-            if (!m_setPath.isEmpty())
-            {
-                removePath(m_setPath);
-            }
-            m_setPath = curPath;
-            addPath(m_setPath);
-        }        
+        clearPaths();
+        m_setPaths.clear();
+        m_setPaths.append(curPath);
+        QFileSystemWatcher::addPath(curPath);
     }
     DEBUG_FSWATCHER();
 }
 
 
+void ExternalFSWatcher::setCurrentPaths(const QStringList &paths)
+{
+    QStringList myPaths(paths);
+    ::qSort(myPaths);
+    clearPaths();
+    m_setPaths = myPaths;
+    QFileSystemWatcher::addPaths(paths);
+}
+
+
+void ExternalFSWatcher::clearPaths()
+{
+    QStringList existentPaths = QFileSystemWatcher::directories();
+    if (existentPaths.count() > 0)
+    {
+        QFileSystemWatcher::removePaths(existentPaths);
+    }
+}
+
+
+/*!
+ * \brief ExternalFSWatcher::slotDirChanged() schedules a Disk change to be notified
+ *
+ *  Once path that belongs to \a m_setPaths is modified in the Disk it becomes the \a m_changedPath and
+ *  its change is scheculed to notified later. This path is taken out from QFileSystemWatcher to avoid
+ *  lots of continuous notifications from QFileSystemWatcher when having hevy disk io.
+ *
+ * \param dir directory changed in the File System
+ */
 void ExternalFSWatcher::slotDirChanged(const QString &dir)
 {
     DEBUG_FSWATCHER();
-    if (    (m_setPath == dir)
-         && ( m_waitingEmitCounter == 0 || m_setPath != m_changedPath )
-       )
+    int index = m_setPaths.indexOf(dir);
+    if (index != -1  && (m_waitingEmitCounter == 0 || dir != m_changedPath))
     {
-        removePath(m_setPath);
+        m_lastChangedIndex = index;
+        //changed path is taken from the QFileSystemWatcher and it becomes the current changed
+        //in this case there will not be slotDirChanged() for this path until slotFireChanges()
+        //restores the path in the QFileSystemWatcher
+        removePath(m_setPaths.at(m_lastChangedIndex));
         ++m_waitingEmitCounter;
-        m_changedPath = m_setPath;
+        m_changedPath = dir;
         QTimer::singleShot(m_msWaitTime, this, SLOT(slotFireChanges()));       
     }
 }
@@ -81,22 +109,28 @@
 
 /*!
  * \brief ExternalFSWatcher::slotFireChanges() emits \ref pathModified() only when it is sure
- *  that the  current path was changed.
- *
- *  A change for the current path (the last current) MUST be notified at least once.
+ *  that the LAST current path was changed.
+ *
+ *  The notification will be sent out only for the LAST modified path (if more than one) from the \a m_setPaths
+ *
+ *  \sa \ref ExternalFSWatcher class
  */
 void ExternalFSWatcher::slotFireChanges()
 {
-   if (--m_waitingEmitCounter == 0)
-   {
-       addPath(m_setPath);
-       if (m_setPath == m_changedPath)
-       {
-           emit pathModified();
+   if (   --m_waitingEmitCounter == 0
+       && m_lastChangedIndex != -1
+       && m_lastChangedIndex < m_setPaths.count() )
+   {            
+       if (m_setPaths.at(m_lastChangedIndex) == m_changedPath)
+       {          
+           emit pathModified(m_changedPath);
 #if DEBUG_EXT_FS_WATCHER
        DEBUG_FSWATCHER() << "emit pathModified()";
 #endif
        }
+       //restore the original list in QFileSystemWatcher
+       clearPaths();
+       QFileSystemWatcher::addPaths(m_setPaths);
    }  
 }
 

=== modified file 'src/plugin/folderlistmodel/externalfswatcher.h'
--- src/plugin/folderlistmodel/externalfswatcher.h	2013-10-20 16:58:41 +0000
+++ src/plugin/folderlistmodel/externalfswatcher.h	2014-05-13 23:29:05 +0000
@@ -23,22 +23,30 @@
 #define EXTERNALFSWATCHER_H
 
 #include <QFileSystemWatcher>
+#include <QStringList>
 
 #define DEFAULT_NOTICATION_PERIOD  500
 
 
 /*!
- * \brief The ExternalFSWatcher class notifies the owner when the File System when the current path \a m_setPath has changed
- *            emitting pathModified() signal.
- *
- *  The current path \a m_setPath is set by using the slot \ref  setCurrentPath()
- *
- *  The idea of this class is to minimize notifications as the current path can change quickly.
+ * \brief The ExternalFSWatcher class watches for external changes in Disk emitting pathModified() signal.
+ *
+ *  The path(s) being watched  is/are set by using the slot \ref  setCurrentPath() or \ref setCurrentPaths()
+ *
+ *  The idea of this class is to minimize notifications as the current path in the File Manager can change quickly.
  *  A notification will occur if it was requested for a path and this path is still the current at the moment
  *  of the notification.
  *
- *  Once it detects a change it will wait \ref getIntervalToNotifyChanges() milliseconds to notify that change.
- *  At this moment it checks if no \ref setCurrentPath() has been called during this time and then notifies that change.
+ *  Once it detects a Disk change it will wait \ref getIntervalToNotifyChanges() milliseconds to notify that change.
+ *  During the time it waits:
+ *    \li  the notified path will NOT be watched until pathModified() is emitted
+ *    \li  another call to \ref setCurrentPath() or \ref setCurrentPaths() invalidades the current change,
+ *         that mean the signal pathModified() will NOT be emitted.
+ *
+ * \note When more than one path is being watched by using \ref setCurrentPaths() and changes happen in
+ *       more than one path before the getIntervalToNotifyChanges() expires, only the LAST path modified
+ *       will be notified as changed. It may possible that it goes to a loop if all the paths were modified,
+ *       but the loop finishes when the last one from the list is modified.
  */
 class ExternalFSWatcher : public QFileSystemWatcher
 {
@@ -47,22 +55,29 @@
     explicit ExternalFSWatcher(QObject *parent = 0);
     int      getIntervalToNotifyChanges() const;
 
+    inline const QStringList& pathsWatched() const { return m_setPaths;}
+
 signals:
-     void      pathModified();
+     void      pathModified(const QString& path);
 
-     public slots:
+public slots:
      void      setCurrentPath(const QString& curPath);
+     void      setCurrentPaths(const QStringList& paths);
      void      setIntervalToNotifyChanges(int ms);     
 
 private slots:
      void      slotDirChanged(const QString&);
      void      slotFireChanges();
 
- private:
-     QString   m_setPath;
-     QString   m_changedPath;   
-     unsigned  m_waitingEmitCounter;
-     int       m_msWaitTime;
+private:
+     void      clearPaths();
+
+private:
+     QStringList m_setPaths;
+     QString     m_changedPath;
+     unsigned    m_waitingEmitCounter;
+     int         m_msWaitTime;
+     int         m_lastChangedIndex;
 };
 
 #endif // EXTERNALFSWATCHER_H

=== modified file 'src/plugin/test_folderlistmodel/regression/tst_folderlistmodel.cpp'
--- src/plugin/test_folderlistmodel/regression/tst_folderlistmodel.cpp	2014-03-03 17:19:49 +0000
+++ src/plugin/test_folderlistmodel/regression/tst_folderlistmodel.cpp	2014-05-13 23:29:05 +0000
@@ -77,7 +77,7 @@
     void cancel(int index, int, int percent);
     void slotclipboardChanged();
     void slotError(QString title, QString message);
-    void slotExtFsWatcherPathModified()     { ++m_extFSWatcherPathModifiedCounter; }
+    void slotExtFsWatcherPathModified(const QString&)     { ++m_extFSWatcherPathModifiedCounter; }
     void slotSelectionChanged(int counter)  { m_selectedItemsCounter = counter; }
     void slotSelectionModeChanged(int m)    { m_selectionMode = m;}
 
@@ -1859,8 +1859,8 @@
 void TestDirModel::extFsWatcherChangePathManyTimesModifyAllPathsLessLast()
 {
     ExternalFSWatcher  watcher;
-    connect(&watcher, SIGNAL(pathModified()),
-            this,     SLOT(slotExtFsWatcherPathModified()));
+    connect(&watcher, SIGNAL(pathModified(QString)),
+            this,     SLOT(slotExtFsWatcherPathModified(QString)));
 
     const int items = 150;
     QVector<DeepDir *>  deepDirs;
@@ -1896,8 +1896,8 @@
 void TestDirModel::extFsWatcherChangePathManyTimesModifyManyTimes()
 {
     ExternalFSWatcher  watcher;
-    connect(&watcher, SIGNAL(pathModified()),
-            this,     SLOT(slotExtFsWatcherPathModified()));
+    connect(&watcher, SIGNAL(pathModified(QString)),
+            this,     SLOT(slotExtFsWatcherPathModified(QString)));
 
     const int items = 50;
     QVector<DeepDir *>  deepDirs;
@@ -1930,8 +1930,8 @@
 void TestDirModel::extFsWatcherModifySamePathManyTimesWithInInterval()
 {
     ExternalFSWatcher  watcher;
-    connect(&watcher, SIGNAL(pathModified()),
-            this,     SLOT(slotExtFsWatcherPathModified()));
+    connect(&watcher, SIGNAL(pathModified(QString)),
+            this,     SLOT(slotExtFsWatcherPathModified(QString)));
 
     QString dirName("extFsWatcher_expects_just_one_signal");
     m_deepDir_01 = new DeepDir(dirName,0);
@@ -1957,8 +1957,8 @@
 void TestDirModel::extFsWatcherSetPathAndModifyManyTimesWithInInterval()
 {
     ExternalFSWatcher  watcher;
-    connect(&watcher, SIGNAL(pathModified()),
-            this,     SLOT(slotExtFsWatcherPathModified()));
+    connect(&watcher, SIGNAL(pathModified(QString)),
+            this,     SLOT(slotExtFsWatcherPathModified(QString)));
 
     QList<DeepDir *>  deepDirs;
 
@@ -1995,8 +1995,8 @@
 {
     bool updateAndSetModificationTime(const QString& filename, QDateTime& desiredTime);
 
-    connect( m_dirModel_01->mExtFSWatcher, SIGNAL(pathModified()),
-            this,     SLOT(slotExtFsWatcherPathModified()));
+    connect( m_dirModel_01->getExternalFSWatcher(), SIGNAL(pathModified(QString)),
+            this,     SLOT(slotExtFsWatcherPathModified(QString)));
 
     QString dirName("extFsWatcher_generate_fileswithsameTimeStamp");
     m_deepDir_01 = new DeepDir(dirName,0);
@@ -2029,7 +2029,7 @@
     QFileInfo firstFile(firstPathName);
     QCOMPARE(firstFile.lastModified(),  timeStamp);
 
-    const int maxWait   = m_dirModel_01->mExtFSWatcher->getIntervalToNotifyChanges() + 10;
+    const int maxWait   = m_dirModel_01->getExternalFSWatcher()->getIntervalToNotifyChanges() + 10;
     int counter = 0;
 
     //make sure ExternalFSWatcher has not notified any change