ubuntu-touch-coreapps-reviewers team mailing list archive

[Arto Jalkanen <ajalkane@xxxxxxxxx>]

Review: Approve

Several places that this comment applies to:

Why don't you use d(setParentIfRelative(folderName)); here also?

My understanding of C++ is that is more efficient than using assignment to created object. With assignment the object is first created, and then it's operator=() called to set instance's variables. If using directly copy constructor, the compiler can optimize the creation of the object.

I'll approve this nevertheless as it's a minor thing and you can consider yourself whether to change it or top-approve this change.


Diff comments:

> === modified file 'src/plugin/folderlistmodel/dirmodel.cpp'
> --- src/plugin/folderlistmodel/dirmodel.cpp	2015-07-18 21:32:12 +0000
> +++ src/plugin/folderlistmodel/dirmodel.cpp	2015-07-18 21:32:12 +0000
> @@ -1509,80 +1509,51 @@
>  
>  bool DirModel::existsDir(const QString &folderName) const
>  {
> -    DirItemInfo d(setParentIfRelative(folderName));
> +    DirItemInfo d = setParentIfRelative(folderName);

Why don't you use d(setParentIfRelative(folderName)); here also?

My understanding of C++ is that is more efficient than using assignment to created object. With assignment the object is first created, and then it's operator=() called to set instance's variables. If using directly copy constructor, the compiler can optimize the creation of the object.

>      return d.exists() && d.isDir();
>  }
>  
>  bool  DirModel::canReadDir(const QString &folderName) const
>  {
> -    DirItemInfo d(setParentIfRelative(folderName));
> -    return canReadDir(d.diskFileInfo());
> +    DirItemInfo d = setParentIfRelative(folderName);
> +    return d.isDir() && d.isReadable();
>  }
>  
> -bool  DirModel::canReadDir(const QFileInfo & d) const
> -{
> -    return d.exists() && d.isDir() && d.isReadable() && d.isExecutable();
> -}
>  
>  bool DirModel::existsFile(const QString &fileName) const
>  {
> -     DirItemInfo f(setParentIfRelative(fileName));
> +     DirItemInfo f = setParentIfRelative(fileName);
>       return f.exists() && f.isFile();
>  }
>  
>  bool DirModel::canReadFile(const QString &fileName) const
>  {
> -    DirItemInfo  f(setParentIfRelative(fileName));
> -    return canReadFile(f.diskFileInfo());
> -}
> -
> -bool DirModel::canReadFile(const QFileInfo &f) const
> -{
> -    return f.exists() && f.isFile() && f.isReadable();
> -}
> -
> +    DirItemInfo  f = setParentIfRelative(fileName);
> +    return f.isReadable() && f.isFile();
> +}
>  
>  
>  QDateTime DirModel::curPathCreatedDate() const
> -{
> -   QDateTime d;
> -   QFileInfo f(mCurrentDir);
> -   if (f.exists())
> -   {
> -       d = f.created();
> -   }
> -   return d;
> +{  
> +    return mCurLocation->currentInfo()->created();
>  }
>  
>  
>  QDateTime DirModel::curPathModifiedDate() const
>  {
> -    QDateTime d;
> -    QFileInfo f(mCurrentDir);
> -    if (f.exists())
> -    {
> -        d = f.lastModified();
> -    }
> -    return d;
> +     return mCurLocation->currentInfo()->lastModified();
>  }
>  
>  
>  QDateTime DirModel::curPathAccessedDate() const
>  {
> -    QDateTime d;
> -    QFileInfo f(mCurrentDir);
> -    if (f.exists())
> -    {
> -        d = f.lastRead();
> -    }
> -    return d;
> +    return mCurLocation->currentInfo()->lastRead();
>  }
>  
>  
>  bool  DirModel::curPathIsWritable() const
>  {
> -    QFileInfo f(mCurrentDir);
> -    return f.exists() && f.isWritable();
> +     return mCurLocation->currentInfo()->isWritable();
>  }
>  
>  QString DirModel::curPathCreatedDateLocaleShort() const


-- 
https://code.launchpad.net/~carlos-mazieri/ubuntu-filemanager-app/samba-actions-03/+merge/265194
Your team Ubuntu File Manager Developers is subscribed to branch lp:ubuntu-filemanager-app.