-
Notifications
You must be signed in to change notification settings - Fork 31
core/desktopentry: add file system watcher for desktop entry directories #114
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
base: master
Are you sure you want to change the base?
Conversation
I'm generally conflicted on this one because I prefer state change notifications to manually triggered scanning, which often leads to polling.
Looks like its just a simple directory watch, but it likely has to be recursive on every potential desktop entry dir, otherwise it's unlikely to handle nix generation switches and such. This is the general direction I'd like to take it in. No pressure on you to implement it if you don't want to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you've marked it as WIP. Partial review
src/core/desktopentrymonitor.cpp
Outdated
void DesktopEntryMonitor::addDirectoryHierarchy(const QString& dirPath) { | ||
this->scanAndWatch(dirPath); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
src/core/desktopentrymonitor.cpp
Outdated
this->watcher = new QFileSystemWatcher(this); | ||
this->debounceTimer = new QTimer(this); | ||
this->debounceTimer->setSingleShot(true); | ||
this->debounceTimer->setInterval(500); // 500ms debounce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
500ms is unreasonably long for a debounce
src/core/desktopentrymonitor.cpp
Outdated
connect( | ||
this->watcher, | ||
&QFileSystemWatcher::directoryChanged, | ||
this, | ||
&DesktopEntryMonitor::onDirectoryChanged | ||
); | ||
connect( | ||
this->watcher, | ||
&QFileSystemWatcher::fileChanged, | ||
this, | ||
&DesktopEntryMonitor::onFileChanged | ||
); | ||
connect(this->debounceTimer, &QTimer::timeout, this, &DesktopEntryMonitor::processChanges); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QObject::connect
src/core/desktopentrymonitor.cpp
Outdated
} | ||
|
||
void DesktopEntryMonitor::scanAndWatch(const QString& dirPath) { | ||
QDir dir(dirPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disallowed style, use auto
this->scanAndWatch(dirPath); | ||
} | ||
|
||
void DesktopEntryMonitor::scanAndWatch(const QString& dirPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scanAndWatch + all the various change handlers share a lot of the code. consider initialization to be a diff from an empty set of entries and share the code.
src/core/desktopentrymonitor.cpp
Outdated
if (watchedFile.startsWith(path + "/") || watchedFile == path) { | ||
this->watcher->removePath(watchedFile); | ||
this->fileTimestamps.remove(watchedFile); | ||
this->queueChange(ChangeEvent::Removed, watchedFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The debounce is not used in a logical way. If you are going to debounce, it should guard re-scanning directories, not model updates.
src/core/desktopentrymonitor.cpp
Outdated
for (auto it = this->watchedFiles.begin(); it != this->watchedFiles.end();) { | ||
const QString& watchedFile = *it; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we tracking watched files, the fs watcher already does that
src/core/desktopentrymonitor.cpp
Outdated
return; | ||
} | ||
|
||
QList<QString> currentFiles = dir.entryList({"*.desktop"}, QDir::Files); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto
@outfoxxed I addressed the comments, I had it initially watching .desktop files and then decided against it, to just watch the directories and rescan when there's a change instead. Sorry for the confusion, not wip anymore - ready for review |
De-dupe code paths to scanAndWatch function, Optimize lookups by using a QSet, remove paths from watcher if deleted
- Create less fs watchers - Just re-scan when a directory containing .desktop files changes
Would there be anything holding this one up for primetime? It will be quite useful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional notes:
- Please don't add useless comments
- Please do scanning triggered by a change in another thread via QRunnable. The desktop entry scanner takes long enough to run that it may have a noticeable stutter on some systems without it.
void DesktopEntryMonitor::onDirectoryChanged(const QString& path) { | ||
qCDebug(logDesktopMonitor) << "Directory changed:" << path; | ||
auto dir = QDir(path); | ||
|
||
// Check if directory still exists - handle removal/unmounting | ||
if (!dir.exists()) { | ||
qCDebug(logDesktopMonitor) << "Directory no longer exists, cleaning up:" << path; | ||
this->watcher->removePath(path); | ||
// Directory removal will be handled by full rescan | ||
this->queueChange(ChangeEvent::Modified, path); // Trigger full rescan | ||
return; | ||
} | ||
|
||
// Check for new subdirectories | ||
auto subdirs = dir.entryList(QDir::Dirs | QDir::NoDotAndDotDot); | ||
for (const QString& subdir: subdirs) { | ||
auto subdirPath = dir.absoluteFilePath(subdir); | ||
if (!this->watcher->directories().contains(subdirPath)) { | ||
this->scanAndWatch(subdirPath); | ||
} | ||
} | ||
|
||
// Queue a change to trigger full rescan of all desktop paths | ||
this->queueChange(ChangeEvent::Modified, path); | ||
} | ||
|
||
void DesktopEntryMonitor::queueChange(ChangeEvent event, const QString& path) { | ||
this->pendingChanges.insert(path, event); | ||
this->debounceTimer->start(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of pending changes or tracking, if we're going to do a full rescan anyway then this should do nothing more than set a flag, and potentially bump the debounce timer for a rescan. The timer should be no more than 100ms
void DesktopEntryManager::handleFileChanges( | ||
const QHash<QString, DesktopEntryMonitor::ChangeEvent>& | ||
) { | ||
qCDebug(logDesktopEntry) << "Directory change detected, performing full rescan"; | ||
|
||
auto oldEntries = this->desktopEntries; | ||
|
||
this->desktopEntries.clear(); | ||
this->lowercaseDesktopEntries.clear(); | ||
|
||
this->scanDesktopEntries(); | ||
|
||
QVector<DesktopEntry*> newApplications; | ||
for (auto& entry: this->desktopEntries.values()) { | ||
if (!entry->noDisplay()) { | ||
newApplications.append(entry); | ||
} | ||
} | ||
|
||
this->mApplications.diffUpdate(newApplications); | ||
|
||
for (auto* e: oldEntries) { | ||
if (!this->desktopEntries.contains(e->mId)) { | ||
e->deleteLater(); | ||
} | ||
} | ||
|
||
emit applicationsChanged(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as directory changed
#include <qstringlist.h> | ||
|
||
namespace DesktopUtils { | ||
QList<QString> getDesktopDirectories(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why this was extracted into a new file
DesktopEntries only scans one time~~, and doesnt expose scanning functions to QML. This adds a
rescan
function, a signal for applications changed. It just allows a nicer launcher that doesn't get stale.~~This adds:
DesktopUtils
which had a common function for locating.desktop
filesDesktopEntryMonitor
which implements QFileSystemWatcherDesktopEntryManager
and add a new signalapplicationsChanged