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

[Carlos Jose Mazieri]

Some DirItemInfo::isDir() changed by DirItemInfo::isBrowasable()
Renamed DirModel::cdInto() to DirModel::cdIntoItem()
Improved DirModel::cdIntoIndex() and DirModel::cdIntoPath()

Some DirItemInfo::isDir() changed by DirItemInfo::isBrowasable()
Renamed DirModel::cdInto() to DirModel::cdIntoItem()
Improved DirModel::cdIntoIndex() and DirModel::cdIntoPath()

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

PASSED: Continuous integration, rev:387
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/468/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2303
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/2303/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-utopic-amd64-ci/184
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-vivid-amd64-ci/90

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

review: Approve (continuous-integration)

[Arto Jalkanen (ajalkane)]

review: Needs Information

[Arto Jalkanen (ajalkane)]

Please see inlined comment with my question

[Carlos Jose Mazieri (carlos-mazieri)]

The code already in place prevents this by setting the last item to null before parsing a new url/path, see http://bazaar.launchpad.net/~carlos-mazieri/ubuntu-filemanager-app/samba-browsing-02/view/head:/src/plugin/folderlistmodel/locationsfactory.cpp#L109 and http://bazaar.launchpad.net/~carlos-mazieri/ubuntu-filemanager-app/samba-browsing-02/view/head:/src/plugin/folderlistmodel/locationsfactory.cpp#L181.

The idea of LocationsFactory::lastValidFileInfo() is: Every time a new url/path is entered in DirModel::setPath() it is parsed by LocationsFactory, when it is browsable (it is a directory or host or workgroup) it is opened and items are fetched, when it is not browsable but exits it is saved and it is active until next url/path be parsed.
I think this does not change the behaviour of the current version, except that it will try to open files while ubuntu filemanager uses content hub to open files, I thought about creating a flag/property "openFiles" which defaults to "false" and it will be checked before trying to open files. Let me know if you agree with this so I can add this.

I tested that use case again to make sure it does not happen.

[Arto Jalkanen (ajalkane)]

I do not understand this, but I trust your explanation. I will approve it now.

review: Approve

=== modified file 'src/plugin/folderlistmodel/dirmodel.cpp'
--- src/plugin/folderlistmodel/dirmodel.cpp	2015-03-08 12:04:13 +0000
+++ src/plugin/folderlistmodel/dirmodel.cpp	2015-03-08 12:04:13 +0000
@@ -881,13 +881,9 @@
 bool  DirModel::cdIntoIndex(int row)
 {
     bool ret = false;
-    if (IS_VALID_ROW(row)                       &&
-        mDirectoryContents.at(row).isDir()      &&
-        mDirectoryContents.at(row).isContentReadable())
+    if (IS_VALID_ROW(row))
     {
-        mCurLocation->setInfoItem(mDirectoryContents.at(row));
-        setPathFromCurrentLocation();
-        ret = true;
+        ret = cdIntoItem(mDirectoryContents.at(row));
     }
     else
     {
@@ -896,42 +892,29 @@
     return ret;
 }
 
-
+/*!
+ * \brief DirModel::cdIntoPath()  It is used to go into an item from the current path or to a absolute path
+ * \param filename
+ * \return
+ */
 bool  DirModel::cdIntoPath(const QString &filename)
 {
-    bool ret = false;
-    DirItemInfo fi(filename);
-    if (fi.isValid())
-    {
-        if (fi.isRelative())
-        {
-            fi.setFile(mCurrentDir, filename);
-        }
-        ret =  cdInto(fi);
-    }
-    return ret;
+    return openPath(filename);
 }
 
 
-bool  DirModel::cdInto(const DirItemInfo &fi)
+bool  DirModel::cdIntoItem(const DirItemInfo &fi)
 {
     bool ret = false;
-    if (canReadDir(fi.diskFileInfo()))
-    {
-        if (fi.isRelative())
-        {
-            QDir childDir(mCurrentDir);
-            if ( childDir.cd(fi.fileName()) )
-            {
-                setPath(childDir.absolutePath());
-                ret = true;
-            }
-        }
-        else
-        {
+    if (fi.isBrowsable())
+    {       
+        if (fi.isContentReadable())
+        {
+            mCurLocation->setInfoItem(fi);
+            setPathFromCurrentLocation();
             ret = true;
-            setPath(fi.absoluteFilePath());
         }
+
     }
     return ret;
 }
@@ -1282,7 +1265,7 @@
     bool ret = false;
     if (IS_VALID_ROW(row))
     {
-        if (mDirectoryContents.at(row).isDir())
+        if (mDirectoryContents.at(row).isBrowsable())
         {
             ret = cdIntoIndex(row);
         }
@@ -1298,27 +1281,35 @@
     return ret;
 }
 
+
 bool DirModel::openPath(const QString &filename)
 {
     bool ret = false;
-    //first void any relative path when is root
-    if ( !(mCurLocation && mCurLocation->isRoot() && filename.startsWith(QLatin1String(".."))) )
+    QString myFilename(filename.trimmed());
+    //first avoid any relative path when is root
+    if ( !(mCurLocation && mCurLocation->isRoot() && myFilename.startsWith(QLatin1String(".."))) )
     {
-        Location *location = mLocationFactory->setNewPath(filename);
-        if (location)
+        if (myFilename == QLatin1String("..") || myFilename == QLatin1String("../"))
         {
-            mCurLocation = location;
-            setPathFromCurrentLocation();
-            ret = true;
+            ret = cdUp();
         }
         else
         {
-           const DirItemInfo *item = mLocationFactory->lastValidFileInfo();
-           // DirItemInfo fi(setParentIfRelative(filename));
-           if (item && item->isFile())
-           {
-               ret =  openItem(*item);
-           }
+            Location *location = mLocationFactory->setNewPath(myFilename);
+            if (location)
+            {
+                mCurLocation = location;
+                setPathFromCurrentLocation();
+                ret = true;
+            }
+            else
+            {
+                const DirItemInfo *item = mLocationFactory->lastValidFileInfo();               
+                if (item && item->isFile())
+                {
+                    ret =  openItem(*item);
+                }
+            }
         }
     }
     return ret;
@@ -1332,19 +1323,16 @@
 bool DirModel::openItem(const DirItemInfo &fi)
 {
     bool ret = false;
-    if (fi.isLocal())
-    {
-        if (canReadDir(fi.diskFileInfo()))
-        {
-            ret = cdInto(fi.diskFileInfo());
-        }
-        else
-        {
-            //TODO open executables
-            if (canReadFile(fi.diskFileInfo()))
-            {
-                ret = QDesktopServices::openUrl(QUrl::fromLocalFile(fi.absoluteFilePath()));
-            }
+    if (fi.isBrowsable())
+    {
+        ret = cdIntoItem(fi);
+    }
+    else
+    {
+        //TODO open executables
+        if (fi.isLocal() && fi.isReadable())
+        {
+            ret = QDesktopServices::openUrl(QUrl::fromLocalFile(fi.absoluteFilePath()));
         }
     }
     return ret;

=== modified file 'src/plugin/folderlistmodel/dirmodel.h'
--- src/plugin/folderlistmodel/dirmodel.h	2015-03-08 12:04:13 +0000
+++ src/plugin/folderlistmodel/dirmodel.h	2015-03-08 12:04:13 +0000
@@ -459,7 +459,7 @@
     int           rowOfItem(const DirItemInfo& fi);
     QDir::Filter  currentDirFilter()  const;
     QString       dirItems(const DirItemInfo& fi) const;
-    bool          cdInto(const DirItemInfo& fi);
+    bool          cdIntoItem(const DirItemInfo& fi);
     bool          openItem(const DirItemInfo& fi);  
     bool          canReadDir(const QFileInfo& d)   const;
     bool          canReadFile(const QFileInfo& f)  const;