Commit 97fbbd47 authored by Harald Sitter's avatar Harald Sitter 🏳️‍🌈
Browse files

port to smart pointers

on errors.ubuntu.com and the window partner center there are odd crashes
on bad pointers that I can't quite place. as a short in the dark lets
use smart pointers instead of raw pointers to keep things alive as long
as they need to rather than as long as we think they need to. in
particular duplication was a bit dodgy there. that said, duplication is
why we need shared ptrs, otherwise we'd probably have gotten by with
unique_ptrs.

had the additional advantage of forcing me to revisit the qobject use in
file/folder, which is now gone since it is pointless (qml wouldn't know
what to do with shared_ptr) instead the single use  for it is now part
of the item itself, a bit questionable from an OOP POV but at the same
time it saves us the overhead of N qobjects.

all in all this should improve memory safety substantially.
parent c5c7d2c8
Pipeline #204986 passed with stage
in 2 minutes and 26 seconds
SET(FILENAME_TEST_SRCS
testFileTree.cpp
../../src/fileTree.cpp
)
ecm_add_test(
${FILENAME_TEST_SRCS}
TEST_NAME "filetree_test"
LINK_LIBRARIES Qt${QT_MAJOR_VERSION}::Test
LINK_LIBRARIES Qt${QT_MAJOR_VERSION}::Test filelightInternal
KF5::CoreAddons
)
......@@ -7,15 +7,10 @@
#include "testFileTree.h"
TestFileTree::TestFileTree()
: fl(new File("./autotests/core/dummy.txt", 20))
: fl(std::make_unique<File>("./autotests/core/dummy.txt", 20))
{
}
TestFileTree::~TestFileTree()
{
delete fl;
}
void TestFileTree::testFileName()
{
const QString fname = fl->displayName();
......@@ -30,7 +25,7 @@ void TestFileTree::testFileSize()
void TestFileTree::testFilePath()
{
const Folder *folder = new Folder("./autotests/core/");
auto folder = std::make_shared<Folder>("./autotests/core/");
const QString fpath = fl->displayPath(folder);
QVERIFY(!fpath.isEmpty());
}
......
/***********************************************************************
* SPDX-FileCopyrightText: 2020 Shubham <aryan100jangid@gmail.com>
* SPDX-FileCopyrightText: 2022 Harald Sitter <sitter@kde.org>
*
* SPDX-License-Identifier: GPL-2.0-only OR GPL-3.0-only OR LicenseRef-KDE-Accepted-GPL
***********************************************************************/
......@@ -7,6 +8,8 @@
#ifndef TESTFILETREE_H
#define TESTFILETREE_H
#include <memory>
#include "fileTree.h"
#include <QTest>
......@@ -17,7 +20,6 @@ class TestFileTree : public QObject
public:
TestFileTree();
~TestFileTree() override;
private:
void testFileName();
......@@ -25,7 +27,7 @@ private:
void testFilePath();
private:
File *fl;
std::unique_ptr<File> fl;
};
#endif
......@@ -15,7 +15,7 @@ if (HAVE_SINCOS)
add_definitions(-DHAVE_SINCOS)
endif()
add_library(filelightInternal STATIC fileTree.cpp directoryIterator.cpp)
add_library(filelightInternal STATIC fileTree.cpp directoryIterator.cpp fileCleaner.cpp)
if (WIN32)
target_sources(filelightInternal PRIVATE windowsWalker.cpp)
else()
......
// SPDX-License-Identifier: GPL-2.0-only OR GPL-3.0-only OR LicenseRef-KDE-Accepted-GPL
// SPDX-FileCopyrightText: 2022 Harald Sitter <sitter@kde.org>
#include "fileCleaner.h"
#include <QScopeGuard>
#include <QThread>
void FileCleaner::clean(const QList<std::shared_ptr<File>> &files)
{
// move into our thread
QMetaObject::invokeMethod(
this,
[this, files] {
cleanUp(files);
},
Qt::QueuedConnection);
}
FileCleaner *FileCleaner::instance()
{
static auto cleaner = new FileCleaner;
static QThread thread;
auto *threadPtr = &thread;
static auto joinThread = qScopeGuard([threadPtr] { // join the thread before it gets stack unwound
threadPtr->quit();
threadPtr->wait();
});
static bool started = false;
if (!started) {
cleaner->moveToThread(&thread);
thread.start(QThread::IdlePriority);
}
return cleaner;
}
void FileCleaner::cleanUp(QList<std::shared_ptr<File>> files)
{
files.clear();
}
// SPDX-License-Identifier: GPL-2.0-only OR GPL-3.0-only OR LicenseRef-KDE-Accepted-GPL
// SPDX-FileCopyrightText: 2022 Harald Sitter <sitter@kde.org>
#pragma once
#include "fileTree.h"
// Tiny helper class to defer file deletion into a thread. The Files are being held until actual cleanup.
class FileCleaner : public QObject
{
public:
static FileCleaner *instance();
void clean(const QList<std::shared_ptr<File>> &files);
private:
using QObject::QObject;
Q_INVOKABLE void cleanUp(QList<std::shared_ptr<File>> files);
};
/***********************************************************************
* SPDX-FileCopyrightText: 2003-2004 Max Howell <max.howell@methylblue.com>
* SPDX-FileCopyrightText: 2008-2009 Martin Sandsmark <martin.sandsmark@kde.org>
* SPDX-FileCopyrightText: 2017 Harald Sitter <sitter@kde.org>
* SPDX-FileCopyrightText: 2017-2022 Harald Sitter <sitter@kde.org>
*
* SPDX-License-Identifier: GPL-2.0-only OR GPL-3.0-only OR LicenseRef-KDE-Accepted-GPL
***********************************************************************/
......@@ -11,13 +11,27 @@
#include <QDir>
#include <QUrl>
#include "fileCleaner.h"
Folder::~Folder()
{
// Trees can be partially destroyed, make sure we remove the reference up to us, it's not a smart pointer
// because that'd risk causing a loop.
if (!m_parent) {
for (auto &file : files) {
file->m_parent = nullptr;
}
}
FileCleaner::instance()->clean(files);
}
QString File::displayName() const
{
const QString decodedName = QString::fromUtf8(m_name);
return url().isLocalFile() ? QDir::toNativeSeparators(decodedName) : decodedName;
}
QString File::displayPath(const Folder *root) const
QString File::displayPath(const std::shared_ptr<Folder> &root) const
{
// Use QUrl to sanitize the path for display and then run it through
// QDir to make sure we use native path separators.
......@@ -26,15 +40,13 @@ QString File::displayPath(const Folder *root) const
return url.isLocalFile() ? QDir::toNativeSeparators(cleanPath) : cleanPath;
}
QUrl File::url(const Folder *root) const
QUrl File::url(const std::shared_ptr<Folder> &root) const
{
QString path;
if (root == this) {
root = nullptr; // prevent returning empty string when there is something we could return
}
for (const File *d = this; d != root && d; d = d->parent()) {
// prevent returning empty string when there is something we could return
const auto rootPtr = root.get() != this ? root.get() : nullptr;
for (const File *d = this; d != rootPtr && d; d = d->parent()) {
path.prepend(QString::fromUtf8(d->name8Bit()));
}
......
......@@ -12,30 +12,28 @@
#include <KFormat>
#include <QByteArray> //qstrdup
#include <QByteArray>
using FileSize = quint64;
using DirSize = quint64; //**** currently unused
class Folder;
class File : public QObject
class File
{
public:
friend class Folder;
Q_OBJECT
public:
File(const char *name, FileSize size)
: m_parent(nullptr)
, m_name(qstrdup(name))
, m_name(name)
, m_size(size)
{
}
~File() override
{
delete[] m_name;
}
virtual ~File() = default;
File(const File &) = default;
File &operator=(const File &) = default;
File(File &&) = default;
File &operator=(File &&) = default;
Folder *parent() const
{
......@@ -45,12 +43,11 @@ public:
/** Do not use for user visible strings. Use name instead. */
const char *name8Bit() const
{
return m_name;
return m_name.constData();
}
void setName(const QByteArray &newName)
{
delete[] m_name;
m_name = qstrdup(newName.constData());
m_name = newName;
}
/** Decoded name. Use when you need a QString. */
QString decodedName() const
......@@ -77,45 +74,41 @@ public:
* Human readable path for display (including native separators where applicable.
* Only use for display.
*/
QString displayPath(const Folder * = nullptr) const;
QString displayPath(const std::shared_ptr<Folder> &root = {}) const;
QString humanReadableSize() const
{
return KFormat().formatByteSize(m_size);
}
/** Builds a complete QUrl by walking up to root. */
QUrl url(const Folder *root = nullptr) const;
QUrl url(const std::shared_ptr<Folder> &root = {}) const;
protected:
File(const char *name, FileSize size, Folder *parent)
: m_parent(parent)
, m_name(qstrdup(name))
, m_name(name)
, m_size(size)
{
}
Folder *m_parent; // 0 if this is treeRoot
char *m_name; // partial path name (e.g. 'boot/' or 'foo.svg')
Folder *m_parent; // 0 if this is treeRoot; this is a non-owning pointer, the parent owns "us"
QByteArray m_name; // partial path name (e.g. 'boot/' or 'foo.svg')
FileSize m_size; // in units of bytes; sum of all children's sizes
private:
Q_DISABLE_COPY_MOVE(File)
};
class Folder : public File
{
Q_OBJECT
Q_PROPERTY(uint children MEMBER m_children CONSTANT)
public:
explicit Folder(const char *name)
: File(name, 0)
{
} // DON'T pass the full path!
~Folder() override
{
qDeleteAll(files);
}
~Folder();
Folder(const Folder &) = default;
Folder &operator=(const Folder &) = default;
Folder(Folder &&) = default;
Folder &operator=(Folder &&) = default;
uint children() const
{
......@@ -126,45 +119,36 @@ public:
return true;
}
Folder *duplicate() const
std::shared_ptr<Folder> duplicate() const
{
auto ret = new Folder(m_name);
for (const File *child : files) {
if (child->isFolder()) {
ret->append(((Folder *)child)->duplicate());
} else {
ret->append(child->name8Bit(), child->size());
}
}
return ret;
return std::make_shared<Folder>(*this);
}
/// appends a Folder
void append(Folder *d, const char *name = nullptr)
void append(const std::shared_ptr<Folder> &d, const char *name = nullptr)
{
if (name) {
delete[] d->m_name;
d->m_name = qstrdup(name);
d->m_name = name;
} // directories that had a fullpath copy just their names this way
m_children += d->children(); // doesn't include the dir itself
Q_ASSERT(d->m_parent == this || d->m_parent == nullptr);
d->m_parent = this;
append((File *)d); // will add 1 to filecount for the dir itself
appendFile(d); // will add 1 to filecount for the dir itself
}
/// appends a File
void append(const char *name, FileSize size)
{
append(new File(name, size, this));
appendFile(std::shared_ptr<File>(new File(name, size, this)));
}
/// removes a file
void remove(const File *f)
void remove(const std::shared_ptr<Folder> &f)
{
files.removeAll(const_cast<File *>(f));
files.removeAll(f);
const FileSize childSize = f->size();
delete f;
for (Folder *d = this; d; d = d->parent()) {
d->m_size -= childSize;
d->m_children--;
......@@ -172,11 +156,10 @@ public:
}
// Removes a file, but does not delete
const File *take(const File *f)
std::shared_ptr<Folder> take(const std::shared_ptr<Folder> &f)
{
files.removeAll(const_cast<File *>(f));
files.removeAll(f);
const FileSize childSize = f->size();
for (Folder *d = this; d; d = d->parent()) {
d->m_size -= childSize;
d->m_children--;
......@@ -184,10 +167,10 @@ public:
return f;
}
QList<File *> files;
QList<std::shared_ptr<File>> files;
private:
void append(File *p)
void appendFile(const std::shared_ptr<File> &p)
{
// This is also called by append(Folder), but only once all its children
// were scanned. We do not need to forward the size change to our parent
......@@ -199,7 +182,4 @@ private:
}
uint m_children = 0;
private:
Q_DISABLE_COPY_MOVE(Folder)
};
......@@ -26,7 +26,7 @@ namespace Filelight
QStringList LocalLister::s_remoteMounts;
QStringList LocalLister::s_localMounts;
LocalLister::LocalLister(const QString &path, QList<Folder *> *cachedTrees, ScanManager *parent)
LocalLister::LocalLister(const QString &path, QList<std::shared_ptr<Folder>> *cachedTrees, ScanManager *parent)
: m_path(path)
, m_trees(cachedTrees)
, m_parent(parent)
......@@ -48,7 +48,7 @@ LocalLister::LocalLister(const QString &path, QList<Folder *> *cachedTrees, Scan
if (!folderName.endsWith(QLatin1Char('/'))) {
folderName += QLatin1Char('/');
}
m_trees->append(new Folder(folderName.toLocal8Bit().constData()));
m_trees->append(std::make_shared<Folder>(folderName.toLocal8Bit().constData()));
}
}
}
......@@ -59,7 +59,7 @@ void LocalLister::run()
timer.start();
// recursively scan the requested path
const QByteArray path = m_path.toUtf8();
Folder *tree = scan(path, path);
auto tree = scan(path, path);
static constexpr auto msToS = 1000; // not worth using std::chrono for this single line
qCDebug(FILELIGHT_LOG) << "Scan completed in" << (timer.elapsed() / msToS);
......@@ -71,7 +71,6 @@ void LocalLister::run()
if (m_parent->m_abort) // scan was cancelled
{
qCDebug(FILELIGHT_LOG) << "Scan successfully aborted";
delete tree;
tree = nullptr;
}
qCDebug(FILELIGHT_LOG) << "Emitting signal to cache results ...";
......@@ -79,9 +78,9 @@ void LocalLister::run()
qCDebug(FILELIGHT_LOG) << "Thread terminating ...";
}
Folder *LocalLister::scan(const QByteArray &path, const QByteArray &dirname)
std::shared_ptr<Folder> LocalLister::scan(const QByteArray &path, const QByteArray &dirname)
{
auto cwd = new Folder(dirname.constData());
auto cwd = std::make_shared<Folder>(dirname.constData());
QVector<QPair<QByteArray, QByteArray>> subDirectories;
for (const auto &entry : DirectoryIterator(path)) {
......@@ -97,16 +96,16 @@ Folder *LocalLister::scan(const QByteArray &path, const QByteArray &dirname)
cwd->append(entry.name.constData(), entry.size);
m_parent->m_totalSize += entry.size;
} else if (entry.isDir) {
Folder *d = nullptr;
std::shared_ptr<Folder> d = nullptr;
const QByteArray new_dirname = entry.name + QByteArrayLiteral("/");
const QByteArray new_path = path + entry.name + '/';
// check to see if we've scanned this section already
QMutexLocker lock(&m_treeMutex);
for (Folder *folder : std::as_const(*m_trees)) {
for (const auto /* hold the folder, we delete it below! */ folder : std::as_const(*m_trees)) {
if (new_path == folder->name8Bit()) {
qDebug() << "Tree pre-completed: " << folder->decodedName();
qCDebug(FILELIGHT_LOG) << "Tree pre-completed: " << folder->decodedName() << folder.get();
d = folder;
m_trees->removeAll(folder);
m_parent->m_files += folder->children();
......@@ -116,7 +115,7 @@ Folder *LocalLister::scan(const QByteArray &path, const QByteArray &dirname)
lock.unlock();
if (!d) { // then scan
// qDebug() << "Tree fresh" <<new_path << new_dirname;
qCDebug(FILELIGHT_LOG) << "Tree fresh" << new_path << new_dirname;
subDirectories.append({new_path, new_dirname});
}
}
......@@ -127,7 +126,7 @@ Folder *LocalLister::scan(const QByteArray &path, const QByteArray &dirname)
// Scan all subdirectories, either in separate threads or immediately,
// depending on how many free threads there are in the threadpool.
// Yes, it isn't optimal, but it's better than nothing and pretty simple.
QVector<Folder *> returnedCwds(subDirectories.count());
QVector<std::shared_ptr<Folder>> returnedCwds(subDirectories.count());
QSemaphore semaphore;
for (int i = 0; i < subDirectories.count(); i++) {
std::function<void()> scanSubdir = [this, i, &subDirectories, &semaphore, &returnedCwds]() {
......@@ -139,13 +138,13 @@ Folder *LocalLister::scan(const QByteArray &path, const QByteArray &dirname)
}
}
semaphore.acquire(subDirectories.count());
for (Folder *d : std::as_const(returnedCwds)) {
for (const auto &d : std::as_const(returnedCwds)) {
if (d) { // then scan was successful
cwd->append(d);
}
}
std::sort(cwd->files.begin(), cwd->files.end(), [](File *a, File *b) {
std::sort(cwd->files.begin(), cwd->files.end(), [](const auto &a, const auto &b) {
return a->size() > b->size();
});
......
/***********************************************************************
* SPDX-FileCopyrightText: 2003-2004 Max Howell <max.howell@methylblue.com>
* SPDX-FileCopyrightText: 2008-2009 Martin Sandsmark <martin.sandsmark@kde.org>
* SPDX-FileCopyrightText: 2022 Harald Sitter <sitter@kde.org>
*
* SPDX-License-Identifier: GPL-2.0-only OR GPL-3.0-only OR LicenseRef-KDE-Accepted-GPL
***********************************************************************/
......@@ -22,25 +23,37 @@ class LocalLister : public QThread
Q_OBJECT
public:
LocalLister(const QString &path, QList<Folder *> *cachedTrees, ScanManager *parent);
LocalLister(const QString &path, QList<std::shared_ptr<Folder>> *cachedTrees, ScanManager *parent);
static void readMounts();
Q_SIGNALS:
void branchCompleted(Folder *tree);
void branchCompleted(std::shared_ptr<Folder> tree);
private:
QString m_path;
QMutex m_treeMutex;
QList<Folder *> *m_trees;
QList<std::shared_ptr<Folder>> *m_trees;
ScanManager *m_parent;
private:
void run() override;
Folder *scan(const QByteArray &, const QByteArray &);
std::shared_ptr<Folder> scan(const QByteArray &, const QByteArray &);
private:
static QStringList s_localMounts;
static QStringList s_remoteMounts; // TODO namespace
};
} // namespace Filelight
namespace std
{
template<>
struct default_delete<Filelight::LocalLister> {
void operator()(Filelight::LocalLister *ptr) const
{
// It's a QThread, delete on its own event loop, not the deleters. Ensures against data races.
ptr->deleteLater();
}
};
} // namespace std
......@@ -20,6 +20,8 @@
#include <KLocalizedString>
#include <KSharedConfig>
#include "fileTree.h"
int main(int argc, char *argv[])
{
#if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)
......@@ -31,6 +33,9 @@ int main(int argc, char *argv[])
#endif
QApplication app(argc, argv);
qRegisterMetaType<std::shared_ptr<File>>("std::shared_ptr<File>");
qRegisterMetaType<std::shared_ptr<Folder>>("std::shared_ptr<Folder>");
if (qEnvironmentVariableIsEmpty("QT_QUICK_CONTROLS_STYLE")) {
QQuickStyle::setStyle(QStringLiteral("org.kde.desktop"));
}
......
......@@ -264,6 +264,16 @@ void MainContext::addHistoryAction(QObject *action)
Q_EMIT historyActionsChanged();
}
void MainContext::connectMapItem(QObject *mapItem) const
{ // we transfer shared_ptrs around, let's not do it via qml, we can't check if it is nullptr there
auto item = qobject_cast<RadialMap::Item *>(mapItem);
Q_ASSERT(item);
connect(m_manager, &ScanManager::completed, item, [item](const auto &tree) {
if (tree) {
item->create(tree);
}
});
}
} // namespace Filelight
#include "mainContext.moc"
......@@ -66,6 +66,7 @@ public Q_SLOTS:
void updateURL(const QUrl &);
void rescanSingleDir(const QUrl &) const;
void connectMapItem(QObject *mapItem) const;
private:
void setupActions(QQmlApplicationEngine *engine);
......
......@@ -90,9 +90,9 @@ Kirigami.Page {
Layout.minimumHeight: Kirigami.Units.gridUnit
Layout.minimumWidth: Kirigami.Units.gridUnit
onFolderCreated: (tree) => {
onFolderCreated: {
appWindow.completed()
appWindow.mapChanged(tree)
appWindow.mapChanged()
appWindow.status = ""
}
onActivated: (url) => {
......
......@@ -20,6 +20,8 @@ Kirigami.ApplicationWindow {
property var mapItem: null
property var mapPage: null
onMapItemChanged: MainContext.connectMapItem(mapItem)
Kirigami.Action {
id: scanFolderAction
iconName: "folder"
......@@ -138,9 +140,9 @@ Kirigami.ApplicationWindow {
MainContext.slotUp()