michaelh added a subscriber: mgallien. michaelh added a comment.
Under the premise that I still have to learn about baloo's inner workings, here are some concerns: - I'm not convinced, that index cleaning should be part of the dbus interface. Why should cleaning be done out of process? Do we really want any application to trigger database manipulation? Could elaborate your rationale please? - Which application is using baloo dbus interface anyway and which functions? @mgallien How is elisa using it? - Purging stale entries is by far not enough to get the index in a good shape. As soon as removable drives or network shares are involved all sorts of weird things can happen. IndexCleaner is much too simple to account for that. - Looks like indexcleaner has been dead code until now. Have you read Vishesh's commit message? https://cgit.kde.org/baloo.git/commit/?id=ea2afe88b0c4299d7540e5b6c8b8e46858336f0c Do we have to be concerned about his note? INLINE COMMENTS > fileindexscheduler.cpp:140 > + > + if (m_purgeDeindexableFiles) { > + auto runnable = new IndexCleaner(m_db, m_config); Same as above > fileindexscheduler.cpp:225 > > +void FileIndexScheduler::purgeDeindexableFiles() > +{ Same as above > fileindexscheduler.cpp:227 > +{ > + m_purgeDeindexableFiles = true; > + scheduleIndexing(); Same as above > fileindexscheduler.h:83 > Q_SCRIPTABLE void checkUnindexedFiles(); > + Q_SCRIPTABLE void purgeDeindexableFiles(); > Q_SCRIPTABLE uint getBatchSize(); Nitpick: We're removing db entries not files. Also every file is deindexable. Maybe 'purgeStaleEntries', 'removeLostEntries' or so would be a better name to describe what's going on. > fileindexscheduler.h:110 > bool m_checkUnindexedFiles; > + bool m_purgeDeindexableFiles; > }; Same as above > indexcleaner.cpp:44 > { > QMimeDatabase mimeDb; > Transaction tr(m_db, Transaction::ReadWrite); Unrelated whitespace change > indexcleaner.cpp:80 > + for (const QString& folder : m_config->includeFolders()) { > + if (folder.startsWith(device.mountPath() + "/")) { > + quint64 id = filePathToId(QFile::encodeName(folder)); You can use `QStringLiteral("%1/").arg(device.mountPath())` here and maybe define it outside the loop > main.cpp:85 > parser.addPositionalArgument(QStringLiteral("resume"), i18n("Resume the > file indexer")); > - parser.addPositionalArgument(QStringLiteral("check"), i18n("Check for > any unindexed files and index them")); > + parser.addPositionalArgument(QStringLiteral("check"), i18n("Check for > changes in the monitored folders")); > parser.addPositionalArgument(QStringLiteral("index"), i18n("Index the > specified files")); Something like "Update the index. Remove stale entries and add new files." would describe the actions taken more clearly. > main.cpp:197 > out << "Started search for unindexed files\n"; > + schedulerinterface.purgeDeindexableFiles(); > + out << "Started search for deindexable files\n"; Do purging before indexing. Sometimes the parent id of a file is lost (Why?) and they appear as `//foo.bar` if that is deleted first it can be reindexed. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D11529 To: smithjd, #baloo, vhanda, michaelh Cc: mgallien, #frameworks, ashaposhnikov, michaelh, astippich, spoorun, nicolasfella, ngraham, alexeymin
