ubuntu-touch-coreapps-reviewers team mailing list archive
-
ubuntu-touch-coreapps-reviewers team
-
Mailing list archive
-
Message #00730
Re: [Merge] lp:~carlos-mazieri/ubuntu-filemanager-app/samba-browsing-01 into lp:ubuntu-filemanager-app
Review: Needs Fixing
Please fix the few issues I inlined in comments.
Diff comments:
> === modified file 'src/plugin/folderlistmodel/diriteminfo.cpp'
> --- src/plugin/folderlistmodel/diriteminfo.cpp 2014-05-09 21:03:22 +0000
> +++ src/plugin/folderlistmodel/diriteminfo.cpp 2015-03-08 11:54:25 +0000
> @@ -20,6 +20,8 @@
> */
>
> #include "diriteminfo.h"
> +#include <sys/types.h>
> +#include <sys/stat.h>
>
>
> QMimeDatabase DirItemInfoPrivate::mimeDatabase;
> @@ -32,12 +34,18 @@
> ,_isSelected(false)
> ,_isAbsolute(false)
> ,_exists(false)
> + ,_isFile(false)
> ,_isDir(false)
> ,_isSymLink(false)
> ,_isRoot(false)
> ,_isReadable(false)
> ,_isWritable(false)
> ,_isExecutable(false)
> + ,_isLocalSharedDir(false)
> + ,_isHost(false)
> + ,_isWorkGroup(false)
> + ,_isNetworkShare(false)
> + ,_needsAuthentication(false)
> ,_permissions(0)
> ,_size(0)
> {
> @@ -53,19 +61,26 @@
> ,_isSelected(other._isSelected)
> ,_isAbsolute(other._isAbsolute)
> ,_exists(other._exists)
> + ,_isFile(other._isFile)
> ,_isDir(other._isDir)
> ,_isSymLink(other._isSymLink)
> ,_isRoot(other._isRoot)
> ,_isReadable(other._isReadable)
> ,_isWritable(other._isWritable)
> ,_isExecutable(other._isExecutable)
> + ,_isLocalSharedDir(other._isLocalSharedDir)
> + ,_isHost(other._isHost)
> + ,_isWorkGroup(false)
> + ,_isNetworkShare(false)
> + ,_needsAuthentication(false)
> ,_permissions(other._permissions)
> ,_size(other._size)
> ,_created(other._created)
> ,_lastModified(other._lastModified)
> ,_lastRead(other._lastRead)
> ,_path(other._path)
> - ,_fileName(other._fileName)
> + ,_fileName(other._fileName)
> + ,_normalizedPath(other._normalizedPath)
> {
>
> }
> @@ -76,6 +91,21 @@
> ,_isLocal(true)
> ,_isRemote(false)
> ,_isSelected(false)
> + ,_isAbsolute(false)
> + ,_exists(false)
> + ,_isFile(false)
> + ,_isDir(false)
> + ,_isSymLink(false)
> + ,_isRoot(false)
> + ,_isReadable(false)
> + ,_isWritable(false)
> + ,_isExecutable(false)
> + ,_isLocalSharedDir(false)
> + ,_isHost(false)
> + ,_isWorkGroup(false)
> + ,_isNetworkShare(false)
> + ,_needsAuthentication(false)
> + ,_permissions(0)
> {
> setFileInfo(fi);
> }
> @@ -333,3 +363,218 @@
> filepath += d_ptr->_fileName;
> return filepath;
> }
> +
> +bool DirItemInfo::permission(QFileDevice::Permissions permissions) const
> +{
> + return (d_ptr->_permissions & permissions) == permissions;
> +}
> +
> +bool DirItemInfo::isSharedDir() const
> +{
> + return d_ptr->_isLocalSharedDir;
> +}
> +
> +bool DirItemInfo::isHost() const
> +{
> + return d_ptr->_isHost;
> +}
> +
> +
> +bool DirItemInfo::isWorkGroup() const
> +{
> + return d_ptr->_isWorkGroup;
> +}
> +
> +bool DirItemInfo::isShare() const
> +{
> + return d_ptr->_isNetworkShare;
> +}
> +
> +/*!
> + * \brief DirItemInfo::isBrowsable() considers browsable items that can hold a list of items
> + * \return
> + */
> +bool DirItemInfo::isBrowsable() const
> +{
> + return isDir() || isHost() || isShare() || isWorkGroup();
> +}
> +
> +bool DirItemInfo::needsAuthentication() const
> +{
> + return d_ptr->_needsAuthentication;
> +}
> +
> +QString DirItemInfo::authenticationPath() const
> +{
> + return QLatin1String(0);
> +}
> +
> +
> +/*!
> + * \brief This was copied from DirItemInfo::fillFromStatBuf \ref QFileSystemMetaData::fillFromStatBuf()
You mean I guess copied from QFileSystemMetaData:: ...
> + * \param statBuffer
> + */
> +void DirItemInfo::fillFromStatBuf(const struct stat & statBuffer)
> +{
> +#define LinkType 0x00010000
> +#define FileType 0x00020000
> +#define DirectoryType 0x00040000
> +#define SequentialType 0x00800000
> +
> + //size
> + d_ptr->_size = statBuffer.st_size;
> + //times
> + d_ptr->_lastModified = statBuffer.st_mtime ?
> + QDateTime::fromTime_t(statBuffer.st_mtime) :
> + QDateTime(QDate(), QTime());
> + d_ptr->_created = statBuffer.st_ctime ?
> + QDateTime::fromTime_t(statBuffer.st_ctime) :
> + d_ptr->_lastModified ;
> + d_ptr->_lastRead = statBuffer.st_atime ?
> + QDateTime::fromTime_t(statBuffer.st_atime) :
> + d_ptr->_lastModified;
> +
> + /*
> + //user, group
> + userId_ = statBuffer.st_uid;
> + groupId_ = statBuffer.st_gid;
> + */
> +
> + /*
> + * When handling filesystems other than local (e.g. any network)
> + * Permissions are relative to the user being used to access the resource
> + *
> + * So it is necessary to qualify the user accessing the resource as
> + * owner / belongs to group or / others
> + */
> + // quint32 userMatches = 0;
> + QFile::Permissions readPermission = 0;
> + QFile::Permissions writePermission = 0;
> + QFile::Permissions execPermission = 0;
> +
> +
> + //owner permissions
> + if (statBuffer.st_mode & S_IRUSR)
> + {
> + readPermission |= QFile::ReadOwner;
> + }
> + if (statBuffer.st_mode & S_IWUSR)
> + {
> + writePermission |= QFile::WriteOwner;
> + }
> + if (statBuffer.st_mode & S_IXUSR)
> + {
> + execPermission |= QFile::ExeOwner;
> + }
> + //group permissions
> + if (statBuffer.st_mode & S_IRGRP)
> + {
> + readPermission |= QFile::ReadGroup;
> + }
> + if (statBuffer.st_mode & S_IWGRP)
> + {
> + writePermission |= QFile::WriteGroup;
> + }
> + if (statBuffer.st_mode & S_IXGRP)
> + {
> + execPermission |= QFile::ExeGroup;
> + }
> + //other permissions
> + if (statBuffer.st_mode & S_IROTH)
> + {
> + readPermission |= QFile::ReadOther;
> + }
> + if (statBuffer.st_mode & S_IWOTH)
> + {
> + writePermission |= QFile::WriteOther;
> + }
> + if (statBuffer.st_mode & S_IXOTH)
> + {
> + execPermission |= QFile::ExeOther;
> + }
> +
> + /*
> + * Permissions are relative to a remote user
> + * it was necessary to be the user being accessing the file
> + */
> + if (readPermission)
> + {
> + d_ptr->_isReadable = true;
> + }
> + if (writePermission)
> + {
> + d_ptr->_isWritable = true;
> + }
> + if (execPermission)
> + {
> + d_ptr->_isExecutable = true;
> + }
> +
> + //set full permissions flag
> + d_ptr->_permissions = readPermission | writePermission | execPermission;
> +
> + // Type
> + if ((statBuffer.st_mode & S_IFMT) == S_IFREG)
> + {
> + // d_ptr->_permissions |= FileType;
> + d_ptr->_isFile = true;
> + }
> + else if ((statBuffer.st_mode & S_IFMT) == S_IFDIR)
> + {
> + // d_ptr->_permissions |= DirectoryType;
> + d_ptr->_isDir = true;
> + }
> + else
> + {
> + // d_ptr->_permissions |= SequentialType;
> + }
> +}
> +
> +
> +
> +QString DirItemInfo::removeExtraSlashes(const QString &url, int firstSlashIndex)
> +{
> + QString ret;
> + if (firstSlashIndex == -1)
> + {
> + firstSlashIndex = url.indexOf(QLatin1Char(':'));
Note that in Linux it's valid to have ":" in filename, for example "touch 'how:dy'"
> + if (firstSlashIndex != -1 && url.at(firstSlashIndex+1) == QDir::separator())
url.at(firstSlashIndex+1) breaks if passed invalid "url" like for example "file:". Ie. if ":" is last character of the string
> + {
> + ++firstSlashIndex;
> + }
> + else
> + {
> + firstSlashIndex = -1;
> + }
> + }
> + if (firstSlashIndex >=0)
> + {
> + while ( firstSlashIndex < url.length() && url.at(firstSlashIndex) == QDir::separator())
> + {
> + ++firstSlashIndex;
> + }
> + if (firstSlashIndex < url.length())
> + {
> + ret = url.mid(firstSlashIndex);
> + }
> + }
> + else
> + {
> + ret = url;
> + firstSlashIndex = -1;
> + }
> + if (firstSlashIndex >= 0 && ret.endsWith(QDir::separator()))
> + {
> + ret.chop(1);
> + }
> + //replace any double slashes by just one
> + for(int charCounter = ret.size() -1; charCounter > 0; --charCounter)
> + {
> + if (ret.at(charCounter) == QDir::separator() &&
> + ret.at(charCounter-1) == QDir::separator())
> + {
> + ret.remove(charCounter,1);
> + }
> + }
> + return ret;
> +}
>
> === modified file 'src/plugin/folderlistmodel/diriteminfo.h'
> --- src/plugin/folderlistmodel/diriteminfo.h 2014-05-18 18:09:04 +0000
> +++ src/plugin/folderlistmodel/diriteminfo.h 2015-03-08 11:54:25 +0000
> @@ -101,16 +101,26 @@
> virtual QDateTime lastModified() const;
> virtual QDateTime lastRead() const;
> virtual QMimeType mimeType() const;
> + virtual bool isHost() const;
> + virtual bool isSharedDir() const;
> + virtual bool isWorkGroup() const;
> + virtual bool isShare() const;
> + virtual bool isBrowsable() const;
> + virtual bool needsAuthentication() const;
> + virtual QString authenticationPath() const;
> + virtual void setFile(const QString &dir, const QString & file);
> + virtual bool permission(QFile::Permissions permissions) const;
> + void fillFromStatBuf(const struct stat& statBuffer);
>
> - virtual void setFile(const QString &dir, const QString & file);
> +public:
> + static QString removeExtraSlashes(const QString &url, int firstSlashIndex = -1);
>
> #if 0
> virtual QString path() const;
> virtual QString owner() const;
> virtual uint ownerId() const;
> virtual QString group() const;
> - virtual uint groupId() const;
> - virtual bool permission(QFile::Permissions permissions) const;
> + virtual uint groupId() const;
> #endif
>
> protected:
> @@ -149,7 +159,14 @@
> bool _isRoot :1;
> bool _isReadable :1;
> bool _isWritable :1;
> - bool _isExecutable:1;
> + bool _isExecutable:1;
> + bool _isLocalSharedDir :1; //!< the directory in the local disk is shared folder (perhaps using Samba)
> + bool _isHost :1; //!< the item points to a host like fish://localhost, smb://10.10.200.1, etc.
> + bool _isWorkGroup :1; //!< specific to Samba
> + bool _isNetworkShare :1; //!< samba share (entry point)
> + bool _needsAuthentication:1;//!< the url may require authentication do access
> +
> +
> QFile::Permissions _permissions;
> qint64 _size;
> QDateTime _created;
> @@ -158,6 +175,7 @@
> QString _path;
> QString _fileName;
> QString _normalizedPath;
> +
> static QMimeDatabase mimeDatabase;
> };
>
>
> === modified file 'src/plugin/folderlistmodel/dirmodel.cpp'
> --- src/plugin/folderlistmodel/dirmodel.cpp 2015-01-01 20:43:56 +0000
> +++ src/plugin/folderlistmodel/dirmodel.cpp 2015-03-08 11:54:25 +0000
> @@ -216,7 +216,12 @@
> roles.insert(FileSizeRole, QByteArray("fileSize"));
> roles.insert(IconSourceRole, QByteArray("iconSource"));
> roles.insert(FilePathRole, QByteArray("filePath"));
> - roles.insert(IsDirRole, QByteArray("isDir"));
> + roles.insert(IsDirRole, QByteArray("isDir"));
> + roles.insert(IsHostRole, QByteArray("isHost"));
> + roles.insert(IsSmbWorkgroupRole, QByteArray("IsSmbWorkgroup"));
> + roles.insert(IsSmbShareRole, QByteArray("IsSmbShare"));
> + roles.insert(IsSharedDirRole, QByteArray("IsSharedDir"));
> + roles.insert(IsSharingAllowedRole, QByteArray("IsSharingAllowed"));
> roles.insert(IsFileRole, QByteArray("isFile"));
> roles.insert(IsReadableRole, QByteArray("isReadable"));
> roles.insert(IsWritableRole, QByteArray("isWritable"));
> @@ -268,7 +273,7 @@
> {
> QIcon icon;
> QMimeType mime = mDirectoryContents.at(index.row()).mimeType();
> - if (mime.isValid())
> + if (mime.isValid() && mDirectoryContents.at(index.row()).isLocal())
> {
> if (QIcon::hasThemeIcon(mime.iconName()) ) {
> icon = QIcon::fromTheme(mime.iconName());
> @@ -333,9 +338,15 @@
> case ModifiedDateRole:
> return fi.lastModified();
> case FileSizeRole: {
> - if (fi.isDir() && fi.isLocal())
> + if (fi.isBrowsable())
> {
> - return dirItems(fi.diskFileInfo());
> + if(fi.isLocal())
space after "if": "if (fi"
Sorry for nitpicking :)
> + {
> + return dirItems(fi.diskFileInfo());
> + }
> + //it is possible to browse network folders and get its
> + //number of items, but it may take longer
> + return tr("unkown");
> }
> return fileSize(fi.size());
> }
> @@ -357,7 +368,7 @@
> case IsDirRole:
> return fi.isDir();
> case IsFileRole:
> - return !fi.isDir();
> + return !fi.isBrowsable();
> case IsReadableRole:
> return fi.isReadable();
> case IsWritableRole:
> @@ -366,6 +377,18 @@
> return fi.isExecutable();
> case IsSelectedRole:
> return fi.isSelected();
> + case IsHostRole:
> + return fi.isHost();
> + case IsSmbWorkgroupRole:
> + return fi.isWorkGroup();
> + case IsSmbShareRole:
> + return fi.isShare();
> + case IsSharingAllowedRole:
> + return fi.isDir() && !fi.isSymLink() && !fi.isSharedDir()
> + && mCurLocation->type() == LocationsFactory::LocalDisk
> + && fi.isWritable() && fi.isExecutable() && fi.isReadable();
> + case IsSharedDirRole:
> + return fi.isSharedDir();
> #ifndef DO_NOT_USE_TAG_LIB
> case TrackTitleRole:
> case TrackArtistRole:
> @@ -388,7 +411,7 @@
> qWarning() << Q_FUNC_INFO << this << "Got an unknown role: " << role;
> #endif
> break;
> - }
> + }//switch (role)
>
> return QVariant();
> }
>
> === modified file 'src/plugin/folderlistmodel/dirmodel.h'
> --- src/plugin/folderlistmodel/dirmodel.h 2015-01-01 20:43:56 +0000
> +++ src/plugin/folderlistmodel/dirmodel.h 2015-03-08 11:54:25 +0000
> @@ -62,6 +62,11 @@
> IconSourceRole,
> FilePathRole,
> IsDirRole,
> + IsHostRole, //!< it can also be used for other protocols than smb/cifs
> + IsSmbWorkgroupRole,
> + IsSmbShareRole,
> + IsSharedDirRole, //!< it can also be used for other protocols than smb/cifs
> + IsSharingAllowedRole,//!< true for local directories (not in Trash) and not IsSharedDirRole
> IsFileRole,
> IsReadableRole,
> IsWritableRole,
>
--
https://code.launchpad.net/~carlos-mazieri/ubuntu-filemanager-app/samba-browsing-01/+merge/252216
Your team Ubuntu File Manager Developers is subscribed to branch lp:ubuntu-filemanager-app.
References
