Merge lp:~carlos-mazieri/ubuntu-filemanager-app/trash-operations-1 into lp:ubuntu-filemanager-app

[Carlos Jose Mazieri]

created ActionPaths structure

ActionPaths will be used for handling Actions,
it provides common helper functions for source and target paths or urls.

Currently Actions are supposed to have just one source parent folder and one target parent path as copy/cut are made from just one place and paste puts items on just one place.
These items are stored on FileSystemAction::Action::origPath and FileSystemAction::Action::targetPath.

Restoring items from Trash requires items to be moved to different places, so an ActionPaths will be used in the FileSystemAction::ActionEntry which makes items to be independent from the main FileSystemAction::Action once they have their own source/target paths information.

It also provides ActionPaths::toggle() which is intended for Undo Actions.

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

PASSED: Continuous integration, rev:185
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/234/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2679
        deb: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2679/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/216
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/216/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-trusty-amd64-ci/184
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-utopic-amd64-ci/32

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

review: Approve (continuous-integration)

[Arto Jalkanen (ajalkane)]

1)

Why this, pathLen is local variable and never used after this:

32 + else
33 + { //avoid bad index
34 + pathLen = 0;
35 + }

2)

This is a problematic function: inline void setSource(const QString& source)

Consider if it's been called like this:

setSource("/path/to/file");
...
setSource("aFile")

Then we have QStringRef's that will not be valid and probably causes undefined behaviour if I've understood QStringRef's documentation correctly.

review: Needs Fixing

[Carlos Jose Mazieri (carlos-mazieri)]

> 1)
>
> Why this, pathLen is local variable and never used after this:
>
> 32 + else
> 33 + { //avoid bad index
> 34 + pathLen = 0;
> 35 + }

This is a mistake, from some previous versions not presented here pathLen was not local.
I will remove this.

> 2)
>
> This is a problematic function: inline void setSource(const QString& source)
>
> Consider if it's been called like this:
>
> setSource("/path/to/file");
> ...
> setSource("aFile")
>
> Then we have QStringRef's that will not be valid and probably causes undefined
> behaviour if I've understood QStringRef's documentation correctly.

I understand that you are asking about relative paths, for absolute paths like setSource("/path/to/file") it is supposed to work.

If so, regarding relative paths it would not work, but it states in the beginning of the file:
        "Paths stored here are supposed to NOT be relative."
In the current implementation it is only used with absolute paths.
Let me know if it is acceptable.

[Arto Jalkanen (ajalkane)]

> > 1)
> >
> > Why this, pathLen is local variable and never used after this:
> >
> > 32 + else
> > 33 + { //avoid bad index
> > 34 + pathLen = 0;
> > 35 + }
>
> This is a mistake, from some previous versions not presented here pathLen was
> not local.
> I will remove this.
>
>
>
> > 2)
> >
> > This is a problematic function: inline void setSource(const QString& source)
> >
> > Consider if it's been called like this:
> >
> > setSource("/path/to/file");
> > ...
> > setSource("aFile")
> >
> > Then we have QStringRef's that will not be valid and probably causes
> undefined
> > behaviour if I've understood QStringRef's documentation correctly.
>
> I understand that you are asking about relative paths, for absolute paths like
> setSource("/path/to/file") it is supposed to work.
>
> If so, regarding relative paths it would not work, but it states in the
> beginning of the file:
> "Paths stored here are supposed to NOT be relative."
> In the current implementation it is only used with absolute paths.
> Let me know if it is acceptable.

I'd ask that the function setSource() will set or reset _sFile and _origPath variables whenever _source variable changes. While it may not be problem in how it's currently used, it is a potentially hard to find error in future if the implementation changes.

So in the current merge request it could be something like:

if (pathLen != -1)
{
    _sFile = QStringRef(&_source, pathLen + 1, _source.size() - pathLen - 1);
    _origPath = QStringRef(&_source, 0, pathLen);
}
else
{ //avoid bad index
    pathLen = 0;
    _sFile = QStringRef();
    _origPath = QStringRef();
}

That ways there's no possibility using _sFile or _origPath results in memory corruption however setSource() function is called.

[lp:~carlos-mazieri/ubuntu-filemanager-app/trash-operations-1]

186. By Carlos Jose Mazieri on 2014-05-18

improved ActionPaths::setSource(), prevent possible memory corruption if using relative paths even relative paths are not supported.

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

PASSED: Continuous integration, rev:186
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/239/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2685
        deb: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2685/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/222
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/222/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-trusty-amd64-ci/189
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-utopic-amd64-ci/37

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

review: Approve (continuous-integration)

[Carlos Jose Mazieri (carlos-mazieri)]

Revision 186 addresses requests 1) and 2)

[Carlos Jose Mazieri (carlos-mazieri)]

> > > 1)
> > >
> > > Why this, pathLen is local variable and never used after this:
> > >
> > > 32 + else
> > > 33 + { //avoid bad index
> > > 34 + pathLen = 0;
> > > 35 + }
> >
> > This is a mistake, from some previous versions not presented here pathLen
> was
> > not local.
> > I will remove this.
> >
> >
> >
> > > 2)
> > >
> > > This is a problematic function: inline void setSource(const QString&
> source)
> > >
> > > Consider if it's been called like this:
> > >
> > > setSource("/path/to/file");
> > > ...
> > > setSource("aFile")
> > >
> > > Then we have QStringRef's that will not be valid and probably causes
> > undefined
> > > behaviour if I've understood QStringRef's documentation correctly.
> >
> > I understand that you are asking about relative paths, for absolute paths
> like
> > setSource("/path/to/file") it is supposed to work.
> >
> > If so, regarding relative paths it would not work, but it states in the
> > beginning of the file:
> > "Paths stored here are supposed to NOT be relative."
> > In the current implementation it is only used with absolute paths.
> > Let me know if it is acceptable.
>
> I'd ask that the function setSource() will set or reset _sFile and _origPath
> variables whenever _source variable changes. While it may not be problem in
> how it's currently used, it is a potentially hard to find error in future if
> the implementation changes.
>
> So in the current merge request it could be something like:
>
> if (pathLen != -1)
> {
> _sFile = QStringRef(&_source, pathLen + 1, _source.size() - pathLen -
> 1);
> _origPath = QStringRef(&_source, 0, pathLen);
> }
> else
> { //avoid bad index
> pathLen = 0;
> _sFile = QStringRef();
> _origPath = QStringRef();

Thinking a little more about it, it looks like the two lines above do not have any effect.
Both "_sFile" and "_origPath" were already initialized with QStringRef default constructor.

I will change for that:

 inline void setSource(const QString& source)
 {
        int pathLen = source.lastIndexOf(QDir::separator());
        if (pathLen != -1)
        {
           _source = source;
           _sFile = QStringRef(&_source, pathLen + 1, _source.size() - pathLen - 1);
           _origPath = QStringRef(&_source, 0, pathLen);
        }
        else
        { // using ./source
            setSource(QString(".") + QDir::separator() + source);
        }
    }

> }
>
> That ways there's no possibility using _sFile or _origPath results in memory
> corruption however setSource() function is called.

[lp:~carlos-mazieri/ubuntu-filemanager-app/trash-operations-1]

187. By Carlos Jose Mazieri on 2014-05-18

ActionPaths::setSource(), forcing current path if using relative paths which is not supported.

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

PASSED: Continuous integration, rev:187
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/240/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2686
        deb: http://91.189.93.70:8080/job/generic-mediumtests-trusty/2686/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-utopic/223
        deb: http://91.189.93.70:8080/job/generic-mediumtests-utopic/223/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-trusty-amd64-ci/190
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-utopic-amd64-ci/38

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

review: Approve (continuous-integration)

[Arto Jalkanen (ajalkane)]

> > > > 1)
> > > >
> > > > Why this, pathLen is local variable and never used after this:
> > > >
> > > > 32 + else
> > > > 33 + { //avoid bad index
> > > > 34 + pathLen = 0;
> > > > 35 + }
> > >
> > > This is a mistake, from some previous versions not presented here pathLen
> > was
> > > not local.
> > > I will remove this.
> > >
> > >
> > >
> > > > 2)
> > > >
> > > > This is a problematic function: inline void setSource(const QString&
> > source)
> > > >
> > > > Consider if it's been called like this:
> > > >
> > > > setSource("/path/to/file");
> > > > ...
> > > > setSource("aFile")
> > > >
> > > > Then we have QStringRef's that will not be valid and probably causes
> > > undefined
> > > > behaviour if I've understood QStringRef's documentation correctly.
> > >
> > > I understand that you are asking about relative paths, for absolute paths
> > like
> > > setSource("/path/to/file") it is supposed to work.
> > >
> > > If so, regarding relative paths it would not work, but it states in the
> > > beginning of the file:
> > > "Paths stored here are supposed to NOT be relative."
> > > In the current implementation it is only used with absolute paths.
> > > Let me know if it is acceptable.
> >
> > I'd ask that the function setSource() will set or reset _sFile and _origPath
> > variables whenever _source variable changes. While it may not be problem in
> > how it's currently used, it is a potentially hard to find error in future if
> > the implementation changes.
> >
> > So in the current merge request it could be something like:
> >
> > if (pathLen != -1)
> > {
> > _sFile = QStringRef(&_source, pathLen + 1, _source.size() - pathLen -
> > 1);
> > _origPath = QStringRef(&_source, 0, pathLen);
> > }
> > else
> > { //avoid bad index
> > pathLen = 0;
> > _sFile = QStringRef();
> > _origPath = QStringRef();
>
> Thinking a little more about it, it looks like the two lines above do not have
> any effect.
> Both "_sFile" and "_origPath" were already initialized with QStringRef default
> constructor.

There is no problem if setSource() function is only used from constructor. Then all is fine. But as it stands now, setSource is a public function and can be called after the object has been created. If you want to enforce it being called only in constructor, make it private and then no such issues that I brought up can happen.

But I see no problems with the changes you've done now, the QStringRef variables seem to be quaranteed to be always set again when _source changes. So I'm approving, and if you want to refine it further it can be another commit. Thanks!

>
> I will change for that:
>
> inline void setSource(const QString& source)
> {
> int pathLen = source.lastIndexOf(QDir::separator());
> if (pathLen != -1)
> {
> _source = source;
> _sFile = QStringRef(&_source, pathLen + 1, _source.size() -
> pathLen - 1);
> _origPath = QStringRef(&_source, 0, pathLen);
> }
> else
> { // using ./source
> setSource(QString(".") + QDir::separator() + source);
> }
> ...

review: Approve

=== modified file 'src/plugin/folderlistmodel/diriteminfo.h'
--- src/plugin/folderlistmodel/diriteminfo.h	2014-05-01 17:38:23 +0000
+++ src/plugin/folderlistmodel/diriteminfo.h	2014-05-18 18:09:25 +0000
@@ -163,4 +163,84 @@
 
 
 
+/*!
+ * \brief The ActionPaths struct contains helper functions to do simple path handling
+ *
+ *  Paths stored here are supposed to NOT be relative.
+ *
+ *  It does not use any QFileInfo method, so it may work for any URL type
+ */
+struct ActionPaths
+{
+ public:
+    ActionPaths() {}
+    ActionPaths(const QString& source)
+    {
+        setSource(source);
+    }
+    inline void setSource(const QString& source)
+    {        
+        int pathLen = source.lastIndexOf(QDir::separator());
+        if (pathLen != -1)
+        {
+           _source   = source;
+           _sFile    = QStringRef(&_source, pathLen + 1,  _source.size() - pathLen - 1);
+           _origPath = QStringRef(&_source, 0, pathLen);
+        }
+        else
+        {
+           //avoids possible memory corruption using relative paths, QStringRef would be empty/null
+           //relative paths currently are not supported
+            setSource(QString(".") + QDir::separator() + source);
+        }
+    }
+    inline void setTargetPathOnly(const QString& path)
+    {
+        _targetPath = path;
+        _target     = path +  QDir::separator();
+        _target    += _sFile;
+    }
+    inline void setTargetFullName(const QString& fullPathname)
+    {
+        _target = fullPathname;
+        int lastSeparator = _target.lastIndexOf(QDir::separator());
+        if (lastSeparator > 0)
+        {
+            _targetPath  = _target.mid(0, lastSeparator);
+        }
+    }
+    inline ActionPaths& operator=(const ActionPaths& other)
+    {
+        setSource(other._source);
+        setTargetFullName(other._target);
+        return *this;
+    }
+    inline bool              areEquals() const {return _source == _target;}
+    inline bool              arePathsEquals() const
+    {
+        return QStringRef::compare(_origPath, _targetPath) == 0;
+    }
+    inline const QString&    source()    const {return _source;}
+    inline const QString&    target()    const {return _target;}
+    inline const QString&    targetPath()const {return _targetPath;}
+    inline const QStringRef& file()      const {return _sFile;}
+    inline int               baseOrigSize() const {return _origPath.size();}
+    inline void  toggle()
+    {
+        QString savedSource(_source);
+        setSource(_target);
+        setTargetFullName(savedSource);
+    }
+ private:
+    QString    _source;      //!<   source full pathname
+    QString    _target;      //!<   target full pathname
+    QString    _targetPath;  //!<   target path only
+    QStringRef _sFile;       //!<   source file name only
+    QStringRef _origPath;    //!<   source path only
+};
+
+Q_DECLARE_METATYPE(ActionPaths)
+
+typedef QList<ActionPaths>       ActionPathList;
+
 #endif // DIRITEMINFO_H