ubuntu-touch-coreapps-reviewers team mailing list archive

[Arto Jalkanen <ajalkane@xxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'src/plugin/folderlistmodel/dirmodel.cpp'
> --- src/plugin/folderlistmodel/dirmodel.cpp	2015-08-25 18:41:31 +0000
> +++ src/plugin/folderlistmodel/dirmodel.cpp	2015-09-07 21:20:53 +0000
> @@ -542,12 +542,20 @@
>  }
>  
>  bool DirModel::allowAccess(const DirItemInfo &fi) const {
> -    return allowAccess(fi.absoluteFilePath());
> +    bool allowed = !mOnlyAllowedPaths; // !mOnlyAllowedPaths means any path is allowed
> +    if (!allowed)
> +    {
> +        // for remote locations items are visible if them do not require authentication
> +        allowed = mCurLocation->isRemote() ? !fi.needsAuthentication() :
> +                                             isAllowedPath(fi.absoluteFilePath());
> +    }
> +    return allowed;
>  }
>  
>  bool DirModel::allowAccess(const QString &absoluteFilePath) const {
> -    return !mOnlyAllowedPaths || isAllowedPath(absoluteFilePath);
> -}
> +    return !mOnlyAllowedPaths || mCurLocation->isRemote() || isAllowedPath(absoluteFilePath);

This doesn't make sense to me. AllowAccess(absoluteFilePath) is supposed to say if the given absolute file path is allowed access. Instead now it always allows access if current location happens to be remote path, and doesn't in that case take into account if the given path given in parameter is allowed access.

This function and allowAccess(const DirItemInfo &fi) should have the same logic, they just receive their parameter in different representation. But it must be said that after your refactorings it can be that either one of the functions is not necessary anymore.

But if both exist they should either have same logic, preferably sharing the code if possible, or the given function parameters and/or function name reflect what's the real purpose here.

> +}// for remote locations access is allowed
> +
>  
>  void DirModel::onItemsAdded(const DirItemInfoList &newFiles)
>  {


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