Commit 5d72be1b authored by Harald Sitter's avatar Harald Sitter 🏳️‍🌈
Browse files

refactor the remotelister to not rely on raw pointers so much

safer this way. there are various crashes on errors.ubuntu.com and the
microsoft partner center that ultimately boil down to the raw pointer
shuffling we are doing everywhere. so we should be getting away from
that since clearly we aren't doing a good job of managing them and the
modern cpp approach is to not use owning raw pointers anyway so the less
we do it the better

also propagate is now looping instead of descending in on itself, the
latter risks running into stack exhaustion for particularly deep
directory trees. to support that it's also become a free function so we
don't accidentally look at the wrong pointer (i.e. `this`)
parent b7744471
Pipeline #188478 passed with stage
in 3 minutes and 57 seconds
......@@ -16,58 +16,52 @@
namespace Filelight
{
// TODO: delete all this stuff!
// You need to use a single DirLister.
// One per folder breaks KIO (seemingly) and also uses un-godly amounts of memory!
struct Store {
using List = QList<Store *>;
/// location of the folder
const QUrl url;
/// the folder on which we are operating
Folder *folder = nullptr;
Folder *folder;
/// so we can reference the parent store
Store *parent = nullptr;
const std::shared_ptr<Store> parent = nullptr;
/// directories in this folder that need to be scanned before we can propagate()
List stores;
QList<std::shared_ptr<Store>> stores;
Store(const QUrl &u, const QString &name, Store *s)
: url(u)
Store(const QUrl &url_, const QString &name, const std::shared_ptr<Store> &parentStore)
: url(url_)
, folder(new Folder((name + QLatin1Char('/')).toUtf8().constData()))
, parent(s)
, parent(parentStore)
{
}
~Store() = default;
Store *propagate()
{
/// returns the next store available for scanning
private:
Q_DISABLE_COPY_MOVE(Store)
};
qCDebug(FILELIGHT_LOG) << "propagate: " << url;
std::shared_ptr<Store> propagate(Store *store, const std::shared_ptr<Store> &root)
{
/// returns the next store available for scanning (or the root itself)
if (parent) {
parent->folder->append(folder);
if (parent->stores.isEmpty()) {
return parent->propagate();
}
return parent;
while (store->parent) {
qCDebug(FILELIGHT_LOG) << "propagate: " << store->url;
store->parent->folder->append(store->folder);
if (!store->parent->stores.isEmpty()) {
return store->parent;
}
// we reached the root, let's get our next folder scanned
return this;
store = store->parent.get();
}
private:
Q_DISABLE_COPY_MOVE(Store)
};
return root;
}
RemoteLister::RemoteLister(const QUrl &url, ScanManager *manager, QObject *parent)
RemoteLister::RemoteLister(const QUrl &url, ScanManager *parent)
: KDirLister(parent)
, m_root(new Store(url, url.url(), nullptr))
, m_root(std::make_shared<Store>(url, url.url(), nullptr))
, m_store(m_root)
, m_manager(manager)
, m_manager(parent)
{
setShowingDotFiles(true); // Stupid KDirLister API function names
......@@ -76,16 +70,10 @@ RemoteLister::RemoteLister(const QUrl &url, ScanManager *manager, QObject *paren
connect(this, static_cast<void (KCoreDirLister::*)()>(&KCoreDirLister::canceled), this, &RemoteLister::onCanceled);
}
RemoteLister::~RemoteLister()
{
delete m_root;
}
void RemoteLister::onCanceled()
{
qCDebug(FILELIGHT_LOG) << "Canceled";
Q_EMIT branchCompleted(nullptr);
deleteLater();
}
void RemoteLister::onCompleted()
......@@ -95,7 +83,7 @@ void RemoteLister::onCompleted()
const KFileItemList items = KDirLister::items();
for (const auto &item : items) {
if (item.isDir()) {
m_store->stores += new Store(item.url(), item.name(), m_store);
m_store->stores << std::make_shared<Store>(item.url(), item.name(), m_store);
} else {
m_store->folder->append(item.name().toUtf8().constData(), item.size());
m_manager->m_totalSize += item.size();
......@@ -108,35 +96,24 @@ void RemoteLister::onCompleted()
// no directories to scan, so we need to append ourselves to the parent folder
// propagate() will return the next ancestor that has stores left to be
// scanned, or root if we are done
Store *newStore = m_store->propagate();
if (newStore != m_store) {
if (auto newStore = propagate(m_store.get(), m_root); newStore != m_store) {
// We need to clean up old stores
delete m_store;
m_store = newStore;
}
}
if (!m_store->stores.isEmpty()) {
Store::List::Iterator first = m_store->stores.begin();
const QUrl url((*first)->url);
Store *currentStore = m_store;
// we should operate with this store next time this function is called
m_store = *first;
m_store = m_store->stores.takeFirst();
// we don't want to handle this store again
currentStore->stores.erase(first);
// this returns _immediately_
const auto url = m_store->url;
qCDebug(FILELIGHT_LOG) << "scanning: " << url;
openUrl(url);
} else {
qCDebug(FILELIGHT_LOG) << "I think we're done";
Q_ASSERT(m_root == m_store);
Q_EMIT branchCompleted(m_store->folder);
deleteLater();
return;
}
qCDebug(FILELIGHT_LOG) << "I think we're done";
Q_ASSERT(m_root == m_store);
Q_EMIT branchCompleted(m_store->folder);
}
}
} // namespace Filelight
......@@ -8,19 +8,22 @@
#pragma once
#include "fileTree.h"
#include <memory>
#include <KDirLister>
#include "fileTree.h"
namespace Filelight
{
class ScanManager;
struct Store;
class RemoteLister : public KDirLister
{
Q_OBJECT
public:
RemoteLister(const QUrl &url, ScanManager *manager, QObject *parent = nullptr);
~RemoteLister() override;
RemoteLister(const QUrl &url, ScanManager *parent);
Q_SIGNALS:
void branchCompleted(Folder *tree);
......@@ -30,10 +33,8 @@ private Q_SLOTS:
void onCanceled();
private:
struct Store *m_root;
struct Store *m_store;
std::shared_ptr<Store> m_root;
std::shared_ptr<Store> m_store;
ScanManager *m_manager;
Q_DISABLE_COPY_MOVE(RemoteLister)
};
} // namespace Filelight
......@@ -70,10 +70,16 @@ bool ScanManager::start(const QUrl &url)
if (!url.isLocalFile()) {
QGuiApplication::changeOverrideCursor(Qt::BusyCursor);
// will start listing straight away
m_remoteLister = std::make_unique<Filelight::RemoteLister>(url, this, this);
m_remoteLister = std::make_unique<Filelight::RemoteLister>(url, this);
connect(m_remoteLister.get(), &Filelight::RemoteLister::branchCompleted, this, &ScanManager::cacheTree, Qt::QueuedConnection);
connect(m_remoteLister.get(), &Filelight::RemoteLister::completed, this, &ScanManager::runningChanged);
m_remoteLister->setObjectName(QStringLiteral("remote_lister"));
auto updateRunning = [this] {
if (m_remoteLister && m_remoteLister->isFinished()) {
m_remoteLister = nullptr;
Q_EMIT runningChanged();
}
};
connect(m_remoteLister.get(), &Filelight::RemoteLister::completed, this, updateRunning);
connect(m_remoteLister.get(), &Filelight::RemoteLister::canceled, this, updateRunning);
m_remoteLister->openUrl(url);
Q_EMIT runningChanged();
return true;
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment