Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace MDir with mixxx::FileInfo/FileAccess #3744

Merged
merged 8 commits into from
Apr 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,7 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL
src/util/dnd.cpp
src/util/duration.cpp
src/util/experiment.cpp
src/util/file.cpp
src/util/fileaccess.cpp
src/util/fileinfo.cpp
src/util/imageutils.cpp
src/util/indexrange.cpp
Expand Down Expand Up @@ -1146,7 +1146,7 @@ elseif (UNIX)
endif()

if(WIN32)
target_compile_definitions(mixxx-lib PRIVATE __WINDOWS__)
target_compile_definitions(mixxx-lib PUBLIC __WINDOWS__)

# Helps prevent duplicate symbols
target_compile_definitions(mixxx-lib PUBLIC _ATL_MIN_CRT)
Expand Down Expand Up @@ -1177,7 +1177,7 @@ elseif(UNIX)
if(APPLE)
target_compile_definitions(mixxx-lib PUBLIC __APPLE__)
else()
target_compile_definitions(mixxx-lib PRIVATE __UNIX__)
target_compile_definitions(mixxx-lib PUBLIC __UNIX__)
if(CMAKE_SYSTEM_NAME STREQUAL Linux)
target_compile_definitions(mixxx-lib PUBLIC __LINUX__)
elseif(CMAKE_SYSTEM_NAME MATCHES "^.*BSD$")
Expand Down
4 changes: 2 additions & 2 deletions src/coreservices.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "engine/enginemaster.h"
#include "library/coverartcache.h"
#include "library/library.h"
#include "library/trackcollection.h"
#include "library/trackcollectionmanager.h"
#include "mixer/playerinfo.h"
#include "mixer/playermanager.h"
Expand Down Expand Up @@ -284,8 +285,7 @@ void CoreServices::initialize(QApplication* pApp) {

bool hasChanged_MusicDir = false;

QStringList dirs = m_pLibrary->getDirs();
if (dirs.size() < 1) {
if (m_pTrackCollectionManager->internalCollection()->loadRootDirs().isEmpty()) {
// TODO(XXX) this needs to be smarter, we can't distinguish between an empty
// path return value (not sure if this is normally possible, but it is
// possible with the Windows 7 "Music" library, which is what
Expand Down
32 changes: 16 additions & 16 deletions src/library/browse/browsefeature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -249,21 +249,21 @@ void BrowseFeature::activateChild(const QModelIndex& index) {

QString path = item->getData().toString();
if (path == QUICK_LINK_NODE || path == DEVICE_NODE) {
m_browseModel.setPath(MDir());
m_browseModel.setPath({});
} else {
// Open a security token for this path and if we do not have access, ask
// for it.
MDir dir(path);
if (!dir.canAccess()) {
auto dir = mixxx::FileAccess(mixxx::FileInfo(path));
if (!dir.isReadable()) {
if (Sandbox::askForAccess(path)) {
// Re-create to get a new token.
dir = MDir(path);
dir = mixxx::FileAccess(mixxx::FileInfo(path));
} else {
// TODO(rryan): Activate an info page about sandboxing?
return;
}
}
m_browseModel.setPath(dir);
m_browseModel.setPath(std::move(dir));
}
emit showTrackModel(&m_proxyModel);
emit enableCoverArtDisplay(false);
Expand Down Expand Up @@ -402,10 +402,10 @@ void BrowseFeature::onLazyChildExpandation(const QModelIndex& index) {
} else {
// we assume that the path refers to a folder in the file system
// populate childs
MDir dir(path);
const auto dirAccess = mixxx::FileAccess(mixxx::FileInfo(path));

QFileInfoList all = dir.dir().entryInfoList(
QDir::Dirs | QDir::NoDotAndDotDot);
QFileInfoList all = dirAccess.info().toQDir().entryInfoList(
QDir::Dirs | QDir::NoDotAndDotDot);

// loop through all the item and construct the childs
foreach (QFileInfo one, all) {
Expand Down Expand Up @@ -465,7 +465,6 @@ QString BrowseFeature::extractNameFromPath(const QString& spath) {

QStringList BrowseFeature::getDefaultQuickLinks() const {
// Default configuration
QStringList mixxxMusicDirs = m_pTrackCollection->getDirectoryDAO().getDirs();
QDir osMusicDir(QStandardPaths::writableLocation(
QStandardPaths::MusicLocation));
QDir osDocumentsDir(QStandardPaths::writableLocation(
Expand All @@ -485,10 +484,11 @@ QStringList BrowseFeature::getDefaultQuickLinks() const {
bool osDownloadsDirIncluded = false;
bool osDesktopDirIncluded = false;
bool osDocumentsDirIncluded = false;
foreach (QString dirPath, mixxxMusicDirs) {
QDir dir(dirPath);
const auto rootDirs = m_pLibrary->trackCollections()->internalCollection()->loadRootDirs();
for (const mixxx::FileInfo& fileInfo : rootDirs) {
const auto dir = fileInfo.toQDir();
// Skip directories we don't have permission to.
if (!Sandbox::canAccessFile(dir)) {
if (!Sandbox::canAccessDir(dir)) {
continue;
}
if (dir == osMusicDir) {
Expand All @@ -506,22 +506,22 @@ QStringList BrowseFeature::getDefaultQuickLinks() const {
result << dir.canonicalPath() + "/";
}

if (!osMusicDirIncluded && Sandbox::canAccessFile(osMusicDir)) {
if (!osMusicDirIncluded && Sandbox::canAccessDir(osMusicDir)) {
result << osMusicDir.canonicalPath() + "/";
}

if (downloadsExists && !osDownloadsDirIncluded &&
Sandbox::canAccessFile(osDownloadsDir)) {
Sandbox::canAccessDir(osDownloadsDir)) {
result << osDownloadsDir.canonicalPath() + "/";
}

if (!osDesktopDirIncluded &&
Sandbox::canAccessFile(osDesktopDir)) {
Sandbox::canAccessDir(osDesktopDir)) {
result << osDesktopDir.canonicalPath() + "/";
}

if (!osDocumentsDirIncluded &&
Sandbox::canAccessFile(osDocumentsDir)) {
Sandbox::canAccessDir(osDocumentsDir)) {
result << osDocumentsDir.canonicalPath() + "/";
}

Expand Down
6 changes: 3 additions & 3 deletions src/library/browse/browsetablemodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "moc_browsetablemodel.cpp"
#include "track/track.h"
#include "util/compatibility.h"
#include "util/fileaccess.h"
#include "widget/wlibrarytableview.h"

BrowseTableModel::BrowseTableModel(QObject* parent,
Expand Down Expand Up @@ -185,9 +186,8 @@ void BrowseTableModel::addSearchColumn(int index) {
m_searchColumns.push_back(index);
}

void BrowseTableModel::setPath(const MDir& path) {
m_current_directory = path;
m_pBrowseThread->executePopulation(m_current_directory, this);
void BrowseTableModel::setPath(mixxx::FileAccess path) {
m_pBrowseThread->executePopulation(std::move(path), this);
}

TrackPointer BrowseTableModel::getTrack(const QModelIndex& index) const {
Expand Down
10 changes: 7 additions & 3 deletions src/library/browse/browsetablemodel.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

#include "library/trackmodel.h"
#include "recording/recordingmanager.h"
#include "util/file.h"
#include "library/browse/browsethread.h"

//constants
Expand Down Expand Up @@ -33,6 +32,12 @@ const int COLUMN_REPLAYGAIN = 20;

class TrackCollectionManager;

namespace mixxx {

class FileAccess;

} // namespace mixxx

// The BrowseTable models displays tracks
// of given directory on the HDD.
// Usage: Recording and Browse feature.
Expand All @@ -47,7 +52,7 @@ class BrowseTableModel final : public QStandardItemModel, public virtual TrackMo
BrowseTableModel(QObject* parent, TrackCollectionManager* pTrackCollectionManager, RecordingManager* pRec);
virtual ~BrowseTableModel();

void setPath(const MDir& path);
void setPath(mixxx::FileAccess path);

TrackPointer getTrack(const QModelIndex& index) const override;
TrackPointer getTrackByRef(const TrackRef& trackRef) const override;
Expand Down Expand Up @@ -84,7 +89,6 @@ class BrowseTableModel final : public QStandardItemModel, public virtual TrackMo
TrackCollectionManager* const m_pTrackCollectionManager;

QList<int> m_searchColumns;
MDir m_current_directory;
RecordingManager* m_pRecordingManager;
BrowseThreadPointer m_pBrowseThread;
QString m_previewDeckGroup;
Expand Down
19 changes: 8 additions & 11 deletions src/library/browse/browsethread.cpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
/*
* browsethread.cpp (C) 2011 Tobias Rafreider
*/

#include "library/browse/browsethread.h"

#include <QDateTime>
Expand Down Expand Up @@ -65,9 +61,9 @@ BrowseThreadPointer BrowseThread::getInstanceRef() {
return strong;
}

void BrowseThread::executePopulation(const MDir& path, BrowseTableModel* client) {
void BrowseThread::executePopulation(mixxx::FileAccess path, BrowseTableModel* client) {
m_path_mutex.lock();
m_path = path;
m_path = std::move(path);
m_model_observer = client;
m_path_mutex.unlock();
m_locationUpdated.wakeAll();
Expand Down Expand Up @@ -116,15 +112,16 @@ class YearItem: public QStandardItem {

void BrowseThread::populateModel() {
m_path_mutex.lock();
MDir thisPath = m_path;
auto thisPath = m_path;
BrowseTableModel* thisModelObserver = m_model_observer;
m_path_mutex.unlock();

// Refresh the name filters in case we loaded new SoundSource plugins.
QStringList nameFilters(SoundSourceProxy::getSupportedFileNamePatterns());

QDirIterator fileIt(thisPath.dir().absolutePath(), nameFilters,
QDir::Files | QDir::NoDotAndDotDot);
QDirIterator fileIt(thisPath.info().location(),
nameFilters,
QDir::Files | QDir::NoDotAndDotDot);

// remove all rows
// This is a blocking operation
Expand All @@ -139,10 +136,10 @@ void BrowseThread::populateModel() {
// If a user quickly jumps through the folders
// the current task becomes "dirty"
m_path_mutex.lock();
MDir newPath = m_path;
auto newPath = m_path;
m_path_mutex.unlock();

if (thisPath.dir() != newPath.dir()) {
if (thisPath.info() != newPath.info()) {
qDebug() << "Abort populateModel()";
populateModel();
return;
Expand Down
14 changes: 7 additions & 7 deletions src/library/browse/browsethread.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@

#pragma once

#include <QThread>
#include <QMutex>
#include <QWaitCondition>
#include <QStandardItem>
#include <QList>
#include <QMutex>
#include <QSharedPointer>
#include <QStandardItem>
#include <QThread>
#include <QWaitCondition>
#include <QWeakPointer>

#include "util/file.h"
#include "util/fileaccess.h"

// This class is a singleton and represents a thread
// that is used to read ID3 metadata
Expand All @@ -30,7 +30,7 @@ class BrowseThread : public QThread {
Q_OBJECT
public:
virtual ~BrowseThread();
void executePopulation(const MDir& path, BrowseTableModel* client);
void executePopulation(mixxx::FileAccess path, BrowseTableModel* client);
void run();
static BrowseThreadPointer getInstanceRef();

Expand All @@ -49,7 +49,7 @@ class BrowseThread : public QThread {

// You must hold m_path_mutex to touch m_path or m_model_observer
QMutex m_path_mutex;
MDir m_path;
mixxx::FileAccess m_path;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks confusing, because one may think this refrences a file, but it is a directory. Do you have an idea to improve the naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QFileInfo also could reference everything file/dir/symlink, we follow it. This naming is at least consistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I know. Unfortunately this makes the usage code harder to read.
Can we rename m_pPath to something more specific, to improve the situation?

Do we use the security tokens for single files? Looking into the implementation, there are different call stacks for files and directories. Maybe we can inherit a file and a dir version and accept some code duplications for some extra safety

What are your future plans with this?

Copy link
Member

@daschuer daschuer Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nothing that blocks this PR ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just noticed this. The special QDir overloads in Sandbox are not needed an can be removed. We can switch entirely to mixxx::FileInfo. Only the browse view seems to be affected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inheriting does not make sense for this use case. You can only know at runtime if a file system reference is a file, directory, or symlink.

The equivalent of QFileInfo in Rust would be std::path::Path/std::path::PathBuf.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed that Sandbox handles directories differently. Converting back to draft until I have checked that nothing got mixed up here. Thanks for reviewing so far.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this:

Suggested change
mixxx::FileAccess m_path;
mixxx::FileAccess m_dir;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to introduce many unrelated changes just by renaming things. And I especially try to avoid touching this browse stuff.

BrowseTableModel* m_model_observer;

static QWeakPointer<BrowseThread> m_weakInstanceRef;
Expand Down
3 changes: 1 addition & 2 deletions src/library/browse/foldertreemodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "library/browse/foldertreemodel.h"
#include "library/treeitem.h"
#include "moc_foldertreemodel.cpp"
#include "util/file.h"

FolderTreeModel::FolderTreeModel(QObject *parent)
: TreeItemModel(parent) {
Expand Down Expand Up @@ -59,7 +58,7 @@ bool FolderTreeModel::directoryHasChildren(const QString& path) const {
}

// Acquire a security token for the path.
MDir dir(path);
const auto dirAccess = mixxx::FileAccess(mixxx::FileInfo(path));
daschuer marked this conversation as resolved.
Show resolved Hide resolved

/*
* The following code is too expensive, general and SLOW since
Expand Down
Loading