Differential D7995 Diff 22858 kdevplatform/project/abstractfilemanagerplugin.cpp

Changeset View

Standalone View

View Options

kdevplatform/project/abstractfilemanagerplugin.cpp

1/*************************************************************************** 1​/***************************************************************************
mwolffUnsubmitted
Done

remove

mwolff: remove
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. *
Show All 27 Lines
36#include <KDirWatch> 36​#include <KDirWatch>
37 37​
38#include <interfaces/iproject.h> 38​#include <interfaces/iproject.h>
39#include <interfaces/icore.h> 39​#include <interfaces/icore.h>
40#include <interfaces/iprojectcontroller.h> 40​#include <interfaces/iprojectcontroller.h>
41#include <serialization/indexedstring.h> 41​#include <serialization/indexedstring.h>
42 42​
43#include "projectfiltermanager.h" 43​#include "projectfiltermanager.h"
44​#include "projectwatcher.h"
44#include "debug.h" 45​#include "debug.h"
45 46​
46#define ifDebug(x) 47​#define ifDebug(x)
47 48​
48using namespace KDevelop; 49​using namespace KDevelop;
49 50​
50//BEGIN Helper 51​//BEGIN Helper
51 52​
Show All 32 Lines75public:
84 * 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.
85 */ 86​ */
86 Q_REQUIRED_RESULT KIO::Job* eventuallyReadFolder(ProjectFolderItem* item); 87​ Q_REQUIRED_RESULT KIO::Job* eventuallyReadFolder(ProjectFolderItem* item);
87 void addJobItems(FileManagerListJob* job, 88​ void addJobItems(FileManagerListJob* job,
88 ProjectFolderItem* baseItem, 89​ ProjectFolderItem* baseItem,
89 const KIO::UDSEntryList& entries); 90​ const KIO::UDSEntryList& entries);
90 91​
91 void deleted(const QString &path); 92​ void deleted(const QString &path);
92 void created(const QString &path); 93​ void dirty(const QString &path);
93 94​
94 void projectClosing(IProject* project); 95​ void projectClosing(IProject* project);
95 void jobFinished(KJob* job); 96​ void jobFinished(KJob* job);
96 97​
97 /// 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.
98 void stopWatcher(ProjectFolderItem* folder); 99​ void stopWatcher(ProjectFolderItem* folder);
99 /// Continues watching the given folder for changes. 100​ /// Continues watching the given folder for changes.
100 void continueWatcher(ProjectFolderItem* folder); 101​ void continueWatcher(ProjectFolderItem* folder);
101 /// Common renaming function. 102​ /// Common renaming function.
102 bool rename(ProjectBaseItem* item, const Path& newPath); 103​ bool rename(ProjectBaseItem* item, const Path& newPath);
103 104​
104 void removeFolder(ProjectFolderItem* folder); 105​ void removeFolder(ProjectFolderItem* folder);
105 106​
106 QHash<IProject*, KDirWatch*> m_watchers; 107​ QHash<IProject*, ProjectWatcher*> m_watchers;
107 QHash<IProject*, QList<FileManagerListJob*> > m_projectJobs; 108​ QHash<IProject*, QList<FileManagerListJob*> > m_projectJobs;
108 QVector<QString> m_stoppedFolders; 109​ QVector<QString> m_stoppedFolders;
109 ProjectFilterManager m_filters; 110​ ProjectFilterManager m_filters;
110}; 111​};
111 112​
112void AbstractFileManagerPluginPrivate::projectClosing(IProject* project) 113​void AbstractFileManagerPluginPrivate::projectClosing(IProject* project)
113{ 114​{
114 if ( m_projectJobs.contains(project) ) { 115​ if ( m_projectJobs.contains(project) ) {
115 // 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
116 // see also addLotsOfFiles test 117​ // see also addLotsOfFiles test
117 foreach( FileManagerListJob* job, m_projectJobs[project] ) { 118​ foreach( FileManagerListJob* job, m_projectJobs[project] ) {
118 qCDebug(FILEMANAGER) << "killing project job:" << job; 119​ qCDebug(FILEMANAGER) << "killing project job:" << job;
119 job->abort(); 120​ job->abort();
120 } 121​ }
121 m_projectJobs.remove(project); 122​ m_projectJobs.remove(project);
122 } 123​ }
123#ifdef TIME_IMPORT_JOB 124​#ifdef TIME_IMPORT_JOB
124 QElapsedTimer timer; 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…
125 if (m_watchers.contains(project)) { 126​ if (m_watchers.contains(project)) {
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 ;)
126 timer.start(); 127​ timer.start();
127 } 128​ }
128#endif 129​#endif
129 delete m_watchers.take(project); 130​ delete m_watchers.take(project);
130#ifdef TIME_IMPORT_JOB 131​#ifdef TIME_IMPORT_JOB
131 if (timer.isValid()) { 132​ if (timer.isValid()) {
132 qCDebug(FILEMANAGER) << "Deleting dir watcher took" << timer.elapsed() / 1000.0 << "seconds for project" << project->name(); 133​ qCDebug(FILEMANAGER) << "Deleting dir watcher took" << timer.elapsed() / 1000.0 << "seconds for project" << project->name();
133 } 134​ }
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.
134#endif 135​#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…
135 m_filters.remove(project); 136​ 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…
136} 137​}
137 138​
138KIO::Job* AbstractFileManagerPluginPrivate::eventuallyReadFolder(ProjectFolderItem* item) 139​KIO::Job* AbstractFileManagerPluginPrivate::eventuallyReadFolder(ProjectFolderItem* item)
139{ 140​{
141​ ProjectWatcher* watcher = m_watchers.value( item->project(), nullptr );
140 FileManagerListJob* listJob = new FileManagerListJob( item ); 142​ FileManagerListJob* listJob = new FileManagerListJob( item );
141 m_projectJobs[ item->project() ] << listJob; 143​ m_projectJobs[ item->project() ] << listJob;
mwolffUnsubmitted
Done

remove, always watch dirs

mwolff: remove, always watch dirs
142 qCDebug(FILEMANAGER) << "adding job" << listJob << item << item->path() << "for project" << item->project(); 144​ qCDebug(FILEMANAGER) << "adding job" << listJob << item << item->path() << "for project" << item->project();
143 145​
144 q->connect( listJob, &FileManagerListJob::finished, 146​ q->connect( listJob, &FileManagerListJob::finished,
145 q, [&] (KJob* job) { jobFinished(job); } ); 147​ q, [&] (KJob* job) { jobFinished(job); } );
146 148​
147 q->connect( listJob, &FileManagerListJob::entries, 149​ q->connect( listJob, &FileManagerListJob::entries,
148 q, [&] (FileManagerListJob* job, ProjectFolderItem* baseItem, const KIO::UDSEntryList& entries) { 150​ q, [&] (FileManagerListJob* job, ProjectFolderItem* baseItem, const KIO::UDSEntryList& entries) {
149 addJobItems(job, baseItem, entries); } ); 151​ addJobItems(job, baseItem, entries); } );
152​ q->connect( listJob, &FileManagerListJob::watchDir,
153​ watcher, [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…
154​ watcher->addDir(path); } );
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…
150 155​
151 return listJob; 156​ return listJob;
152} 157​}
153 158​
154void AbstractFileManagerPluginPrivate::jobFinished(KJob* job) 159​void AbstractFileManagerPluginPrivate::jobFinished(KJob* job)
155{ 160​{
156 FileManagerListJob* gmlJob = qobject_cast<FileManagerListJob*>(job); 161​ FileManagerListJob* gmlJob = qobject_cast<FileManagerListJob*>(job);
157 if (gmlJob) { 162​ if (gmlJob) {
▲ Show 20 Lines • Show All 94 Lines • ▼ Show 20 Line(s)256 foreach ( const Path& path, folders ) {
252 ProjectFolderItem* folder = q->createFolderItem( baseItem->project(), path, baseItem ); 257​ ProjectFolderItem* folder = q->createFolderItem( baseItem->project(), path, baseItem );
253 if (folder) { 258​ if (folder) {
254 emit q->folderAdded( folder ); 259​ emit q->folderAdded( folder );
255 job->addSubDir( folder ); 260​ job->addSubDir( folder );
256 } 261​ }
257 } 262​ }
258} 263​}
259 264​
260void AbstractFileManagerPluginPrivate::created(const QString& path_) 265​void AbstractFileManagerPluginPrivate::dirty(const QString& path_)
261{ 266​{
262 qCDebug(FILEMANAGER) << "created:" << path_; 267​ qCDebug(FILEMANAGER) << "dirty:" << path_;
263 QFileInfo info(path_); 268​ QFileInfo info(path_);
264 269​
265 ///FIXME: share memory with parent 270​ ///FIXME: share memory with parent
266 const Path path(path_); 271​ const Path path(path_);
267 const IndexedString indexedPath(path.pathOrUrl()); 272​ const IndexedString indexedPath(path.pathOrUrl());
268 const IndexedString indexedParent(path.parent().pathOrUrl()); 273​ const IndexedString indexedParent(path.parent().pathOrUrl());
269 274​
270 QHashIterator<IProject*, KDirWatch*> it(m_watchers); 275​ QHashIterator<IProject*, ProjectWatcher*> it(m_watchers);
271 while (it.hasNext()) { 276​ while (it.hasNext()) {
272 const auto p = it.next().key(); 277​ const auto p = it.next().key();
273 if ( !p->projectItem()->model() ) { 278​ if ( !p->projectItem()->model() ) {
274 // not yet finished with loading 279​ // not yet finished with loading
275 // FIXME: how should this be handled? see unit test 280​ // FIXME: how should this be handled? see unit test
276 continue; 281​ continue;
277 } 282​ }
278 if ( !q->isValid(path, info.isDir(), p) ) { 283​ if ( !q->isValid(path, info.isDir(), p) ) {
Show All 11 Lines286 if ( info.isDir() ) {
290 } 295​ }
291 if ( found ) { 296​ if ( found ) {
292 continue; 297​ continue;
293 } 298​ }
294 } else if (!p->filesForPath(indexedPath).isEmpty()) { 299​ } else if (!p->filesForPath(indexedPath).isEmpty()) {
295 // also gets triggered for kate's backup files 300​ // also gets triggered for kate's backup files
296 continue; 301​ continue;
297 } 302​ }
298 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.
299 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…
300 ProjectFolderItem* folder = q->createFolderItem( p, path, parentItem ); ​
301 if (folder) { ​
302 emit q->folderAdded( folder ); ​
303 auto job = eventuallyReadFolder( folder ); ​
304 job->start(); ​
305 } ​
306 } else { ​
307 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…
308 if (file) { ​
309 emit q->fileAdded( file ); ​
310 } ​
311 } ​
312 } ​
313 } 303​ }
314} 304​}
315 305​
316void AbstractFileManagerPluginPrivate::deleted(const QString& path_) 306​void AbstractFileManagerPluginPrivate::deleted(const QString& path_)
317{ 307​{
318 if ( QFile::exists(path_) ) { 308​ if ( QFile::exists(path_) ) {
319 // stopDirScan... 309​ // stopDirScan...
320 return; 310​ return;
321 } 311​ }
322 // ensure that the path is not inside a stopped folder 312​ // ensure that the path is not inside a stopped folder
323 foreach(const QString& folder, m_stoppedFolders) { 313​ foreach(const QString& folder, m_stoppedFolders) {
324 if (path_.startsWith(folder)) { 314​ if (path_.startsWith(folder)) {
325 return; 315​ return;
326 } 316​ }
327 } 317​ }
328 qCDebug(FILEMANAGER) << "deleted:" << path_; 318​ qCDebug(FILEMANAGER) << "deleted:" << path_;
329 319​
330 const Path path(QUrl::fromLocalFile(path_)); 320​ const Path path(QUrl::fromLocalFile(path_));
331 const IndexedString indexed(path.pathOrUrl()); 321​ const IndexedString indexed(path.pathOrUrl());
332 322​
333 QHashIterator<IProject*, KDirWatch*> it(m_watchers); 323​ QHashIterator<IProject*, ProjectWatcher*> it(m_watchers);
334 while (it.hasNext()) { 324​ while (it.hasNext()) {
335 const auto p = it.next().key(); 325​ const auto p = it.next().key();
336 if (path == p->path()) { 326​ if (path == p->path()) {
337 KMessageBox::error(qApp->activeWindow(), 327​ KMessageBox::error(qApp->activeWindow(),
338 i18n("The base folder of project <b>%1</b>" 328​ i18n("The base folder of project <b>%1</b>"
339 " got deleted or moved outside of KDevelop.\n" 329​ " got deleted or moved outside of KDevelop.\n"
340 "The project has to be closed.", p->name()), 330​ "The project has to be closed.", p->name()),
341 i18n("Project Folder Deleted") ); 331​ i18n("Project Folder Deleted") );
▲ Show 20 Lines • Show All 51 Lines • ▼ Show 20 Line(s)352{
393 return false; 383​ return false;
394} 384​}
395 385​
396void AbstractFileManagerPluginPrivate::stopWatcher(ProjectFolderItem* folder) 386​void AbstractFileManagerPluginPrivate::stopWatcher(ProjectFolderItem* folder)
397{ 387​{
398 if ( !folder->path().isLocalFile() ) { 388​ if ( !folder->path().isLocalFile() ) {
399 return; 389​ return;
400 } 390​ }
401 Q_ASSERT(m_watchers.contains(folder->project())); 391​ Q_ASSERT(m_watchers.contains(folder->project()));
mwolffUnsubmitted
Not Done

undo

mwolff: undo
402 const QString path = folder->path().toLocalFile(); 392​ const QString path = folder->path().toLocalFile();
403 m_watchers[folder->project()]->stopDirScan(path); 393​ 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)
404 m_stoppedFolders.append(path); 394​ m_stoppedFolders.append(path);
405} 395​}
406 396​
407void AbstractFileManagerPluginPrivate::continueWatcher(ProjectFolderItem* folder) 397​void AbstractFileManagerPluginPrivate::continueWatcher(ProjectFolderItem* folder)
408{ 398​{
409 if ( !folder->path().isLocalFile() ) { 399​ if ( !folder->path().isLocalFile() ) {
410 return; 400​ return;
411 } 401​ }
412 auto watcher = m_watchers.value(folder->project(), nullptr); 402​ auto watcher = m_watchers.value(folder->project(), nullptr);
413 Q_ASSERT(watcher); 403​ Q_ASSERT(watcher);
414 const QString path = folder->path().toLocalFile(); 404​ const QString path = folder->path().toLocalFile();
415 if (!watcher->restartDirScan(path)) { 405​ if (!watcher->restartDirScan(path)) {
416 // path wasn't being watched yet - can we be 100% certain of that will never happen? 406​ // 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
417 qCWarning(FILEMANAGER) << "Folder" << path << "in project" << folder->project()->name() << "wasn't yet being watched"; 407​ qCWarning(FILEMANAGER) << "Folder" << path << "in project" << folder->project()->name() << "wasn't yet being watched";
418 watcher->addDir(path); 408​ watcher->addDir(path);
419 } 409​ }
mwolffUnsubmitted
Done

undo, remove new code - always watch all dirs

mwolff: undo, remove new code - always watch all dirs
420 const int idx = m_stoppedFolders.indexOf(path); 410​ const int idx = m_stoppedFolders.indexOf(path);
421 if (idx != -1) { 411​ if (idx != -1) {
422 m_stoppedFolders.remove(idx); 412​ m_stoppedFolders.remove(idx);
423 } 413​ }
424} 414​}
425 415​
426bool isChildItem(ProjectBaseItem* parent, ProjectBaseItem* child) 416​bool isChildItem(ProjectBaseItem* parent, ProjectBaseItem* child)
427{ 417​{
Show All 13 Lines430 foreach(FileManagerListJob* job, m_projectJobs[folder->project()]) {
441 if (isChildItem(folder, job->item())) { 431​ if (isChildItem(folder, job->item())) {
442 qCDebug(FILEMANAGER) << "killing list job for removed folder" << job << folder->path(); 432​ qCDebug(FILEMANAGER) << "killing list job for removed folder" << job << folder->path();
443 job->abort(); 433​ job->abort();
444 Q_ASSERT(!m_projectJobs.value(folder->project()).contains(job)); 434​ Q_ASSERT(!m_projectJobs.value(folder->project()).contains(job));
445 } else { 435​ } else {
446 job->removeSubDir(folder); 436​ job->removeSubDir(folder);
447 } 437​ }
448 } 438​ }
439​ if (folder->path().isLocalFile()) {
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
440​ ProjectWatcher* watcher = m_watchers.value(folder->project(), nullptr);
441​ Q_ASSERT(watcher);
442​ watcher->removeDir(folder->path().toLocalFile());
443​ }
449 folder->parent()->removeRow( folder->row() ); 444​ folder->parent()->removeRow( folder->row() );
450} 445​}
451 446​
452//END Private 447​//END Private
453 448​
454//BEGIN Plugin 449​//BEGIN Plugin
455 450​
456AbstractFileManagerPlugin::AbstractFileManagerPlugin( const QString& componentName, 451​AbstractFileManagerPlugin::AbstractFileManagerPlugin( const QString& componentName,
Show All 25 Lines
482ProjectFolderItem *AbstractFileManagerPlugin::import( IProject *project ) 477​ProjectFolderItem *AbstractFileManagerPlugin::import( IProject *project )
483{ 478​{
484 ProjectFolderItem *projectRoot = createFolderItem( project, project->path(), nullptr ); 479​ ProjectFolderItem *projectRoot = createFolderItem( project, project->path(), nullptr );
485 emit folderAdded( projectRoot ); 480​ emit folderAdded( projectRoot );
486 qCDebug(FILEMANAGER) << "imported new project" << project->name() << "at" << projectRoot->path(); 481​ qCDebug(FILEMANAGER) << "imported new project" << project->name() << "at" << projectRoot->path();
487 482​
488 ///TODO: check if this works for remote files when something gets changed through another KDE app 483​ ///TODO: check if this works for remote files when something gets changed through another KDE app
489 if ( project->path().isLocalFile() ) { 484​ if ( project->path().isLocalFile() ) {
490 auto watcher = new KDirWatch( project ); 485​ auto watcher = new ProjectWatcher(project);
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…
491 486​
492 // set up the signal handling 487​ // set up the signal handling; feeding the dirwatcher is handled by FileManagerListJob.
493 connect(watcher, &KDirWatch::created, 488​ connect(watcher, &KDirWatch::dirty,
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…
494 this, [&] (const QString& path_) { d->created(path_); }); 489​ this, [&] (const QString& path_) { d->dirty(path_); });
mwolffUnsubmitted
Done

specify _where_ it is handled

mwolff: specify _where_ it is handled
495 connect(watcher, &KDirWatch::deleted, 490​ connect(watcher, &KDirWatch::deleted,
496 this, [&] (const QString& path_) { d->deleted(path_); }); 491​ this, [&] (const QString& path_) { d->deleted(path_); });
497 watcher->addDir(project->path().toLocalFile(), KDirWatch::WatchSubDirs | KDirWatch:: WatchFiles ); ​
498 d->m_watchers[project] = watcher; 492​ d->m_watchers[project] = watcher;
499 } 493​ }
500 494​
501 d->m_filters.add(project); 495​ d->m_filters.add(project);
502 496​
503 return projectRoot; 497​ return projectRoot;
504} 498​}
505 499​
▲ Show 20 Lines • Show All 172 Lines • Show Last 20 Lines