Differential D7995 Diff 20162 kdevplatform/project/abstractfilemanagerplugin.cpp

Changeset View

Standalone View

View Options

kdevplatform/project/abstractfilemanagerplugin.cpp

1/*************************************************************************** 1​/***************************************************************************
2 * This file is part of KDevelop * 2​ * This file is part of KDevelop *
3 * Copyright 2010-2012 Milian Wolff <mail@milianw.de> * 3​ * Copyright 2010-2012 Milian Wolff <mail@milianw.de> *
4 * * 4​ * *
5 * This program is free software; you can redistribute it and/or modify * 5​ * This program is free software; you can redistribute it and/or modify *
6 * it under the terms of the GNU Library General Public License as * 6​ * it under the terms of the GNU Library General Public License as *
7 * published by the Free Software Foundation; either version 2 of the * 7​ * published by the Free Software Foundation; either version 2 of the *
8 * License, or (at your option) any later version. * 8​ * License, or (at your option) any later version. *
9 * * 9​ * *
10 * This program is distributed in the hope that it will be useful, * 10​ * This program is distributed in the hope that it will be useful, *
11 * but WITHOUT ANY WARRANTY; without even the implied warranty of * 11​ * but WITHOUT ANY WARRANTY; without even the implied warranty of *
12 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * 12​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
mwolffUnsubmitted
Done

remove

mwolff: remove
13 * GNU General Public License for more details. * 13​ * GNU General Public License for more details. *
14 * * 14​ * *
15 * You should have received a copy of the GNU Library General Public * 15​ * You should have received a copy of the GNU Library General Public *
16 * License along with this program; if not, write to the * 16​ * License along with this program; if not, write to the *
17 * Free Software Foundation, Inc., * 17​ * Free Software Foundation, Inc., *
18 * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * 18​ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. *
19 ***************************************************************************/ 19​ ***************************************************************************/
20 20​
21#include "abstractfilemanagerplugin.h" 21​#include "abstractfilemanagerplugin.h"
22 22​
23#include "filemanagerlistjob.h" 23​#include "filemanagerlistjob.h"
24#include "projectmodel.h" 24​#include "projectmodel.h"
25#include "helper.h" 25​#include "helper.h"
26 26​
27#include <QHashIterator> 27​#include <QHashIterator>
28#include <QFileInfo> 28​#include <QFileInfo>
29#include <QApplication> 29​#include <QApplication>
30​#ifdef TIME_IMPORT_JOB
31​#include <QElapsedTimer>
32​#endif
30 33​
31#include <KMessageBox> 34​#include <KMessageBox>
32#include <KLocalizedString> 35​#include <KLocalizedString>
33#include <KDirWatch> 36​#include <KDirWatch>
34 37​
35#include <interfaces/iproject.h> 38​#include <interfaces/iproject.h>
36#include <interfaces/icore.h> 39​#include <interfaces/icore.h>
37#include <interfaces/iprojectcontroller.h> 40​#include <interfaces/iprojectcontroller.h>
38#include <serialization/indexedstring.h> 41​#include <serialization/indexedstring.h>
39 42​
40#include "projectfiltermanager.h" 43​#include "projectfiltermanager.h"
44​#include "projectwatcher.h"
41#include "debug.h" 45​#include "debug.h"
42 46​
43#define ifDebug(x) 47​#define ifDebug(x)
44 48​
45using namespace KDevelop; 49​using namespace KDevelop;
46 50​
47//BEGIN Helper 51​//BEGIN Helper
48 52​
Show All 32 Lines75public:
81 * to have any affect. The job will then auto-delete itself upon completion. 85​ * to have any affect. The job will then auto-delete itself upon completion.
82 */ 86​ */
83 Q_REQUIRED_RESULT KIO::Job* eventuallyReadFolder(ProjectFolderItem* item); 87​ Q_REQUIRED_RESULT KIO::Job* eventuallyReadFolder(ProjectFolderItem* item);
84 void addJobItems(FileManagerListJob* job, 88​ void addJobItems(FileManagerListJob* job,
85 ProjectFolderItem* baseItem, 89​ ProjectFolderItem* baseItem,
86 const KIO::UDSEntryList& entries); 90​ const KIO::UDSEntryList& entries);
87 91​
88 void deleted(const QString &path); 92​ void deleted(const QString &path);
89 void created(const QString &path); 93​ void dirty(const QString &path);
90 94​
91 void projectClosing(IProject* project); 95​ void projectClosing(IProject* project);
92 void jobFinished(KJob* job); 96​ void jobFinished(KJob* job);
93 97​
94 /// Stops watching the given folder for changes, only useful for local files. 98​ /// Stops watching the given folder for changes, only useful for local files.
95 void stopWatcher(ProjectFolderItem* folder); 99​ void stopWatcher(ProjectFolderItem* folder);
96 /// Continues watching the given folder for changes. 100​ /// Continues watching the given folder for changes.
97 void continueWatcher(ProjectFolderItem* folder); 101​ void continueWatcher(ProjectFolderItem* folder);
98 /// Common renaming function. 102​ /// Common renaming function.
99 bool rename(ProjectBaseItem* item, const Path& newPath); 103​ bool rename(ProjectBaseItem* item, const Path& newPath);
100 104​
101 void removeFolder(ProjectFolderItem* folder); 105​ void removeFolder(ProjectFolderItem* folder);
102 106​
103 QHash<IProject*, KDirWatch*> m_watchers; 107​ QHash<IProject*, ProjectWatcher*> m_watchers;
104 QHash<IProject*, QList<FileManagerListJob*> > m_projectJobs; 108​ QHash<IProject*, QList<FileManagerListJob*> > m_projectJobs;
105 QVector<QString> m_stoppedFolders; 109​ QVector<QString> m_stoppedFolders;
106 ProjectFilterManager m_filters; 110​ ProjectFilterManager m_filters;
107}; 111​};
108 112​
109void AbstractFileManagerPluginPrivate::projectClosing(IProject* project) 113​void AbstractFileManagerPluginPrivate::projectClosing(IProject* project)
110{ 114​{
111 if ( m_projectJobs.contains(project) ) { 115​ if ( m_projectJobs.contains(project) ) {
112 // make sure the import job does not live longer than the project 116​ // make sure the import job does not live longer than the project
113 // see also addLotsOfFiles test 117​ // see also addLotsOfFiles test
114 foreach( FileManagerListJob* job, m_projectJobs[project] ) { 118​ foreach( FileManagerListJob* job, m_projectJobs[project] ) {
115 qCDebug(FILEMANAGER) << "killing project job:" << job; 119​ qCDebug(FILEMANAGER) << "killing project job:" << job;
116 job->abort(); 120​ job->abort();
117 } 121​ }
118 m_projectJobs.remove(project); 122​ m_projectJobs.remove(project);
119 } 123​ }
124​#ifdef TIME_IMPORT_JOB
125​ QElapsedTimer timer;
mwolffUnsubmitted
Not Done

you removed the wrong conditional - I added the comment to this line, the if for the m_watchers.contains() - this should be removed, _not_ the #ifdef for the actual timing feature (that should remain optional)

mwolff: you removed the wrong conditional - I added the comment to this line, the if for the…
126​ int watched;
mwolffUnsubmitted
Done

remove the conditionals, always output the time?

mwolff: remove the conditionals, always output the time?
rjvbbAuthorUnsubmitted
Not Done

fine with me, someone suggested I add the conditional ;)

rjvbb: fine with me, someone suggested I add the conditional ;)
127​ if (m_watchers.contains(project)) {
128​ watched = m_watchers[project]->size();
129​ timer.start();
130​ }
131​#endif
120 delete m_watchers.take(project); 132​ delete m_watchers.take(project);
133​#ifdef TIME_IMPORT_JOB
134​ if (timer.isValid()) {
135​ qCInfo(FILEMANAGER) << "Deleting" << watched << "entry dir watcher took" << timer.elapsed() / 1000.0 << "seconds for project" << project->name();
136​ }
mwolffUnsubmitted
Not Done
if (auto* watcher = m_watchers.take(project)) {
    delete watcher;
}
mwolff: if (auto* watcher = m_watchers.take(project)) { delete watcher; }
rjvbbAuthorUnsubmitted
Not Done

Dropped the size indication and went back to an earlier, simpler suggestion.

rjvbb: Dropped the size indication and went back to an earlier, simpler suggestion.
137​#endif
mwolffUnsubmitted
Done

the whole timer code must be hidden behind ifdefs, or better yet, removed overall and instead a benchmark should measure the time for that without littering the codebase here

mwolff: the whole timer code must be hidden behind ifdefs, or better yet, removed overall and instead a…
121 m_filters.remove(project); 138​ m_filters.remove(project);
mwolffUnsubmitted
Done

delete + take like before

mwolff: delete + take like before
mwolffUnsubmitted
Not Done

delete + take like before

not sure this comment was seen, phab says the above comment isn't saved/submitted. No clue how to resubmit it...

mwolff: delete + take like before not sure this comment was seen, phab says the above comment isn't…
rjvbbAuthorUnsubmitted
Not Done

It's done locally but I'm waiting for more info to include other requested changes in the next revision too.

rjvbb: It's done locally but I'm waiting for more info to include other requested changes in the next…
122} 139​}
123 140​
124KIO::Job* AbstractFileManagerPluginPrivate::eventuallyReadFolder(ProjectFolderItem* item) 141​KIO::Job* AbstractFileManagerPluginPrivate::eventuallyReadFolder(ProjectFolderItem* item)
125{ 142​{
143​ ProjectWatcher* watcher = m_watchers.value( item->project(), nullptr );
126 FileManagerListJob* listJob = new FileManagerListJob( item ); 144​ FileManagerListJob* listJob = new FileManagerListJob( item );
127 m_projectJobs[ item->project() ] << listJob; 145​ m_projectJobs[ item->project() ] << listJob;
mwolffUnsubmitted
Done

remove, always watch dirs

mwolff: remove, always watch dirs
128 qCDebug(FILEMANAGER) << "adding job" << listJob << item << item->path() << "for project" << item->project(); 146​ qCDebug(FILEMANAGER) << "adding job" << listJob << item << item->path() << "for project" << item->project();
129 147​
130 q->connect( listJob, &FileManagerListJob::finished, 148​ q->connect( listJob, &FileManagerListJob::finished,
131 q, [&] (KJob* job) { jobFinished(job); } ); 149​ q, [&] (KJob* job) { jobFinished(job); } );
132 150​
133 q->connect( listJob, &FileManagerListJob::entries, 151​ q->connect( listJob, &FileManagerListJob::entries,
134 q, [&] (FileManagerListJob* job, ProjectFolderItem* baseItem, const KIO::UDSEntryList& entries) { 152​ q, [&] (FileManagerListJob* job, ProjectFolderItem* baseItem, const KIO::UDSEntryList& entries) {
135 addJobItems(job, baseItem, entries); } ); 153​ addJobItems(job, baseItem, entries); } );
154​ q->connect( listJob, &FileManagerListJob::watchDir,
155​ q, [this, item, watcher] (const QString& path) {
mwolffUnsubmitted
Not Done

on a more serious note: your crash probably arises from this being bound to the AFMP lifetime - it won't get disconnected when the watcher gets killed. You probably want to use watcher instead of q as third param here.

still, doesn't change the fact that the whole watcher wrapper should be removed

mwolff: on a more serious note: your crash probably arises from this being bound to the AFMP lifetime…
156​ watcher->addDir(path); }, Qt::QueuedConnection );
mwolffUnsubmitted
Done

please document why queuing is required here? Qt should do this automatically, if you emit the signal from a background thread. I.e. it takes QThread::currentThread into account, and not the sender object's thread.

mwolff: please document why queuing is required here? Qt should do this automatically, if you emit the…
136 157​
137 return listJob; 158​ return listJob;
138} 159​}
139 160​
140void AbstractFileManagerPluginPrivate::jobFinished(KJob* job) 161​void AbstractFileManagerPluginPrivate::jobFinished(KJob* job)
141{ 162​{
142 FileManagerListJob* gmlJob = qobject_cast<FileManagerListJob*>(job); 163​ FileManagerListJob* gmlJob = qobject_cast<FileManagerListJob*>(job);
143 if (gmlJob) { 164​ if (gmlJob) {
▲ Show 20 Lines • Show All 98 Lines • ▼ Show 20 Line(s)262 foreach ( const Path& path, folders ) {
242 ProjectFolderItem* folder = q->createFolderItem( baseItem->project(), path, baseItem ); 263​ ProjectFolderItem* folder = q->createFolderItem( baseItem->project(), path, baseItem );
243 if (folder) { 264​ if (folder) {
244 emit q->folderAdded( folder ); 265​ emit q->folderAdded( folder );
245 job->addSubDir( folder ); 266​ job->addSubDir( folder );
246 } 267​ }
247 } 268​ }
248} 269​}
249 270​
250void AbstractFileManagerPluginPrivate::created(const QString& path_) 271​void AbstractFileManagerPluginPrivate::dirty(const QString& path_)
251{ 272​{
252 qCDebug(FILEMANAGER) << "created:" << path_; 273​ qCDebug(FILEMANAGER) << "dirty:" << path_;
253 QFileInfo info(path_); 274​ QFileInfo info(path_);
254 275​
255 ///FIXME: share memory with parent 276​ ///FIXME: share memory with parent
256 const Path path(path_); 277​ const Path path(path_);
257 const IndexedString indexedPath(path.pathOrUrl()); 278​ const IndexedString indexedPath(path.pathOrUrl());
258 const IndexedString indexedParent(path.parent().pathOrUrl()); 279​ const IndexedString indexedParent(path.parent().pathOrUrl());
259 280​
260 QHashIterator<IProject*, KDirWatch*> it(m_watchers); 281​ QHashIterator<IProject*, ProjectWatcher*> it(m_watchers);
261 while (it.hasNext()) { 282​ while (it.hasNext()) {
262 const auto p = it.next().key(); 283​ const auto p = it.next().key();
263 if ( !p->projectItem()->model() ) { 284​ if ( !p->projectItem()->model() ) {
264 // not yet finished with loading 285​ // not yet finished with loading
265 // FIXME: how should this be handled? see unit test 286​ // FIXME: how should this be handled? see unit test
266 continue; 287​ continue;
267 } 288​ }
268 if ( !q->isValid(path, info.isDir(), p) ) { 289​ if ( !q->isValid(path, info.isDir(), p) ) {
Show All 11 Lines292 if ( info.isDir() ) {
280 } 301​ }
281 if ( found ) { 302​ if ( found ) {
282 continue; 303​ continue;
283 } 304​ }
284 } else if (!p->filesForPath(indexedPath).isEmpty()) { 305​ } else if (!p->filesForPath(indexedPath).isEmpty()) {
285 // also gets triggered for kate's backup files 306​ // also gets triggered for kate's backup files
286 continue; 307​ continue;
287 } 308​ }
288 foreach ( ProjectFolderItem* parentItem, p->foldersForPath(indexedParent) ) { ​
mwolffUnsubmitted
Not Done

why was this removed?

mwolff: why was this removed?
rjvbbAuthorUnsubmitted
Not Done

As far as I understand it is no longer necessary because we no longer get the created signal. When watching a folder for change you get dirty signals that mean only that something in the directory has changed, and that's handled in the "force reload of" case above (eventuallyReadFolder() does a recursive read, right?).

Do you remember exactly what cases were handled by this code?

rjvbb: As far as I understand it is no longer necessary because we no longer get the `created` signal.
289 if ( info.isDir() ) { ​
mwolffUnsubmitted
Not Done

ok so this code was already dead before your patch, it seems (cf. loc 273-283 above) - so this can be removed separately from your patch to minimize the diff

mwolff: ok so this code was already dead before your patch, it seems (cf. loc 273-283 above) - so this…
290 ProjectFolderItem* folder = q->createFolderItem( p, path, parentItem ); ​
291 if (folder) { ​
292 emit q->folderAdded( folder ); ​
293 auto job = eventuallyReadFolder( folder ); ​
294 job->start(); ​
295 } ​
296 } else { ​
297 ProjectFileItem* file = q->createFileItem( p, path, parentItem ); ​
mwolffUnsubmitted
Not Done

this one needs to stay in master, it added new files to the folder

mwolff: this one needs to stay in master, it added new files to the folder
rjvbbAuthorUnsubmitted
Not Done

What does "needs to stay in master" mean? (I'm working on the 5.2 branch.) And why would it (this method) go adding new files to the folder?

New files that are created while the project is open are signalled as a "dirty" parent folder, which this method handles by queuing the folder for re-reading. That handles new files, as can easily be proven in real-world usage. What scenario am I missing according to you?

rjvbb: What does "needs to stay in master" mean? (I'm working on the 5.2 branch.) And why would it…
mwolffUnsubmitted
Not Done

This patch should target master, not 5.2. It's highly experimental.

Then see above, remove the dead code in master in a separate patch, but keep this part of the loop. Then rebase your patch remove the rest of this loop, too.

mwolff: This patch should target master, not 5.2. It's highly experimental. Then see above, remove the…
rjvbbAuthorUnsubmitted
Not Done

Please be more specific exactly what I have to remove and what loop you're referring to. There are 2 columns with line numbers I can choose from.

Are you saying this won't make it into 5.2? As a Mac user and thus concerned directly by the current inefficiency that would cool off my enthusiasm to invest in this significantly.

rjvbb: Please be more specific exactly what I have to remove and what loop you're referring to. There…
mwolffUnsubmitted
Not Done

actually, ignore the part about a separate patch, now that I re-read the old code I finally understand what it's doing - it adds a new folder or file item when none exists. So this is an atomic change after all, keep it as-is.

And yes, in my opinion this cannot target 5.2, considering @kfunk and @brauch want to release 5.2 soon. Landing such a big change shortly before release is never a good idea.

I honestly cannot understand your reasoning though. If you want to help out with KDevelop development, you have to target master. It's as simple as that.

mwolff: actually, ignore the part about a separate patch, now that I re-read the old code I finally…
rjvbbAuthorUnsubmitted
Not Done

(Also see my other non-inline reaction)

I'm really doubtful that this is even a good idea. I think it handled the notification of creation of new entries *in the parent directory*.

We no longer get created signals but only signals that tell us that something has changed in a given directory - a folder that is already represented in the project view.

I don't know if/why/how there can be multiple FolderItems corresponding to the dirty path's parent, nor why you'd want to emit the folderAdded signal (it wasn't just added, it was already known), and then read the folder again. Maybe we need to pretend that it was just added so that clients get the signal that they too need to reload the folder?
This is assuming that p->foldersForPath(indexedPath) == q->createFolderItem( p, path, parentItem ).

The 2nd hunk containing ProjectFileItem* file = q->createFileItem( p, path, parentItem ); ought never trigger because we only get called with folder paths (info.isDir() is always true).

rjvbb: (Also see my other non-inline reaction) I'm really doubtful that this is even a good idea. I…
mwolffUnsubmitted
Done

sigh, lost in translation once more... With "keep it as-is" I meant keep your version that removed this altogether. As I said, I misunderstood this code initially - you are right in that this loop should be removed now that you listen to dirty instead of created.

mwolff: sigh, lost in translation once more... With "keep it as-is" I meant keep your version that…
rjvbbAuthorUnsubmitted
Not Done

I think it just sounded different (clearer) in your head.
(we could try you speaking German and me Dutch but I doubt that'll improve anything) ;)

rjvbb: I think it just sounded different (clearer) in your head. (we could try you speaking German and…
298 if (file) { ​
299 emit q->fileAdded( file ); ​
300 } ​
301 } ​
302 } ​
303 } 309​ }
304} 310​}
305 311​
306void AbstractFileManagerPluginPrivate::deleted(const QString& path_) 312​void AbstractFileManagerPluginPrivate::deleted(const QString& path_)
307{ 313​{
308 if ( QFile::exists(path_) ) { 314​ if ( QFile::exists(path_) ) {
309 // stopDirScan... 315​ // stopDirScan...
310 return; 316​ return;
311 } 317​ }
312 // ensure that the path is not inside a stopped folder 318​ // ensure that the path is not inside a stopped folder
313 foreach(const QString& folder, m_stoppedFolders) { 319​ foreach(const QString& folder, m_stoppedFolders) {
314 if (path_.startsWith(folder)) { 320​ if (path_.startsWith(folder)) {
315 return; 321​ return;
316 } 322​ }
317 } 323​ }
318 qCDebug(FILEMANAGER) << "deleted:" << path_; 324​ qCDebug(FILEMANAGER) << "deleted:" << path_;
319 325​
320 const Path path(QUrl::fromLocalFile(path_)); 326​ const Path path(QUrl::fromLocalFile(path_));
321 const IndexedString indexed(path.pathOrUrl()); 327​ const IndexedString indexed(path.pathOrUrl());
322 328​
323 QHashIterator<IProject*, KDirWatch*> it(m_watchers); 329​ QHashIterator<IProject*, ProjectWatcher*> it(m_watchers);
324 while (it.hasNext()) { 330​ while (it.hasNext()) {
325 const auto p = it.next().key(); 331​ const auto p = it.next().key();
326 if (path == p->path()) { 332​ if (path == p->path()) {
327 KMessageBox::error(qApp->activeWindow(), 333​ KMessageBox::error(qApp->activeWindow(),
328 i18n("The base folder of project <b>%1</b>" 334​ i18n("The base folder of project <b>%1</b>"
329 " got deleted or moved outside of KDevelop.\n" 335​ " got deleted or moved outside of KDevelop.\n"
330 "The project has to be closed.", p->name()), 336​ "The project has to be closed.", p->name()),
331 i18n("Project Folder Deleted") ); 337​ i18n("Project Folder Deleted") );
▲ Show 20 Lines • Show All 51 Lines • ▼ Show 20 Line(s)358{
383 return false; 389​ return false;
384} 390​}
385 391​
386void AbstractFileManagerPluginPrivate::stopWatcher(ProjectFolderItem* folder) 392​void AbstractFileManagerPluginPrivate::stopWatcher(ProjectFolderItem* folder)
387{ 393​{
388 if ( !folder->path().isLocalFile() ) { 394​ if ( !folder->path().isLocalFile() ) {
389 return; 395​ return;
390 } 396​ }
391 Q_ASSERT(m_watchers.contains(folder->project())); 397​ Q_ASSERT(m_watchers.contains(folder->project()));
mwolffUnsubmitted
Not Done

undo

mwolff: undo
392 const QString path = folder->path().toLocalFile(); 398​ const QString path = folder->path().toLocalFile();
393 m_watchers[folder->project()]->stopDirScan(path); 399​ m_watchers[folder->project()]->stopDirScan(path);
mwolffUnsubmitted
Done

undo the change hunk completely

mwolff: undo the change hunk completely
mwolffUnsubmitted
Done

move up, like before (just use git checkout -p HEAD^ and undo this hunk)

mwolff: move up, like before (just use `git checkout -p HEAD^` and undo this hunk)
394 m_stoppedFolders.append(path); 400​ m_stoppedFolders.append(path);
395} 401​}
396 402​
397void AbstractFileManagerPluginPrivate::continueWatcher(ProjectFolderItem* folder) 403​void AbstractFileManagerPluginPrivate::continueWatcher(ProjectFolderItem* folder)
398{ 404​{
399 if ( !folder->path().isLocalFile() ) { 405​ if ( !folder->path().isLocalFile() ) {
400 return; 406​ return;
401 } 407​ }
402 auto watcher = m_watchers.value(folder->project(), nullptr); 408​ auto watcher = m_watchers.value(folder->project(), nullptr);
403 Q_ASSERT(watcher); 409​ Q_ASSERT(watcher);
404 const QString path = folder->path().toLocalFile(); 410​ const QString path = folder->path().toLocalFile();
405 if (!watcher->restartDirScan(path)) { 411​ if (!watcher->restartDirScan(path)) {
406 // path wasn't being watched yet - can we be 100% certain of that will never happen? 412​ // path wasn't being watched yet - can we be 100% certain of that will never happen?
mwolffUnsubmitted
Not Done

it worked before, is this really required now? if not, you can make that a separate patch if you think it solves an issue. to me it looks like a blind change that has nothing to do with the actual thing you are trying to fix here

mwolff: it worked before, is this really required now? if not, you can make that a separate patch if…
rjvbbAuthorUnsubmitted
Not Done

As stated in the comment, I don't know if we (read I) can be certain the directory to restart will always be monitored already.

I suspect there is a slightly bigger chance with the proposed dirwatching model that we end here for a new directory that somehow hasn't been processed by the dirty handler yet. I'll put a qWarning and run that locally for a bit to see if it ever triggers.

I made this change because it seemed like a simple and cheap failsafe, using a KDW feature that was undoubtedly implemented for a reason.

rjvbb: As stated in the comment, I don't know if we (read I) can be certain the directory to restart…
rjvbbAuthorUnsubmitted
Not Done

Doh, I had already added that warning message.

rjvbb: Doh, I had already added that warning message.
mwolffUnsubmitted
Not Done

Well, then do it in a separate patch - I'm not going to oppose it. But it's not related to the bigger change set here at all.

mwolff: Well, then do it in a separate patch - I'm not going to oppose it. But it's not related to the…
rjvbbAuthorUnsubmitted
Not Done

direct commit or review first? I'd prefer the former, evidently.

rjvbb: direct commit or review first? I'd prefer the former, evidently.
mwolffUnsubmitted
Not Done

yes, direct commit is fine, but now that you access the watcher multiple times, ensure you cleanup the code:

auto watcher = m_watchers.value(folder->project());
Q_ASSERT(watcher);
if (!watcher->restartDirScan(path)) {
    // warning + comment
    watcher->addDir(path);
}
mwolff: yes, direct commit is fine, but now that you access the watcher multiple times, ensure you…
rjvbbAuthorUnsubmitted
Not Done

So, what branch (this shouldn't break anything in 5.2) ?

rjvbb: So, what branch (this shouldn't break anything in 5.2) ?
mwolffUnsubmitted
Done

5.2 is fine with me

mwolff: 5.2 is fine with me
407 qCWarning(FILEMANAGER) << "Folder" << path << "in project" << folder->project()->name() << "wasn't yet being watched"; 413​ qCWarning(FILEMANAGER) << "Folder" << path << "in project" << folder->project()->name() << "wasn't yet being watched";
408 watcher->addDir(path); 414​ watcher->addDir(path);
409 } 415​ }
mwolffUnsubmitted
Done

undo, remove new code - always watch all dirs

mwolff: undo, remove new code - always watch all dirs
410 const int idx = m_stoppedFolders.indexOf(path); 416​ const int idx = m_stoppedFolders.indexOf(path);
411 if (idx != -1) { 417​ if (idx != -1) {
412 m_stoppedFolders.remove(idx); 418​ m_stoppedFolders.remove(idx);
413 } 419​ }
414} 420​}
415 421​
416bool isChildItem(ProjectBaseItem* parent, ProjectBaseItem* child) 422​bool isChildItem(ProjectBaseItem* parent, ProjectBaseItem* child)
417{ 423​{
Show All 13 Lines436 foreach(FileManagerListJob* job, m_projectJobs[folder->project()]) {
431 if (isChildItem(folder, job->item())) { 437​ if (isChildItem(folder, job->item())) {
432 qCDebug(FILEMANAGER) << "killing list job for removed folder" << job << folder->path(); 438​ qCDebug(FILEMANAGER) << "killing list job for removed folder" << job << folder->path();
433 job->abort(); 439​ job->abort();
434 Q_ASSERT(!m_projectJobs.value(folder->project()).contains(job)); 440​ Q_ASSERT(!m_projectJobs.value(folder->project()).contains(job));
435 } else { 441​ } else {
436 job->removeSubDir(folder); 442​ job->removeSubDir(folder);
437 } 443​ }
438 } 444​ }
445​ ProjectWatcher* watcher = m_watchers.value(folder->project(), nullptr);
mwolffUnsubmitted
Done

auto, and assert the watcher, it must exist for a directory

mwolff: auto, and assert the watcher, it must exist for a directory
mwolffUnsubmitted
Done

only when this is a local path

mwolff: only when this is a local path
446​ Q_ASSERT(watcher);
447​ watcher->removeDir(folder->path().toLocalFile());
439 folder->parent()->removeRow( folder->row() ); 448​ folder->parent()->removeRow( folder->row() );
440} 449​}
441 450​
442//END Private 451​//END Private
443 452​
444//BEGIN Plugin 453​//BEGIN Plugin
445 454​
446AbstractFileManagerPlugin::AbstractFileManagerPlugin( const QString& componentName, 455​AbstractFileManagerPlugin::AbstractFileManagerPlugin( const QString& componentName,
Show All 25 Lines
472ProjectFolderItem *AbstractFileManagerPlugin::import( IProject *project ) 481​ProjectFolderItem *AbstractFileManagerPlugin::import( IProject *project )
473{ 482​{
474 ProjectFolderItem *projectRoot = createFolderItem( project, project->path(), nullptr ); 483​ ProjectFolderItem *projectRoot = createFolderItem( project, project->path(), nullptr );
475 emit folderAdded( projectRoot ); 484​ emit folderAdded( projectRoot );
476 qCDebug(FILEMANAGER) << "imported new project" << project->name() << "at" << projectRoot->path(); 485​ qCDebug(FILEMANAGER) << "imported new project" << project->name() << "at" << projectRoot->path();
477 486​
478 ///TODO: check if this works for remote files when something gets changed through another KDE app 487​ ///TODO: check if this works for remote files when something gets changed through another KDE app
479 if ( project->path().isLocalFile() ) { 488​ if ( project->path().isLocalFile() ) {
480 d->m_watchers[project] = new KDirWatch( project ); 489​ auto watcher = new ProjectWatcher(project, &d->m_filters);
mwolffUnsubmitted
Done

while at it, can you please clean this up to not do multiple lookups:

auto watcher = new ProjectWatcher(project);
connect(watcher ,...);
connect(watcher ,...);
d->m_watchers[project] = watcher;
mwolff: while at it, can you please clean this up to not do multiple lookups: auto watcher = new…
481 490​
482 connect(d->m_watchers[project], &KDirWatch::created, 491​ // set up the signal handling; feeding the dirwatcher is handled by FileManagerListJob.
mwolffUnsubmitted
Done

why don't we listen to created anymore?

mwolff: why don't we listen to created anymore?
rjvbbAuthorUnsubmitted
Not Done

Because when watching directories you only get that signal when a previously watched, deleted directory is recreated. We'll always get a dirty signal for such an event too (I checked).

rjvbb: Because when watching directories you only get that signal when a previously watched, deleted…
mwolffUnsubmitted
Done

specify _where_ it is handled

mwolff: specify _where_ it is handled
483 this, [&] (const QString& path_) { d->created(path_); }); 492​ connect(watcher, &KDirWatch::dirty,
484 connect(d->m_watchers[project], &KDirWatch::deleted, 493​ this, [&] (const QString& path_) { d->dirty(path_); });
494​ connect(watcher, &KDirWatch::deleted,
485 this, [&] (const QString& path_) { d->deleted(path_); }); 495​ this, [&] (const QString& path_) { d->deleted(path_); });
486 496​ d->m_watchers[project] = watcher;
487 d->m_watchers[project]->addDir(project->path().toLocalFile(), KDirWatch::WatchSubDirs | KDirWatch:: WatchFiles ); ​
488 } 497​ }
489 498​
490 d->m_filters.add(project); 499​ d->m_filters.add(project);
491 500​
492 return projectRoot; 501​ return projectRoot;
493} 502​}
494 503​
495KJob* AbstractFileManagerPlugin::createImportJob(ProjectFolderItem* item) 504​KJob* AbstractFileManagerPlugin::createImportJob(ProjectFolderItem* item)
▲ Show 20 Lines • Show All 160 Lines • ▼ Show 20 Line(s)664{
656 return new ProjectFolderItem( project, path, parent ); 665​ return new ProjectFolderItem( project, path, parent );
657} 666​}
658 667​
659KDirWatch* AbstractFileManagerPlugin::projectWatcher( IProject* project ) const 668​KDirWatch* AbstractFileManagerPlugin::projectWatcher( IProject* project ) const
660{ 669​{
661 return d->m_watchers.value( project, nullptr ); 670​ return d->m_watchers.value( project, nullptr );
662} 671​}
663 672​
673​int AbstractFileManagerPlugin::watchedItems( IProject* project ) const
674​{
675​ auto watcher = d->m_watchers.value( project, nullptr );
676​ Q_ASSERT(watcher);
677​ return watcher->size();
678​}
679​
680​void AbstractFileManagerPlugin::projectClosing( IProject *project )
681​{
682​ d->projectClosing(project);
683​}
684​
685​
664//END Plugin 686​//END Plugin
665 687​
666#include "moc_abstractfilemanagerplugin.cpp" 688​#include "moc_abstractfilemanagerplugin.cpp"