Changeset View
Standalone View
kdevplatform/project/abstractfilemanagerplugin.h
Show All 28 Lines | |||||
29 | 29 | | |||
30 | #include <interfaces/iplugin.h> | 30 | #include <interfaces/iplugin.h> | ||
31 | 31 | | |||
32 | class KDirWatch; | 32 | class KDirWatch; | ||
33 | 33 | | |||
34 | namespace KDevelop { | 34 | namespace KDevelop { | ||
35 | 35 | | |||
36 | class AbstractFileManagerPluginPrivate; | 36 | class AbstractFileManagerPluginPrivate; | ||
37 | class ProjectController; | ||||
37 | 38 | | |||
38 | /** | 39 | /** | ||
39 | * This class can be used as a common base for file managers. | 40 | * This class can be used as a common base for file managers. | ||
40 | * | 41 | * | ||
41 | * It supports remote files using KIO and uses KDirWatch to synchronize with on-disk changes. | 42 | * It supports remote files using KIO and uses KDirWatch to synchronize with on-disk changes. | ||
42 | */ | 43 | */ | ||
43 | class KDEVPLATFORMPROJECT_EXPORT AbstractFileManagerPlugin : public IPlugin, public virtual IProjectFileManager | 44 | class KDEVPLATFORMPROJECT_EXPORT AbstractFileManagerPlugin : public IPlugin, public virtual IProjectFileManager | ||
44 | { | 45 | { | ||
▲ Show 20 Lines • Show All 51 Lines • ▼ Show 20 Line(s) | 75 | // | |||
96 | * The default implementation will return a simple @c ProjectFileItem | 97 | * The default implementation will return a simple @c ProjectFileItem | ||
97 | */ | 98 | */ | ||
98 | virtual ProjectFileItem* createFileItem( IProject* project, const Path& path, | 99 | virtual ProjectFileItem* createFileItem( IProject* project, const Path& path, | ||
99 | ProjectBaseItem* parent); | 100 | ProjectBaseItem* parent); | ||
100 | 101 | | |||
101 | /** | 102 | /** | ||
102 | * @return the @c KDirWatch for the given @p project. | 103 | * @return the @c KDirWatch for the given @p project. | ||
103 | */ | 104 | */ | ||
104 | KDirWatch* projectWatcher( IProject* project ) const; | 105 | KDirWatch* projectWatcher( IProject* project ) const; | ||
mwolff: actually, isn't this now unsafe API? If the dir watcher needs to be protected by a mutex, we… | |||||
I don't think it's any more unsafe than it already was; I don't see anything that suggests that using a KDirWatch once from another thread makes it unsafe to use from its main thread afterwards. Or rather, concurrently from multiple threads because that's what the mutex protects against. I've tried my benchmark app without the mutex locally, and managed to reproduce the double-free crash quite regularly (on Linux). I think it would have been observed in the wild by now if the result from projectWatcher was used concurrently. rjvbb: I don't think it's any more unsafe than it already was; I don't see anything that suggests that… | |||||
The dir watcher was not accessed concurrently before this patch. As such, you are now responsible to ensure all the old code that wasn't written with concurrency in mind plays along. mwolff: The dir watcher **was not accessed concurrently** before this patch. As such, you are now… | |||||
You are concerned that the existing code that uses the getter might end up using the dirwatcher at the same time the filemanagerlistjob uses it? In that case we'll need to export the ProjectWatcher class, and upgrade all code where the getter is used. This would be much less of a concern if KDirWatch::addDir and KDirWatch::removeDir were virtual, right? That might be an easier change to get through upstream. rjvbb: You are concerned that the existing code that uses the getter might end up using the dirwatcher… | |||||
rjvbb: D8043 | |||||
105 | 106 | | |||
107 | /** | ||||
108 | * @return the number of watched directories for the given @p project. | ||||
109 | */ | ||||
110 | int watchedItems( IProject* project ) const; | ||||
mwolff: this should be moved into KDirWatch, then we can use the getter above | |||||
I agree it's lacking from KDirWatch, but can't we develop the change here first and then see what needs to be upstreamed? rjvbb: I agree it's lacking from KDirWatch, but can't we develop the change here first and then see… | |||||
111 | | ||||
112 | /** | ||||
113 | * tell the plugin that the given @p project is going to be closed. | ||||
mwolff: so it's like closing the project, no? why do | |||||
Why do what? It's part of what happens when closing a project in KDevelop, but it isn't actually closing the project. It's telling the file manager to forget about the project. There is no current use case for the method in KDevelop itself, it's only for the benchmark. I tried to get the signal sent through the ProjectController (projectController()->closeProject(m_project)) but that seems to need more setting up. I don't have time to figure out what and if that is going to add noise to the benchmark (parser overhead, for instance). The detach method could be useful in a mode where you want to keep a project open for quick reference (or snatching some code snippets) only and don't want to waste resources on parsing etc. rjvbb: Why do what?
It's part of what happens when closing a project in KDevelop, but it isn't… | |||||
You can, and should, disable the background parser in the benchmark. I don't see why a file manager should be able to "forget about the project". That should only happen when the project is closed, so do close the project. Also, please for the love of $deity stop saying "you don't have time". I also have other things to do than to review your code. If you don't have time we can and should stop this whole process here and there. Either you attend to the code review, or you don't. If the latter, don't contribute to KDevelop. mwolff: You can, and should, disable the background parser in the benchmark. I don't see why a file… | |||||
If someone wants to take this over, by all means, feel free. I've renamed the method to projectClosing. If you don't want to export the method at all we'll just do without benchmarking the dirwatcher dtor. rjvbb: If someone wants to take this over, by all means, feel free.
I've renamed the method to… | |||||
114 | */ | ||||
115 | void projectClosing( IProject* project ); | ||||
116 | | ||||
106 | Q_SIGNALS: | 117 | Q_SIGNALS: | ||
107 | void reloadedFileItem(KDevelop::ProjectFileItem* file); | 118 | void reloadedFileItem(KDevelop::ProjectFileItem* file); | ||
108 | void reloadedFolderItem(KDevelop::ProjectFolderItem* folder); | 119 | void reloadedFolderItem(KDevelop::ProjectFolderItem* folder); | ||
109 | 120 | | |||
110 | void folderAdded(KDevelop::ProjectFolderItem* folder); | 121 | void folderAdded(KDevelop::ProjectFolderItem* folder); | ||
111 | void folderRemoved(KDevelop::ProjectFolderItem* folder); | 122 | void folderRemoved(KDevelop::ProjectFolderItem* folder); | ||
112 | void folderRenamed(const KDevelop::Path& oldFolder, KDevelop::ProjectFolderItem* newFolder); | 123 | void folderRenamed(const KDevelop::Path& oldFolder, KDevelop::ProjectFolderItem* newFolder); | ||
113 | 124 | | |||
114 | void fileAdded(KDevelop::ProjectFileItem* file); | 125 | void fileAdded(KDevelop::ProjectFileItem* file); | ||
115 | void fileRemoved(KDevelop::ProjectFileItem* file); | 126 | void fileRemoved(KDevelop::ProjectFileItem* file); | ||
116 | void fileRenamed(const KDevelop::Path& oldFile, KDevelop::ProjectFileItem* newFile); | 127 | void fileRenamed(const KDevelop::Path& oldFile, KDevelop::ProjectFileItem* newFile); | ||
117 | 128 | | |||
118 | private: | 129 | private: | ||
119 | const QScopedPointer<class AbstractFileManagerPluginPrivate> d; | 130 | const QScopedPointer<class AbstractFileManagerPluginPrivate> d; | ||
120 | friend class AbstractFileManagerPluginPrivate; | 131 | friend class AbstractFileManagerPluginPrivate; | ||
132 | public: | ||||
133 | friend class ProjectController; | ||||
121 | }; | 134 | }; | ||
122 | 135 | | |||
123 | } | 136 | } | ||
124 | 137 | | |||
125 | #endif // KDEVPLATFORM_ABSTRACTFILEMANAGERPLUGIN_H | 138 | #endif // KDEVPLATFORM_ABSTRACTFILEMANAGERPLUGIN_H |
actually, isn't this now unsafe API? If the dir watcher needs to be protected by a mutex, we must not hand it out as a raw pointer, no?