Handle file additions/removals in ContextDirectoryManager.

pkg/analysis_server/lib/src/context_directory_manager.dart

Index: pkg/analysis_server/lib/src/context_directory_manager.dart
diff --git a/pkg/analysis_server/lib/src/context_directory_manager.dart b/pkg/analysis_server/lib/src/context_directory_manager.dart
index ec728c445aadfa2f023ea431f22483f4ec870a95..e136a2b607aa5f5a677a01eee5a4bb0e3378eb32 100644
--- a/pkg/analysis_server/lib/src/context_directory_manager.dart
+++ b/pkg/analysis_server/lib/src/context_directory_manager.dart
@@ -4,9 +4,29 @@
 
 library context.directory.manager;
 
+import 'dart:async';
+
 import 'package:analysis_server/src/resource.dart';
 import 'package:analyzer/src/generated/engine.dart';
 import 'package:analyzer/src/generated/source.dart';
+import 'package:watcher/watcher.dart';
+
+/**
+ * Information tracked by the [ContextDirectoryManager] for each context.
+ */
+class _ContextDirectoryInfo {
+  /**
+   * Stream subscription we are using to watch the context's directory for
+   * changes.
+   */
+  StreamSubscription<WatchEvent> changeSubscription;
+
+  /**
+   * Map from full path to the [Source] object, for each source that has been
+   * added to the context.
+   */
+  Map<String, Source> sources = <String, Source>{};
+}
 
 /**
  * Class that maintains a mapping from included/excluded paths to a set of
@@ -14,10 +34,11 @@ import 'package:analyzer/src/generated/source.dart';
  */
 abstract class ContextDirectoryManager {
   /**
-   * The set of included folders in the most recent successful call to
-   * [setRoots].
+   * [_ContextDirectoryInfo] object for each included directory in the most
+   * recent successful call to [setRoots].
    */
-  Set<Folder> currentFolders = new Set<Folder>();
+  Map<Folder, _ContextDirectoryInfo> _currentDirectoryInfo =
+      <Folder, _ContextDirectoryInfo>{};

[danrubel, 2014/05/29 18:14:43]

Do you need the extra type declaration here since the map is empty?
I find this easier to read...

Map<Folder, _ContextDirectoryInfo> _currentDirectoryInfo = {};

[Paul Berry, 2014/05/29 18:30:23]

Heh, I was just talking about this yesterday with Konstantin, Jaime, and Brian
:)

Unfortunately, both type annotations are needed.  The one on the left prevents
later code from assigning a non-map (or a map of the wrong type) to
_currentDirectoryInfo.  The one on the right affects the runtime type of the
map, which means that in checked mode we get additional runtime type checks
whenever the map is modified (e.g. "_currentDirectoryInfo[a] = b" will check the
types of a and b).  Since the type system is unsound, the additional runtime
type checks can actually make a difference.

 
   /**
    * The [ResourceProvider] using which paths are converted into [Resource]s.
@@ -54,6 +75,7 @@ abstract class ContextDirectoryManager {
     }
     Set<Folder> excludedFolders = new Set<Folder>();
     // diff
+    Set<Folder> currentFolders = _currentDirectoryInfo.keys.toSet();
     Set<Folder> newFolders = includedFolders.difference(currentFolders);
     Set<Folder> oldFolders = currentFolders.difference(includedFolders);
     // remove old contexts
@@ -62,34 +84,81 @@ abstract class ContextDirectoryManager {
     }
     // add new contexts
     for (Folder folder in newFolders) {
+      _ContextDirectoryInfo info = new _ContextDirectoryInfo();
+      _currentDirectoryInfo[folder] = info;
+      info.changeSubscription = folder.changes.listen((WatchEvent event) {
+        _handleWatchEvent(folder, info, event);
+      });
       File pubspecFile = folder.getChild('pubspec.yaml');
       addContext(folder, pubspecFile.exists ? pubspecFile : null);
       ChangeSet changeSet = new ChangeSet();
-      _addSourceFiles(changeSet, folder);
+      _addSourceFiles(changeSet, folder, info);
       applyChangesToContext(folder, changeSet);
     }
-    currentFolders = new Set<Folder>.from(includedFolders);
+  }
+
+  void _handleWatchEvent(Folder folder, _ContextDirectoryInfo info, WatchEvent event) {
+    switch (event.type) {
+      case ChangeType.ADD:
+        // TODO(paulberry): handle adding pubspec.yaml
+        String shortName = resourceProvider.pathContext.basename(event.path);

[scheglov, 2014/05/29 17:37:03]

Should we hide using "path" package and just add such function into
ResourceProvider?

Actually there is Resource.shortName already.

[Paul Berry, 2014/05/29 17:57:21]

That's not a bad idea--it would decrease our risk of mistakenly using "path" and
leading to subtle bugs.
Yes but at this point in the code we haven't created the Resource yet.  And I
don't think we should create the Resource until we've decided that it should be
analyzed, because creating the Resource requires hitting the filesystem, and I
don't want to introduce unnecessary I/O on files that we aren't interested in
analyzing.

I guess this is a moot point considering your comment below that we don't need
to use basename().

+        if (_shouldFileBeAnalyzed(shortName)) {
+          ChangeSet changeSet = new ChangeSet();
+          Resource resource = resourceProvider.getResource(event.path);
+          // Only add the file if it didn't go away before we had a chance
+          // to process the event.
+          if (resource is File) {

[scheglov, 2014/05/29 17:37:03]

AFAIK getResource() will always return a File, it is up to the client to check
if the resource exists or not.

[Paul Berry, 2014/05/29 17:57:21]

Ok, my comment is wrong, but I believe this if-test is still needed, because
getResource() might also return a Folder.

+            File file = resource;
+            Source source = file.createSource(UriKind.FILE_URI);
+            changeSet.addedSource(source);
+            applyChangesToContext(folder, changeSet);
+            info.sources[event.path]= source;
+          }
+        }
+        break;
+      case ChangeType.REMOVE:
+        // TODO(paulberry): handle removing pubspec.yaml
+        Source source = info.sources[event.path];
+        if (source != null) {
+          ChangeSet changeSet = new ChangeSet();
+          changeSet.removedSource(source);
+          applyChangesToContext(folder, changeSet);
+          info.sources.remove(event.path);
+        }
+        break;
+      case ChangeType.MODIFY:
+        // TODO(paulberry): handle modification events
+        break;
+    }
   }
 
   /**
    * Resursively adds all Dart and HTML files to the [changeSet].
    */
-  static void _addSourceFiles(ChangeSet changeSet, Folder folder) {
+  static void _addSourceFiles(ChangeSet changeSet, Folder folder, _ContextDirectoryInfo info) {
     List<Resource> children = folder.getChildren();
     for (Resource child in children) {
       if (child is File) {
         String fileName = child.shortName;
-        if (AnalysisEngine.isDartFileName(fileName)
-            || AnalysisEngine.isHtmlFileName(fileName)) {
+        if (_shouldFileBeAnalyzed(fileName)) {
           Source source = child.createSource(UriKind.FILE_URI);
           changeSet.addedSource(source);
+          // TODO(paulberry): this assumes child.fullName is synonymous with
+          // path.  Can we rely on that?  If so, maybe we should rename
+          // child.fullName to child.path.

[scheglov, 2014/05/29 17:37:03]

SGTM

+          info.sources[child.fullName] = source;
         }
       } else if (child is Folder) {
-        _addSourceFiles(changeSet, child);
+        _addSourceFiles(changeSet, child, info);
       }
     }
   }
 
+  static bool _shouldFileBeAnalyzed(String shortName) {

[scheglov, 2014/05/29 17:37:03]

I don't think isDartFileName() cares if it is given a short name or a full name.
So, might not need to get a basename() at all.

[Paul Berry, 2014/05/29 17:57:21]

You're right.  I'll rework this to remove the call to basename().

+    return AnalysisEngine.isDartFileName(shortName)
+            || AnalysisEngine.isHtmlFileName(shortName);
+  }
+
   /**
    * Called when a new context needs to be created.  If the context is
    * associated with a pubspec file, that file is passed in [pubspecFile];