From 7781e34ba28eb3981363d8420a6cb2fcb9d13968 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 29 May 2015 11:27:00 -0700 Subject: [PATCH 01/21] Introduce atom.directory-searcher service v0.1.0. The contract for a provider for the `atom.directory-searcher` service is defined by the spec of the `DefaultDirectorySearcher`. This modifies `Workspace::scan()` to use the appropriate `DirectorySearcher` for each member of `atom.project.getDirectories()` when scanning the workspace for files that match the specified regex. --- spec/task-spec.coffee | 26 ++++++++ spec/workspace-spec.coffee | 70 +++++++++++++++++++ src/default-directory-searcher.coffee | 61 +++++++++++++++++ src/task.coffee | 10 ++- src/workspace.coffee | 96 ++++++++++++++++++++++----- 5 files changed, 245 insertions(+), 18 deletions(-) create mode 100644 src/default-directory-searcher.coffee diff --git a/spec/task-spec.coffee b/spec/task-spec.coffee index bddb59d86..947db5567 100644 --- a/spec/task-spec.coffee +++ b/spec/task-spec.coffee @@ -70,3 +70,29 @@ describe "Task", -> task.terminate() expect(stdout.listeners('data').length).toBe 0 expect(stderr.listeners('data').length).toBe 0 + + describe "::cancel()", -> + it "dispatches 'task:cancelled' when invoked on an active task", -> + task = new Task(require.resolve('./fixtures/task-spec-handler')) + cancelledEventSpy = jasmine.createSpy('eventSpy') + task.on('task:cancelled', cancelledEventSpy) + completedEventSpy = jasmine.createSpy('eventSpy') + task.on('task:completed', completedEventSpy) + + expect(task.cancel()).toBe(true) + expect(cancelledEventSpy).toHaveBeenCalled() + expect(completedEventSpy).not.toHaveBeenCalled() + + it "does not dispatch 'task:cancelled' when invoked on an inactive task", -> + handlerResult = null + task = Task.once require.resolve('./fixtures/task-spec-handler'), (result) -> + handlerResult = result + + waitsFor -> + handlerResult? + + runs -> + cancelledEventSpy = jasmine.createSpy('eventSpy') + task.on('task:cancelled', cancelledEventSpy) + expect(task.cancel()).toBe(false) + expect(cancelledEventSpy).not.toHaveBeenCalled() diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index 4918fc65f..6b0b1af96 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -938,6 +938,76 @@ describe "Workspace", -> .then -> expect(resultPaths).toEqual([file2]) + describe "when a custom directory searcher is registered", -> + it "can override the DefaultDirectorySearcher on a per-directory basis", -> + foreignFilePath = 'ssh://foreign-directory:8080/hello.txt' + numPathsSearchedInDir2 = 1 + numPathsToPretendToSearchInCustomDirectorySearcher = 10 + class CustomDirectorySearcher + canSearchDirectory: (directory) -> directory.getPath() is dir1 + search: (directory, regexSource, onSearchResult, onSearchError, onPathsSearched, options) -> + searchResult1 = + filePath: foreignFilePath, + matches: [ + { + lineText: 'Hello world', + lineTextOffset: 0, + matchText: 'Hello', + range: [[0, 0], [0, 5]], + } + ] + onSearchResult(searchResult1) + onPathsSearched(numPathsToPretendToSearchInCustomDirectorySearcher) + promise = Promise.resolve() + promise.cancel = -> + promise + + atom.packages.serviceHub.provide( + "atom.directory-searcher", "0.1.0", new CustomDirectorySearcher()) + + resultPaths = [] + onPathsSearched = jasmine.createSpy('onPathsSearched') + waitsForPromise -> + atom.workspace.scan /aaaa/, {onPathsSearched}, ({filePath}) -> + resultPaths.push(filePath) + + runs -> + expect(resultPaths.sort()).toEqual([foreignFilePath, file2].sort()) + # onPathsSearched should be called once by each DirectorySearcher. The order is not + # guaranteed, so we can only verify the total number of paths searched is correct + # after the second call. + expect(onPathsSearched.callCount).toBe(2) + expect(onPathsSearched.mostRecentCall.args[0]).toBe( + numPathsToPretendToSearchInCustomDirectorySearcher + numPathsSearchedInDir2) + + it "can be cancelled by cancelling one of the DirectorySearchers", -> + customDirectorySearcherPromiseInstance = null + class CustomDirectorySearcherToCancel + canSearchDirectory: (directory) -> directory.getPath() is dir1 + search: (directory, regexSource, onSearchResult, onSearchError, onPathsSearched, options) -> + # Note that hoisting reject in this way is generally frowned upon. + hoistedReject = null + promise = new Promise (resolve, reject) -> + hoistedReject = reject + promise.cancel = -> hoistedReject() + customDirectorySearcherPromiseInstance = promise + promise + + atom.packages.serviceHub.provide( + "atom.directory-searcher", "0.1.0", new CustomDirectorySearcherToCancel()) + + resultPaths = [] + cancelableSearch = atom.workspace.scan /aaaa/, ({filePath}) -> + resultPaths.push(filePath) + customDirectorySearcherPromiseInstance.cancel() + + resultOfPromiseSearch = null + waitsForPromise -> + cancelableSearch.then (promiseResult) -> resultOfPromiseSearch = promiseResult + + runs -> + expect(resultOfPromiseSearch).toBe('cancelled') + describe "::replace(regex, replacementText, paths, iterator)", -> [filePath, commentFilePath, sampleContent, sampleCommentContent] = [] diff --git a/src/default-directory-searcher.coffee b/src/default-directory-searcher.coffee new file mode 100644 index 000000000..7adf409c6 --- /dev/null +++ b/src/default-directory-searcher.coffee @@ -0,0 +1,61 @@ +Task = require './task' + +# Default provider for the `atom.directory-searcher` service. +module.exports = +class DefaultDirectorySearcher + # Public: Determines whether this object supports search for a `Directory`. + # + # * `directory` {Directory} whose search needs might be supported by this object. + # + # Returns a `boolean` indicating whether this object can search this `Directory`. + canSearchDirectory: (directory) -> true + + # Public: Performs a text search for files in the specified `Directory`, subject to the + # specified parameters. + # + # Results are streamed back to the caller via `recordSearchResult()` and `recordSearchError()`. + # + # * `directory` {Directory} that has been accepted by this provider's `canSearchDirectory()` + # predicate. + # * `regexSource` {String} regex to search with. Produced via `RegExp::source`. + # (Note this reflects the "Use Regex" option exposed via the ProjectFindView UI.) + # * `onSearchResult` {Function} Should be called with each matching search result. + # * `searchResult` {Object} with the following keys: + # * `filePath` {String} absolute path to the matching file. + # * `matches` {Array} with object elements with the following keys: + # * `lineText` {String} The full text of the matching line (without a line terminator character). + # * `lineTextOffset` {Number} (This always seems to be 0?) + # * `matchText` {String} The text that matched the `regex` used for the search. + # * `range` {Range} Identifies the matching region in the file. (Likely as an array of numeric arrays.) + # * `onSearchError` {Function} Should be called to report a search error. + # * `onPathsSearched` {Function} callback that should be invoked periodically with the number of + # paths searched. + # * `options` {Object} with the following properties: + # * `ignoreCase` {boolean} + # * `inclusions` {Array} of glob patterns (as strings) to search within. Note that this + # array may be empty, indicating that all files should be searched. + # + # Each item in the array is a file/directory pattern, e.g., `src` to search in the "src" + # directory or `*.js` to search all JavaScript files. In practice, this often comes from the + # comma-delimited list of patterns in the bottom text input of the ProjectFindView dialog. + # * `ignoreHidden` {boolean} + # * `excludeVcsIgnores` {boolean} + # * `exclusions` {Array} similar to inclusions + # * `follow` {boolean} whether symlinks should be followed + # + # Returns a `Promise` that includes a `cancel()` method. If invoked before the `Proimse` is + # determined, it will reject the `Promise`. + search: (directory, regexSource, onSearchResult, onSearchError, onPathsSearched, options) -> + task = null + rootPaths = [directory.getPath()] + promise = new Promise (resolve, reject) -> + task = Task.once require.resolve('./scan-handler'), rootPaths, regexSource, options, resolve + task.on 'task:cancelled', reject + promise.cancel = -> + task.cancel() + + task.on 'scan:result-found', onSearchResult + task.on 'scan:file-error', onSearchError + task.on 'scan:paths-searched', onPathsSearched + + promise diff --git a/src/task.coffee b/src/task.coffee index 939b71635..34c943c6a 100644 --- a/src/task.coffee +++ b/src/task.coffee @@ -150,7 +150,7 @@ class Task # # No more events are emitted once this method is called. terminate: -> - return unless @childProcess? + return false unless @childProcess? @childProcess.removeAllListeners() @childProcess.stdout.removeAllListeners() @@ -158,4 +158,10 @@ class Task @childProcess.kill() @childProcess = null - undefined + true + + cancel: -> + didForcefullyTerminate = @terminate() + if didForcefullyTerminate + @emit('task:cancelled') + didForcefullyTerminate diff --git a/src/workspace.coffee b/src/workspace.coffee index 157d002de..c7c5d51b1 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -7,6 +7,7 @@ Serializable = require 'serializable' {Emitter, Disposable, CompositeDisposable} = require 'event-kit' Grim = require 'grim' fs = require 'fs-plus' +DefaultDirectorySearcher = require './default-directory-searcher' Model = require './model' TextEditor = require './text-editor' PaneContainer = require './pane-container' @@ -46,6 +47,14 @@ class Workspace extends Model @paneContainer ?= new PaneContainer() @paneContainer.onDidDestroyPaneItem(@didDestroyPaneItem) + @searchProviders = [new DefaultDirectorySearcher()] + atom.packages.serviceHub.consume( + 'atom.directory-searcher', + '^0.1.0', + # New providers are added to the front of @searchProviders because + # DefaultDirectorySearcher is a catch-all that will always claim to search a Directory. + (provider) => @searchProviders.unshift(provider)) + @panelContainers = top: new PanelContainer({location: 'top'}) left: new PanelContainer({location: 'left'}) @@ -791,36 +800,72 @@ class Workspace extends Model # * `regex` {RegExp} to search with. # * `options` (optional) {Object} (default: {}) # * `paths` An {Array} of glob patterns to search within + # * `onPathsSearched` (optional) {Function} # * `iterator` {Function} callback on each file found # - # Returns a `Promise`. + # Returns a `Promise` with a `cancel()` method. scan: (regex, options={}, iterator) -> if _.isFunction(options) iterator = options options = {} - deferred = Q.defer() - searchOptions = ignoreCase: regex.ignoreCase - inclusions: options.paths + inclusions: options.paths or [] includeHidden: true excludeVcsIgnores: atom.config.get('core.excludeVcsIgnoredPaths') exclusions: atom.config.get('core.ignoredNames') follow: atom.config.get('core.followSymlinks') - task = Task.once require.resolve('./scan-handler'), atom.project.getPaths(), regex.source, searchOptions, -> - deferred.resolve() + # Find a search provider for every Directory in the project. + providersAndDirectories = [] + for directory in atom.project.getDirectories() + providerForDirectory = null + for provider in @searchProviders + if provider.canSearchDirectory(directory) + providerForDirectory = provider + break + if providerForDirectory + providersAndDirectories.push({provider, directory}) + else + throw Error("Could not find search provider for #{directory.getPath()}") - task.on 'scan:result-found', (result) -> + # Now that we are sure every Directory has a provider, construct the search options. + onSearchResult = (result) -> iterator(result) unless atom.project.isPathModified(result.filePath) - - task.on 'scan:file-error', (error) -> + onSearchError = (error) -> iterator(null, error) + # Define the onPathsSearched callback. if _.isFunction(options.onPathsSearched) - task.on 'scan:paths-searched', (numberOfPathsSearched) -> - options.onPathsSearched(numberOfPathsSearched) + # Maintain a map of providers to the number of search results. When notified of a new count, + # replace the entry in the map and update the total. + onPathsSearchedOption = options.onPathsSearched + totalNumberOfPathsSearched = 0 + numberOfPathsSearchedForProvider = new Map() + onPathsSearched = (provider, numberOfPathsSearched) -> + oldValue = numberOfPathsSearchedForProvider.get(provider) + if oldValue + totalNumberOfPathsSearched -= oldValue + numberOfPathsSearchedForProvider.set(provider, numberOfPathsSearched) + totalNumberOfPathsSearched += numberOfPathsSearched + onPathsSearchedOption(totalNumberOfPathsSearched) + else + onPathsSearched = -> + + # Kick off all of the searches and unify them into one Promise. + allSearchPromises = [] + for entry in providersAndDirectories + {provider, directory} = entry + recordNumPathsSearched = onPathsSearched.bind(undefined, provider) + allSearchPromises.push(provider.search( + directory, + regex.source, + onSearchResult, + onSearchError, + recordNumPathsSearched, + searchOptions)) + searchPromise = Promise.all(allSearchPromises) for buffer in atom.project.getBuffers() when buffer.isModified() filePath = buffer.getPath() @@ -829,11 +874,30 @@ class Workspace extends Model buffer.scan regex, (match) -> matches.push match iterator {filePath, matches} if matches.length > 0 - promise = deferred.promise - promise.cancel = -> - task.terminate() - deferred.resolve('cancelled') - promise + # Make sure the Promise that is returned to the client is cancelable. To be consistent + # with the existing behavior, instead of cancel() rejecting the promise, it should + # resolve it with the special value 'cancelled'. At least the built-in find-and-replace + # package relies on this behavior. + cancellablePromise = new Promise (resolve, reject) -> + onSuccess = -> + resolve(null) + return + onFailure = -> + resolve('cancelled') + return + searchPromise.then(onSuccess, onFailure) + cancellablePromise.cancel = -> + # Note that cancelling all (or actually, any) of the members of allSearchPromises + # will cause searchPromise to reject, which will cause cancellablePromise to resolve + # in the desired way. + promise.cancel() for promise in allSearchPromises + + # Although this method claims to return a `Promise`, the `ResultsPaneView.onSearch()` + # method in the find-and-replace package expects the object returned by this method to have a + # `done()` method. Include a done() method until find-and-replace can be updated. + cancellablePromise.done = (f) -> + cancellablePromise.then(f, f) + cancellablePromise # Public: Performs a replace across all the specified files in the project. # From 986640f67085427503faddc320d8251b095d0aed Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 2 Jun 2015 12:01:23 -0400 Subject: [PATCH 02/21] Respond to comments from @maxbrunsfeld and @kevinsawicki. --- src/workspace.coffee | 50 ++++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/src/workspace.coffee b/src/workspace.coffee index c7c5d51b1..a22e2409d 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -47,13 +47,13 @@ class Workspace extends Model @paneContainer ?= new PaneContainer() @paneContainer.onDidDestroyPaneItem(@didDestroyPaneItem) - @searchProviders = [new DefaultDirectorySearcher()] + @directorySearchers = [new DefaultDirectorySearcher()] atom.packages.serviceHub.consume( 'atom.directory-searcher', '^0.1.0', - # New providers are added to the front of @searchProviders because + # New providers are added to the front of @directorySearchers because # DefaultDirectorySearcher is a catch-all that will always claim to search a Directory. - (provider) => @searchProviders.unshift(provider)) + (provider) => @directorySearchers.unshift(provider)) @panelContainers = top: new PanelContainer({location: 'top'}) @@ -817,20 +817,20 @@ class Workspace extends Model exclusions: atom.config.get('core.ignoredNames') follow: atom.config.get('core.followSymlinks') - # Find a search provider for every Directory in the project. - providersAndDirectories = [] + # Find a searcher for every Directory in the project. + searchersAndDirectories = [] for directory in atom.project.getDirectories() - providerForDirectory = null - for provider in @searchProviders - if provider.canSearchDirectory(directory) - providerForDirectory = provider + searcher = null + for directorySearcher in @directorySearchers + if directorySearcher.canSearchDirectory(directory) + searcher = directorySearcher break - if providerForDirectory - providersAndDirectories.push({provider, directory}) + if searcher + searchersAndDirectories.push({searcher, directory}) else - throw Error("Could not find search provider for #{directory.getPath()}") + throw Error("Could not find directory searcher for #{directory.getPath()}") - # Now that we are sure every Directory has a provider, construct the search options. + # Now that we are sure every Directory has a searcher, construct the search options. onSearchResult = (result) -> iterator(result) unless atom.project.isPathModified(result.filePath) onSearchError = (error) -> @@ -838,16 +838,16 @@ class Workspace extends Model # Define the onPathsSearched callback. if _.isFunction(options.onPathsSearched) - # Maintain a map of providers to the number of search results. When notified of a new count, + # Maintain a map of directories to the number of search results. When notified of a new count, # replace the entry in the map and update the total. onPathsSearchedOption = options.onPathsSearched totalNumberOfPathsSearched = 0 - numberOfPathsSearchedForProvider = new Map() - onPathsSearched = (provider, numberOfPathsSearched) -> - oldValue = numberOfPathsSearchedForProvider.get(provider) + numberOfPathsSearchedForDirectory = new Map() + onPathsSearched = (directory, numberOfPathsSearched) -> + oldValue = numberOfPathsSearchedForDirectory.get(directory) if oldValue totalNumberOfPathsSearched -= oldValue - numberOfPathsSearchedForProvider.set(provider, numberOfPathsSearched) + numberOfPathsSearchedForDirectory.set(directory, numberOfPathsSearched) totalNumberOfPathsSearched += numberOfPathsSearched onPathsSearchedOption(totalNumberOfPathsSearched) else @@ -855,15 +855,15 @@ class Workspace extends Model # Kick off all of the searches and unify them into one Promise. allSearchPromises = [] - for entry in providersAndDirectories - {provider, directory} = entry - recordNumPathsSearched = onPathsSearched.bind(undefined, provider) - allSearchPromises.push(provider.search( + for entry in searchersAndDirectories + {searcher, directory} = entry + recordNumberOfPathsSearched = onPathsSearched.bind(undefined, directory) + allSearchPromises.push(searcher.search( directory, regex.source, onSearchResult, onSearchError, - recordNumPathsSearched, + recordNumberOfPathsSearched, searchOptions)) searchPromise = Promise.all(allSearchPromises) @@ -895,8 +895,8 @@ class Workspace extends Model # Although this method claims to return a `Promise`, the `ResultsPaneView.onSearch()` # method in the find-and-replace package expects the object returned by this method to have a # `done()` method. Include a done() method until find-and-replace can be updated. - cancellablePromise.done = (f) -> - cancellablePromise.then(f, f) + cancellablePromise.done = (onSuccessOrFailure) -> + cancellablePromise.then(onSuccessOrFailure, onSuccessOrFailure) cancellablePromise # Public: Performs a replace across all the specified files in the project. From 7294e0cda409803cbd3956119eeb35cee4596ba0 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 2 Jun 2015 14:04:59 -0400 Subject: [PATCH 03/21] Change DirectorySearcher to return a DirectorySearch. --- spec/workspace-spec.coffee | 57 +++++++++++++++++------- src/default-directory-searcher.coffee | 62 ++++++++++++++++++++------- src/workspace.coffee | 21 +++++---- 3 files changed, 100 insertions(+), 40 deletions(-) diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index 6b0b1af96..19775e6fa 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -1,5 +1,6 @@ path = require 'path' temp = require 'temp' +{Disposable} = require 'event-kit' Workspace = require '../src/workspace' Pane = require '../src/pane' {View} = require '../src/space-pen-extensions' @@ -943,9 +944,13 @@ describe "Workspace", -> foreignFilePath = 'ssh://foreign-directory:8080/hello.txt' numPathsSearchedInDir2 = 1 numPathsToPretendToSearchInCustomDirectorySearcher = 10 - class CustomDirectorySearcher - canSearchDirectory: (directory) -> directory.getPath() is dir1 - search: (directory, regexSource, onSearchResult, onSearchError, onPathsSearched, options) -> + class CustomDirectorySearch + constructor: () -> + @promise = Promise.resolve() + then: (args...) -> + @promise.then.apply(@promise, args) + onDidMatch: (callback) -> + # Invoke the callback with the only result we plan to return. searchResult1 = filePath: foreignFilePath, matches: [ @@ -956,11 +961,20 @@ describe "Workspace", -> range: [[0, 0], [0, 5]], } ] - onSearchResult(searchResult1) - onPathsSearched(numPathsToPretendToSearchInCustomDirectorySearcher) - promise = Promise.resolve() - promise.cancel = -> - promise + callback(searchResult1) + new Disposable + onDidError: (callback) -> + new Disposable + onDidSearchPaths: (callback) -> + # Invoke the callback with the one notification we plan to send. + callback(numPathsToPretendToSearchInCustomDirectorySearcher) + new Disposable + cancel: -> + + class CustomDirectorySearcher + canSearchDirectory: (directory) -> directory.getPath() is dir1 + search: (directory, options) -> + new CustomDirectorySearch atom.packages.serviceHub.provide( "atom.directory-searcher", "0.1.0", new CustomDirectorySearcher()) @@ -982,16 +996,27 @@ describe "Workspace", -> it "can be cancelled by cancelling one of the DirectorySearchers", -> customDirectorySearcherPromiseInstance = null + class CustomDirectorySearchToCancel + constructor: () -> + # Note that hoisting reject in this way is generally frowned upon. + @promise = new Promise (resolve, reject) => + @hoistedReject = reject + customDirectorySearcherPromiseInstance = this + then: (args...) -> + @promise.then.apply(@promise, args) + onDidMatch: (callback) -> + new Disposable + onDidError: (callback) -> + new Disposable + onDidSearchPaths: (callback) -> + new Disposable + cancel: -> + @hoistedReject() + class CustomDirectorySearcherToCancel canSearchDirectory: (directory) -> directory.getPath() is dir1 - search: (directory, regexSource, onSearchResult, onSearchError, onPathsSearched, options) -> - # Note that hoisting reject in this way is generally frowned upon. - hoistedReject = null - promise = new Promise (resolve, reject) -> - hoistedReject = reject - promise.cancel = -> hoistedReject() - customDirectorySearcherPromiseInstance = promise - promise + search: (directory, options) -> + new CustomDirectorySearchToCancel atom.packages.serviceHub.provide( "atom.directory-searcher", "0.1.0", new CustomDirectorySearcherToCancel()) diff --git a/src/default-directory-searcher.coffee b/src/default-directory-searcher.coffee index 7adf409c6..a1304d26c 100644 --- a/src/default-directory-searcher.coffee +++ b/src/default-directory-searcher.coffee @@ -1,5 +1,47 @@ Task = require './task' +# Public: +# Implements thenable so it can be used with `Promise.all()`. +class DirectorySearch + # Public: + constructor: (directory, options) -> + @task = new Task(require.resolve('./scan-handler')) + rootPaths = [directory.getPath()] + @promise = new Promise (resolve, reject) => + myResolve = (arg) -> + resolve(arg) + @task.start(rootPaths, options.regexSource, options, myResolve) + @task.on('task:cancelled', reject) + + # Public: + # Returns `Promise`. + then: (args...) -> + @promise.then.apply(@promise, args) + + # Public: + # Returns `Disposable`. + onDidMatch: (callback) -> + @task.on 'scan:result-found', callback + + # Public: + # Returns `Disposable`. + onDidError: (callback) -> + @task.on 'scan:file-error', callback + + # Public: + # + # * `callback` {Function} called with the number of paths searched thus far. + # + # Returns `Disposable`. + onDidSearchPaths: (callback) -> + @task.on 'scan:paths-searched', callback + + # Public: + cancel: -> + # This will cause @promise to reject. + @task.cancel() + + # Default provider for the `atom.directory-searcher` service. module.exports = class DefaultDirectorySearcher @@ -17,7 +59,6 @@ class DefaultDirectorySearcher # # * `directory` {Directory} that has been accepted by this provider's `canSearchDirectory()` # predicate. - # * `regexSource` {String} regex to search with. Produced via `RegExp::source`. # (Note this reflects the "Use Regex" option exposed via the ProjectFindView UI.) # * `onSearchResult` {Function} Should be called with each matching search result. # * `searchResult` {Object} with the following keys: @@ -31,6 +72,7 @@ class DefaultDirectorySearcher # * `onPathsSearched` {Function} callback that should be invoked periodically with the number of # paths searched. # * `options` {Object} with the following properties: + # * `regexSource` {String} regex to search with. Produced via `RegExp::source`. # * `ignoreCase` {boolean} # * `inclusions` {Array} of glob patterns (as strings) to search within. Note that this # array may be empty, indicating that all files should be searched. @@ -43,19 +85,7 @@ class DefaultDirectorySearcher # * `exclusions` {Array} similar to inclusions # * `follow` {boolean} whether symlinks should be followed # - # Returns a `Promise` that includes a `cancel()` method. If invoked before the `Proimse` is + # Returns a `DirectorySearch` that includes a `cancel()` method. If invoked before the `Proimse` is # determined, it will reject the `Promise`. - search: (directory, regexSource, onSearchResult, onSearchError, onPathsSearched, options) -> - task = null - rootPaths = [directory.getPath()] - promise = new Promise (resolve, reject) -> - task = Task.once require.resolve('./scan-handler'), rootPaths, regexSource, options, resolve - task.on 'task:cancelled', reject - promise.cancel = -> - task.cancel() - - task.on 'scan:result-found', onSearchResult - task.on 'scan:file-error', onSearchError - task.on 'scan:paths-searched', onPathsSearched - - promise + search: (directory, options) -> + new DirectorySearch(directory, options) diff --git a/src/workspace.coffee b/src/workspace.coffee index a22e2409d..fe79f5d17 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -803,13 +803,14 @@ class Workspace extends Model # * `onPathsSearched` (optional) {Function} # * `iterator` {Function} callback on each file found # - # Returns a `Promise` with a `cancel()` method. + # Returns a `Promise`. scan: (regex, options={}, iterator) -> if _.isFunction(options) iterator = options options = {} searchOptions = + regexSource: regex.source ignoreCase: regex.ignoreCase inclusions: options.paths or [] includeHidden: true @@ -855,18 +856,22 @@ class Workspace extends Model # Kick off all of the searches and unify them into one Promise. allSearchPromises = [] + disposables = new CompositeDisposable for entry in searchersAndDirectories {searcher, directory} = entry + directorySearcher = searcher.search(directory, searchOptions) + disposables.add(directorySearcher.onDidMatch(onSearchResult)) + disposables.add(directorySearcher.onDidError(onSearchError)) recordNumberOfPathsSearched = onPathsSearched.bind(undefined, directory) - allSearchPromises.push(searcher.search( - directory, - regex.source, - onSearchResult, - onSearchError, - recordNumberOfPathsSearched, - searchOptions)) + disposables.add(directorySearcher.onDidSearchPaths(recordNumberOfPathsSearched)) + allSearchPromises.push(directorySearcher) searchPromise = Promise.all(allSearchPromises) + # Make sure to clean up the disposables once the searchPromise is determined. + disposeAll = (args...) -> + disposables.dispose() + searchPromise.then(disposeAll, disposeAll) + for buffer in atom.project.getBuffers() when buffer.isModified() filePath = buffer.getPath() continue unless atom.project.contains(filePath) From 4330c3a181c63b5dfed453702296cb38796c5721 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 2 Jun 2015 14:15:55 -0400 Subject: [PATCH 04/21] Fix some nits I found during my self-review. --- spec/workspace-spec.coffee | 4 ++-- src/workspace.coffee | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index 19775e6fa..3a709165b 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -945,7 +945,7 @@ describe "Workspace", -> numPathsSearchedInDir2 = 1 numPathsToPretendToSearchInCustomDirectorySearcher = 10 class CustomDirectorySearch - constructor: () -> + constructor: -> @promise = Promise.resolve() then: (args...) -> @promise.then.apply(@promise, args) @@ -997,7 +997,7 @@ describe "Workspace", -> it "can be cancelled by cancelling one of the DirectorySearchers", -> customDirectorySearcherPromiseInstance = null class CustomDirectorySearchToCancel - constructor: () -> + constructor: -> # Note that hoisting reject in this way is generally frowned upon. @promise = new Promise (resolve, reject) => @hoistedReject = reject diff --git a/src/workspace.coffee b/src/workspace.coffee index fe79f5d17..f5cbe2049 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -855,7 +855,7 @@ class Workspace extends Model onPathsSearched = -> # Kick off all of the searches and unify them into one Promise. - allSearchPromises = [] + allSearches = [] disposables = new CompositeDisposable for entry in searchersAndDirectories {searcher, directory} = entry @@ -864,8 +864,8 @@ class Workspace extends Model disposables.add(directorySearcher.onDidError(onSearchError)) recordNumberOfPathsSearched = onPathsSearched.bind(undefined, directory) disposables.add(directorySearcher.onDidSearchPaths(recordNumberOfPathsSearched)) - allSearchPromises.push(directorySearcher) - searchPromise = Promise.all(allSearchPromises) + allSearches.push(directorySearcher) + searchPromise = Promise.all(allSearches) # Make sure to clean up the disposables once the searchPromise is determined. disposeAll = (args...) -> @@ -887,15 +887,15 @@ class Workspace extends Model onSuccess = -> resolve(null) return - onFailure = -> + onFailure = -> resolve('cancelled') return searchPromise.then(onSuccess, onFailure) cancellablePromise.cancel = -> - # Note that cancelling all (or actually, any) of the members of allSearchPromises + # Note that cancelling all (or actually, any) of the members of allSearches # will cause searchPromise to reject, which will cause cancellablePromise to resolve # in the desired way. - promise.cancel() for promise in allSearchPromises + promise.cancel() for promise in allSearches # Although this method claims to return a `Promise`, the `ResultsPaneView.onSearch()` # method in the find-and-replace package expects the object returned by this method to have a From fd670a4dd4096413e65545d0c34ce27643febbbb Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 2 Jun 2015 14:19:55 -0400 Subject: [PATCH 05/21] Remove `myResolve` local variable that I was using for debugging. --- src/default-directory-searcher.coffee | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/default-directory-searcher.coffee b/src/default-directory-searcher.coffee index a1304d26c..af04792eb 100644 --- a/src/default-directory-searcher.coffee +++ b/src/default-directory-searcher.coffee @@ -8,9 +8,7 @@ class DirectorySearch @task = new Task(require.resolve('./scan-handler')) rootPaths = [directory.getPath()] @promise = new Promise (resolve, reject) => - myResolve = (arg) -> - resolve(arg) - @task.start(rootPaths, options.regexSource, options, myResolve) + @task.start(rootPaths, options.regexSource, options, resolve) @task.on('task:cancelled', reject) # Public: From 735bdcca0874561373b7eb5debeea8c79cc4cfa2 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 2 Jun 2015 16:25:44 -0400 Subject: [PATCH 06/21] Two things: 1. Update documentation for `default-directory-search.coffee`. 2. Ensure that multiple `DirectorySearch` searches are run in series rather than in parallel to conserve resources. --- src/default-directory-searcher.coffee | 94 ++++++++++++++++++--------- 1 file changed, 65 insertions(+), 29 deletions(-) diff --git a/src/default-directory-searcher.coffee b/src/default-directory-searcher.coffee index af04792eb..448c47377 100644 --- a/src/default-directory-searcher.coffee +++ b/src/default-directory-searcher.coffee @@ -1,32 +1,73 @@ +{EventEmitter} = require 'events' Task = require './task' -# Public: +# Maintain a queue of ids of searches to run. When a search is complete, the active +# search is cleared and the next search (if any) in the queue is notified to run. +# This ensures there is at most one scan-handler task running at a time. +searchQueue = [] +nextId = 1 +activeSearchId = 0 +emitter = new EventEmitter + +onSearchFinished = () -> + activeSearchId = null + runNextSearch() + +runNextSearch = () -> + unless activeSearchId + activeSearchId = searchQueue.shift() + emitter.emit(activeSearchId, null) if activeSearchId + +enqueue = (id) -> + searchQueue.push(id) + runNextSearch() + + +# Public: Searches local files for lines matching a specified regex. +# # Implements thenable so it can be used with `Promise.all()`. class DirectorySearch - # Public: - constructor: (directory, options) -> + # Public: Creates a new DirectorySearch that will not start running until the + # `emitter` that is private to this file emits an event with the specified `id`. + constructor: (directory, options, id) -> @task = new Task(require.resolve('./scan-handler')) rootPaths = [directory.getPath()] @promise = new Promise (resolve, reject) => - @task.start(rootPaths, options.regexSource, options, resolve) @task.on('task:cancelled', reject) + emitter.once id, => + @task.start(rootPaths, options.regexSource, options, resolve) - # Public: + # Public: Implementation of `then()` to satisfy the *thenable* contract. + # This makes it possible to use a `DirectorySearch` with `Promise.all()`. + # # Returns `Promise`. then: (args...) -> @promise.then.apply(@promise, args) - # Public: + # Public: Get notified when a search result is found. + # + # * `callback` {Function} called with a search result structured as follows: + # * `searchResult` {Object} with the following keys: + # * `filePath` {String} absolute path to the matching file. + # * `matches` {Array} with object elements with the following keys: + # * `lineText` {String} The full text of the matching line (without a line terminator character). + # * `lineTextOffset` {Number} (This always seems to be 0?) + # * `matchText` {String} The text that matched the `regex` used for the search. + # * `range` {Range} Identifies the matching region in the file. (Likely as an array of numeric arrays.) + # # Returns `Disposable`. onDidMatch: (callback) -> @task.on 'scan:result-found', callback - # Public: + # Public: Get notified about any search errors. + # + # * `callback` {Function} called with an Error if there is a problem during the search. + # # Returns `Disposable`. onDidError: (callback) -> @task.on 'scan:file-error', callback - # Public: + # Public: Get notified with the number of paths searched thus far. # # * `callback` {Function} called with the number of paths searched thus far. # @@ -34,10 +75,11 @@ class DirectorySearch onDidSearchPaths: (callback) -> @task.on 'scan:paths-searched', callback - # Public: + # Public: Cancels the search. cancel: -> # This will cause @promise to reject. @task.cancel() + null # Default provider for the `atom.directory-searcher` service. @@ -53,37 +95,31 @@ class DefaultDirectorySearcher # Public: Performs a text search for files in the specified `Directory`, subject to the # specified parameters. # - # Results are streamed back to the caller via `recordSearchResult()` and `recordSearchError()`. + # Results are streamed back to the caller by adding callbacks to the `DirectorySearch` returned by + # this method. # # * `directory` {Directory} that has been accepted by this provider's `canSearchDirectory()` # predicate. - # (Note this reflects the "Use Regex" option exposed via the ProjectFindView UI.) - # * `onSearchResult` {Function} Should be called with each matching search result. - # * `searchResult` {Object} with the following keys: - # * `filePath` {String} absolute path to the matching file. - # * `matches` {Array} with object elements with the following keys: - # * `lineText` {String} The full text of the matching line (without a line terminator character). - # * `lineTextOffset` {Number} (This always seems to be 0?) - # * `matchText` {String} The text that matched the `regex` used for the search. - # * `range` {Range} Identifies the matching region in the file. (Likely as an array of numeric arrays.) - # * `onSearchError` {Function} Should be called to report a search error. - # * `onPathsSearched` {Function} callback that should be invoked periodically with the number of - # paths searched. # * `options` {Object} with the following properties: # * `regexSource` {String} regex to search with. Produced via `RegExp::source`. - # * `ignoreCase` {boolean} + # * `ignoreCase` {boolean} reflects whether the regex should be run with the `i` option. # * `inclusions` {Array} of glob patterns (as strings) to search within. Note that this # array may be empty, indicating that all files should be searched. # # Each item in the array is a file/directory pattern, e.g., `src` to search in the "src" # directory or `*.js` to search all JavaScript files. In practice, this often comes from the # comma-delimited list of patterns in the bottom text input of the ProjectFindView dialog. - # * `ignoreHidden` {boolean} - # * `excludeVcsIgnores` {boolean} + # * `ignoreHidden` {boolean} whether to ignore hidden files. + # * `excludeVcsIgnores` {boolean} whether to exclude VCS ignored paths. # * `exclusions` {Array} similar to inclusions - # * `follow` {boolean} whether symlinks should be followed + # * `follow` {boolean} whether symlinks should be followed. # - # Returns a `DirectorySearch` that includes a `cancel()` method. If invoked before the `Proimse` is - # determined, it will reject the `Promise`. + # Returns a *thenable* `DirectorySearch` that includes a `cancel()` method. If `cancel()` is + # invoked before the `DirectorySearch` is determined, it will reject the `DirectorySearch`. search: (directory, options) -> - new DirectorySearch(directory, options) + id = nextId + nextId += 1 + directorySearch = new DirectorySearch(directory, options, id) + directorySearch.then(onSearchFinished, onSearchFinished) + enqueue(id) + directorySearch From fa3fd9c50c6f196772c26bfb396ddcb8b23b1dc9 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 2 Jun 2015 16:28:31 -0400 Subject: [PATCH 07/21] Remove empty param lists as per the linter. --- src/default-directory-searcher.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/default-directory-searcher.coffee b/src/default-directory-searcher.coffee index 448c47377..b244e4f94 100644 --- a/src/default-directory-searcher.coffee +++ b/src/default-directory-searcher.coffee @@ -9,11 +9,11 @@ nextId = 1 activeSearchId = 0 emitter = new EventEmitter -onSearchFinished = () -> +onSearchFinished = -> activeSearchId = null runNextSearch() -runNextSearch = () -> +runNextSearch = -> unless activeSearchId activeSearchId = searchQueue.shift() emitter.emit(activeSearchId, null) if activeSearchId From 4eb30f39259c1c226bab655c39739ecbaec50b57 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 2 Jun 2015 17:40:27 -0400 Subject: [PATCH 08/21] Switch to the delegate pattern, eliminating a nasty race condition and deleting a lot of code. --- spec/workspace-spec.coffee | 30 ++++----------- src/default-directory-searcher.coffee | 54 +++++++++------------------ src/workspace.coffee | 26 ++++++------- 3 files changed, 38 insertions(+), 72 deletions(-) diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index 3a709165b..1b05d6c10 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -945,12 +945,8 @@ describe "Workspace", -> numPathsSearchedInDir2 = 1 numPathsToPretendToSearchInCustomDirectorySearcher = 10 class CustomDirectorySearch - constructor: -> + constructor: (delegate) -> @promise = Promise.resolve() - then: (args...) -> - @promise.then.apply(@promise, args) - onDidMatch: (callback) -> - # Invoke the callback with the only result we plan to return. searchResult1 = filePath: foreignFilePath, matches: [ @@ -959,22 +955,18 @@ describe "Workspace", -> lineTextOffset: 0, matchText: 'Hello', range: [[0, 0], [0, 5]], - } + }, ] - callback(searchResult1) - new Disposable - onDidError: (callback) -> - new Disposable - onDidSearchPaths: (callback) -> - # Invoke the callback with the one notification we plan to send. - callback(numPathsToPretendToSearchInCustomDirectorySearcher) - new Disposable + delegate.onDidMatch(searchResult1) + delegate.onDidSearchPaths(numPathsToPretendToSearchInCustomDirectorySearcher) + then: (args...) -> + @promise.then.apply(@promise, args) cancel: -> class CustomDirectorySearcher canSearchDirectory: (directory) -> directory.getPath() is dir1 - search: (directory, options) -> - new CustomDirectorySearch + search: (directory, delegate, options) -> + new CustomDirectorySearch(delegate) atom.packages.serviceHub.provide( "atom.directory-searcher", "0.1.0", new CustomDirectorySearcher()) @@ -1004,12 +996,6 @@ describe "Workspace", -> customDirectorySearcherPromiseInstance = this then: (args...) -> @promise.then.apply(@promise, args) - onDidMatch: (callback) -> - new Disposable - onDidError: (callback) -> - new Disposable - onDidSearchPaths: (callback) -> - new Disposable cancel: -> @hoistedReject() diff --git a/src/default-directory-searcher.coffee b/src/default-directory-searcher.coffee index b244e4f94..eec1ae4de 100644 --- a/src/default-directory-searcher.coffee +++ b/src/default-directory-searcher.coffee @@ -29,13 +29,16 @@ enqueue = (id) -> class DirectorySearch # Public: Creates a new DirectorySearch that will not start running until the # `emitter` that is private to this file emits an event with the specified `id`. - constructor: (directory, options, id) -> + constructor: (directory, delegate, options, id) -> @task = new Task(require.resolve('./scan-handler')) rootPaths = [directory.getPath()] @promise = new Promise (resolve, reject) => @task.on('task:cancelled', reject) emitter.once id, => @task.start(rootPaths, options.regexSource, options, resolve) + @task.on 'scan:result-found', delegate.onDidMatch + @task.on 'scan:file-error', delegate.onDidError + @task.on 'scan:paths-searched', delegate.onDidSearchPaths # Public: Implementation of `then()` to satisfy the *thenable* contract. # This makes it possible to use a `DirectorySearch` with `Promise.all()`. @@ -44,37 +47,6 @@ class DirectorySearch then: (args...) -> @promise.then.apply(@promise, args) - # Public: Get notified when a search result is found. - # - # * `callback` {Function} called with a search result structured as follows: - # * `searchResult` {Object} with the following keys: - # * `filePath` {String} absolute path to the matching file. - # * `matches` {Array} with object elements with the following keys: - # * `lineText` {String} The full text of the matching line (without a line terminator character). - # * `lineTextOffset` {Number} (This always seems to be 0?) - # * `matchText` {String} The text that matched the `regex` used for the search. - # * `range` {Range} Identifies the matching region in the file. (Likely as an array of numeric arrays.) - # - # Returns `Disposable`. - onDidMatch: (callback) -> - @task.on 'scan:result-found', callback - - # Public: Get notified about any search errors. - # - # * `callback` {Function} called with an Error if there is a problem during the search. - # - # Returns `Disposable`. - onDidError: (callback) -> - @task.on 'scan:file-error', callback - - # Public: Get notified with the number of paths searched thus far. - # - # * `callback` {Function} called with the number of paths searched thus far. - # - # Returns `Disposable`. - onDidSearchPaths: (callback) -> - @task.on 'scan:paths-searched', callback - # Public: Cancels the search. cancel: -> # This will cause @promise to reject. @@ -95,11 +67,21 @@ class DefaultDirectorySearcher # Public: Performs a text search for files in the specified `Directory`, subject to the # specified parameters. # - # Results are streamed back to the caller by adding callbacks to the `DirectorySearch` returned by - # this method. + # Results are streamed back to the caller by invoking methods on the specified `delegate`. # # * `directory` {Directory} that has been accepted by this provider's `canSearchDirectory()` # predicate. + # * `delegate` {Object} with the following properties: + # * `onDidMatch` {Function} call with a search result structured as follows: + # * `searchResult` {Object} with the following keys: + # * `filePath` {String} absolute path to the matching file. + # * `matches` {Array} with object elements with the following keys: + # * `lineText` {String} The full text of the matching line (without a line terminator character). + # * `lineTextOffset` {Number} (This always seems to be 0?) + # * `matchText` {String} The text that matched the `regex` used for the search. + # * `range` {Range} Identifies the matching region in the file. (Likely as an array of numeric arrays.) + # * `onDidError` {Function} call with an Error if there is a problem during the search. + # * `onDidSearchPaths` {Function} periodically call with the number of paths searched thus far. # * `options` {Object} with the following properties: # * `regexSource` {String} regex to search with. Produced via `RegExp::source`. # * `ignoreCase` {boolean} reflects whether the regex should be run with the `i` option. @@ -116,10 +98,10 @@ class DefaultDirectorySearcher # # Returns a *thenable* `DirectorySearch` that includes a `cancel()` method. If `cancel()` is # invoked before the `DirectorySearch` is determined, it will reject the `DirectorySearch`. - search: (directory, options) -> + search: (directory, delegate, options) -> id = nextId nextId += 1 - directorySearch = new DirectorySearch(directory, options, id) + directorySearch = new DirectorySearch(directory, delegate, options, id) directorySearch.then(onSearchFinished, onSearchFinished) enqueue(id) directorySearch diff --git a/src/workspace.coffee b/src/workspace.coffee index f5cbe2049..e92b3ac34 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -832,10 +832,12 @@ class Workspace extends Model throw Error("Could not find directory searcher for #{directory.getPath()}") # Now that we are sure every Directory has a searcher, construct the search options. - onSearchResult = (result) -> - iterator(result) unless atom.project.isPathModified(result.filePath) - onSearchError = (error) -> - iterator(null, error) + delegateProto = { + onDidMatch: (result) -> + iterator(result) unless atom.project.isPathModified(result.filePath) + onDidError: (error) -> + iterator(null, error) + } # Define the onPathsSearched callback. if _.isFunction(options.onPathsSearched) @@ -856,22 +858,18 @@ class Workspace extends Model # Kick off all of the searches and unify them into one Promise. allSearches = [] - disposables = new CompositeDisposable for entry in searchersAndDirectories {searcher, directory} = entry - directorySearcher = searcher.search(directory, searchOptions) - disposables.add(directorySearcher.onDidMatch(onSearchResult)) - disposables.add(directorySearcher.onDidError(onSearchError)) recordNumberOfPathsSearched = onPathsSearched.bind(undefined, directory) - disposables.add(directorySearcher.onDidSearchPaths(recordNumberOfPathsSearched)) + delegate = Object.create(delegateProto, { + onDidSearchPaths: { + value: recordNumberOfPathsSearched, + } + }) + directorySearcher = searcher.search(directory, delegate, searchOptions) allSearches.push(directorySearcher) searchPromise = Promise.all(allSearches) - # Make sure to clean up the disposables once the searchPromise is determined. - disposeAll = (args...) -> - disposables.dispose() - searchPromise.then(disposeAll, disposeAll) - for buffer in atom.project.getBuffers() when buffer.isModified() filePath = buffer.getPath() continue unless atom.project.contains(filePath) From f7e822d41f7533358dece61980a37a43531a5c68 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 2 Jun 2015 17:52:03 -0400 Subject: [PATCH 09/21] Make delegate method names more delegate-like. --- spec/workspace-spec.coffee | 4 ++-- src/default-directory-searcher.coffee | 12 ++++++------ src/workspace.coffee | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index 1b05d6c10..29e4350b0 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -957,8 +957,8 @@ describe "Workspace", -> range: [[0, 0], [0, 5]], }, ] - delegate.onDidMatch(searchResult1) - delegate.onDidSearchPaths(numPathsToPretendToSearchInCustomDirectorySearcher) + delegate.didMatch(searchResult1) + delegate.didSearchPaths(numPathsToPretendToSearchInCustomDirectorySearcher) then: (args...) -> @promise.then.apply(@promise, args) cancel: -> diff --git a/src/default-directory-searcher.coffee b/src/default-directory-searcher.coffee index eec1ae4de..b209cca70 100644 --- a/src/default-directory-searcher.coffee +++ b/src/default-directory-searcher.coffee @@ -36,9 +36,9 @@ class DirectorySearch @task.on('task:cancelled', reject) emitter.once id, => @task.start(rootPaths, options.regexSource, options, resolve) - @task.on 'scan:result-found', delegate.onDidMatch - @task.on 'scan:file-error', delegate.onDidError - @task.on 'scan:paths-searched', delegate.onDidSearchPaths + @task.on 'scan:result-found', delegate.didMatch + @task.on 'scan:file-error', delegate.didError + @task.on 'scan:paths-searched', delegate.didSearchPaths # Public: Implementation of `then()` to satisfy the *thenable* contract. # This makes it possible to use a `DirectorySearch` with `Promise.all()`. @@ -72,7 +72,7 @@ class DefaultDirectorySearcher # * `directory` {Directory} that has been accepted by this provider's `canSearchDirectory()` # predicate. # * `delegate` {Object} with the following properties: - # * `onDidMatch` {Function} call with a search result structured as follows: + # * `didMatch` {Function} call with a search result structured as follows: # * `searchResult` {Object} with the following keys: # * `filePath` {String} absolute path to the matching file. # * `matches` {Array} with object elements with the following keys: @@ -80,8 +80,8 @@ class DefaultDirectorySearcher # * `lineTextOffset` {Number} (This always seems to be 0?) # * `matchText` {String} The text that matched the `regex` used for the search. # * `range` {Range} Identifies the matching region in the file. (Likely as an array of numeric arrays.) - # * `onDidError` {Function} call with an Error if there is a problem during the search. - # * `onDidSearchPaths` {Function} periodically call with the number of paths searched thus far. + # * `didError` {Function} call with an Error if there is a problem during the search. + # * `didSearchPaths` {Function} periodically call with the number of paths searched thus far. # * `options` {Object} with the following properties: # * `regexSource` {String} regex to search with. Produced via `RegExp::source`. # * `ignoreCase` {boolean} reflects whether the regex should be run with the `i` option. diff --git a/src/workspace.coffee b/src/workspace.coffee index e92b3ac34..f57888777 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -833,9 +833,9 @@ class Workspace extends Model # Now that we are sure every Directory has a searcher, construct the search options. delegateProto = { - onDidMatch: (result) -> + didMatch: (result) -> iterator(result) unless atom.project.isPathModified(result.filePath) - onDidError: (error) -> + didError: (error) -> iterator(null, error) } @@ -862,7 +862,7 @@ class Workspace extends Model {searcher, directory} = entry recordNumberOfPathsSearched = onPathsSearched.bind(undefined, directory) delegate = Object.create(delegateProto, { - onDidSearchPaths: { + didSearchPaths: { value: recordNumberOfPathsSearched, } }) From 898f7b87e8c43eb545f89394fa53a3d14b2ec347 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 2 Jun 2015 18:02:25 -0400 Subject: [PATCH 10/21] remove require for Disposable that is no longer needed --- spec/workspace-spec.coffee | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index 29e4350b0..b1e7b033c 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -1,6 +1,5 @@ path = require 'path' temp = require 'temp' -{Disposable} = require 'event-kit' Workspace = require '../src/workspace' Pane = require '../src/pane' {View} = require '../src/space-pen-extensions' From 10d9111f681d668dabe85a5c54f877ad0236c29a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 2 Jun 2015 19:11:08 -0400 Subject: [PATCH 11/21] Clean up test to verify that an individual failed search fails the overall search. --- spec/workspace-spec.coffee | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index b1e7b033c..524adde5f 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -985,31 +985,27 @@ describe "Workspace", -> expect(onPathsSearched.mostRecentCall.args[0]).toBe( numPathsToPretendToSearchInCustomDirectorySearcher + numPathsSearchedInDir2) - it "can be cancelled by cancelling one of the DirectorySearchers", -> - customDirectorySearcherPromiseInstance = null - class CustomDirectorySearchToCancel + it "will have the side-effect of failing the overall search if it fails", -> + # Note that hoisting reject in this way is generally frowned upon. + hoistedReject = null + class CustomDirectorySearchThatWillFail constructor: -> - # Note that hoisting reject in this way is generally frowned upon. - @promise = new Promise (resolve, reject) => - @hoistedReject = reject - customDirectorySearcherPromiseInstance = this + @promise = new Promise (resolve, reject) -> + hoistedReject = reject then: (args...) -> @promise.then.apply(@promise, args) cancel: -> - @hoistedReject() class CustomDirectorySearcherToCancel canSearchDirectory: (directory) -> directory.getPath() is dir1 search: (directory, options) -> - new CustomDirectorySearchToCancel + new CustomDirectorySearchThatWillFail atom.packages.serviceHub.provide( "atom.directory-searcher", "0.1.0", new CustomDirectorySearcherToCancel()) - resultPaths = [] - cancelableSearch = atom.workspace.scan /aaaa/, ({filePath}) -> - resultPaths.push(filePath) - customDirectorySearcherPromiseInstance.cancel() + cancelableSearch = atom.workspace.scan /aaaa/, -> + hoistedReject() resultOfPromiseSearch = null waitsForPromise -> From 5fc9d9e01afe5ff381ae3662d2629ed8bf4d5618 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 2 Jun 2015 19:14:02 -0400 Subject: [PATCH 12/21] Document and test the `cancel()` method on the object returned by `atom.workspace.scan()`. --- spec/workspace-spec.coffee | 31 +++++++++++++++++++++++++++++++ src/workspace.coffee | 3 ++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index 524adde5f..07c543968 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -985,6 +985,37 @@ describe "Workspace", -> expect(onPathsSearched.mostRecentCall.args[0]).toBe( numPathsToPretendToSearchInCustomDirectorySearcher + numPathsSearchedInDir2) + it "can be cancelled when the object returned by scan() has its cancel() method invoked", -> + lastCustomDirectorySearchCreated = null + class CustomDirectorySearch + constructor: -> + lastCustomDirectorySearchCreated = this + @promise = Promise.resolve() + then: (args...) -> + @promise.then.apply(@promise, args) + cancel: -> + + class CustomDirectorySearcher + canSearchDirectory: (directory) -> directory.getPath() is dir1 + search: (directory, delegate, options) -> + new CustomDirectorySearch + + atom.packages.serviceHub.provide( + "atom.directory-searcher", "0.1.0", new CustomDirectorySearcher()) + + thenable = atom.workspace.scan /aaaa/, -> + cancelSpy = spyOn(lastCustomDirectorySearchCreated, 'cancel').andCallThrough() + expect(cancelSpy).not.toHaveBeenCalled() + thenable.cancel() + expect(cancelSpy).toHaveBeenCalled() + + resultOfPromiseSearch = null + waitsForPromise -> + thenable.then (promiseResult) -> resultOfPromiseSearch = promiseResult + + runs -> + expect(resultOfPromiseSearch).toBe('cancelled') + it "will have the side-effect of failing the overall search if it fails", -> # Note that hoisting reject in this way is generally frowned upon. hoistedReject = null diff --git a/src/workspace.coffee b/src/workspace.coffee index f57888777..77706fd69 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -803,7 +803,8 @@ class Workspace extends Model # * `onPathsSearched` (optional) {Function} # * `iterator` {Function} callback on each file found # - # Returns a `Promise`. + # Returns a *thenable* object with a `cancel()` method that will cancel all + # of the underlying searches that were started as part of this scan. scan: (regex, options={}, iterator) -> if _.isFunction(options) iterator = options From 318498464a766ad8f946bf126d083fefb5773003 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 3 Jun 2015 21:35:30 -0400 Subject: [PATCH 13/21] Responded to all of @maxbrunsfeld's comments except for: * The effect of a failed search. * Letting `DirectorySearcher::search` take multiple directories. I'm working on those now. --- spec/workspace-spec.coffee | 105 ++++++++++---------------- src/default-directory-searcher.coffee | 31 +++++--- src/workspace.coffee | 46 ++++------- 3 files changed, 75 insertions(+), 107 deletions(-) diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index 07c543968..26e348f72 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -939,36 +939,47 @@ describe "Workspace", -> expect(resultPaths).toEqual([file2]) describe "when a custom directory searcher is registered", -> + fakeSearch = null + # Function that is invoked once all of the fields on fakeSearch are set. + onFakeSearchCreated = null + + class FakeSearch + constructor: (@options) -> + # Note that hoisting resolve and reject in this way is generally frowned upon. + @promise = new Promise (resolve, reject) => + @hoistedResolve = resolve + @hoistedReject = reject + onFakeSearchCreated?(this) + then: (args...) -> + @promise.then.apply(@promise, args) + cancel: -> @cancelled = true + + beforeEach -> + fakeSearch = null + onFakeSearchCreated = null + atom.packages.serviceHub.provide('atom.directory-searcher', '0.1.0', { + canSearchDirectory: (directory) -> directory.getPath() is dir1 + search: (directory, regex, options) -> fakeSearch = new FakeSearch(options) + }) + it "can override the DefaultDirectorySearcher on a per-directory basis", -> foreignFilePath = 'ssh://foreign-directory:8080/hello.txt' numPathsSearchedInDir2 = 1 numPathsToPretendToSearchInCustomDirectorySearcher = 10 - class CustomDirectorySearch - constructor: (delegate) -> - @promise = Promise.resolve() - searchResult1 = - filePath: foreignFilePath, - matches: [ - { - lineText: 'Hello world', - lineTextOffset: 0, - matchText: 'Hello', - range: [[0, 0], [0, 5]], - }, - ] - delegate.didMatch(searchResult1) - delegate.didSearchPaths(numPathsToPretendToSearchInCustomDirectorySearcher) - then: (args...) -> - @promise.then.apply(@promise, args) - cancel: -> - - class CustomDirectorySearcher - canSearchDirectory: (directory) -> directory.getPath() is dir1 - search: (directory, delegate, options) -> - new CustomDirectorySearch(delegate) - - atom.packages.serviceHub.provide( - "atom.directory-searcher", "0.1.0", new CustomDirectorySearcher()) + searchResult = + filePath: foreignFilePath, + matches: [ + { + lineText: 'Hello world', + lineTextOffset: 0, + matchText: 'Hello', + range: [[0, 0], [0, 5]], + }, + ] + onFakeSearchCreated = (fakeSearch) -> + fakeSearch.options.didMatch(searchResult) + fakeSearch.options.didSearchPaths(numPathsToPretendToSearchInCustomDirectorySearcher) + fakeSearch.hoistedResolve() resultPaths = [] onPathsSearched = jasmine.createSpy('onPathsSearched') @@ -986,28 +997,10 @@ describe "Workspace", -> numPathsToPretendToSearchInCustomDirectorySearcher + numPathsSearchedInDir2) it "can be cancelled when the object returned by scan() has its cancel() method invoked", -> - lastCustomDirectorySearchCreated = null - class CustomDirectorySearch - constructor: -> - lastCustomDirectorySearchCreated = this - @promise = Promise.resolve() - then: (args...) -> - @promise.then.apply(@promise, args) - cancel: -> - - class CustomDirectorySearcher - canSearchDirectory: (directory) -> directory.getPath() is dir1 - search: (directory, delegate, options) -> - new CustomDirectorySearch - - atom.packages.serviceHub.provide( - "atom.directory-searcher", "0.1.0", new CustomDirectorySearcher()) - thenable = atom.workspace.scan /aaaa/, -> - cancelSpy = spyOn(lastCustomDirectorySearchCreated, 'cancel').andCallThrough() - expect(cancelSpy).not.toHaveBeenCalled() + expect(fakeSearch.cancelled).toBe(undefined) thenable.cancel() - expect(cancelSpy).toHaveBeenCalled() + expect(fakeSearch.cancelled).toBe(true) resultOfPromiseSearch = null waitsForPromise -> @@ -1017,26 +1010,8 @@ describe "Workspace", -> expect(resultOfPromiseSearch).toBe('cancelled') it "will have the side-effect of failing the overall search if it fails", -> - # Note that hoisting reject in this way is generally frowned upon. - hoistedReject = null - class CustomDirectorySearchThatWillFail - constructor: -> - @promise = new Promise (resolve, reject) -> - hoistedReject = reject - then: (args...) -> - @promise.then.apply(@promise, args) - cancel: -> - - class CustomDirectorySearcherToCancel - canSearchDirectory: (directory) -> directory.getPath() is dir1 - search: (directory, options) -> - new CustomDirectorySearchThatWillFail - - atom.packages.serviceHub.provide( - "atom.directory-searcher", "0.1.0", new CustomDirectorySearcherToCancel()) - cancelableSearch = atom.workspace.scan /aaaa/, -> - hoistedReject() + fakeSearch.hoistedReject() resultOfPromiseSearch = null waitsForPromise -> diff --git a/src/default-directory-searcher.coffee b/src/default-directory-searcher.coffee index b209cca70..912ad818f 100644 --- a/src/default-directory-searcher.coffee +++ b/src/default-directory-searcher.coffee @@ -29,16 +29,24 @@ enqueue = (id) -> class DirectorySearch # Public: Creates a new DirectorySearch that will not start running until the # `emitter` that is private to this file emits an event with the specified `id`. - constructor: (directory, delegate, options, id) -> + constructor: (directory, regex, options, id) -> + scanHandlerOptions = + ignoreCase: regex.ignoreCase + inclusions: options.inclusions + includeHidden: options.includeHidden + excludeVcsIgnores: options.excludeVcsIgnores + exclusions: options.exclusions + follow: options.follow + @task = new Task(require.resolve('./scan-handler')) rootPaths = [directory.getPath()] @promise = new Promise (resolve, reject) => @task.on('task:cancelled', reject) emitter.once id, => - @task.start(rootPaths, options.regexSource, options, resolve) - @task.on 'scan:result-found', delegate.didMatch - @task.on 'scan:file-error', delegate.didError - @task.on 'scan:paths-searched', delegate.didSearchPaths + @task.start(rootPaths, regex.source, scanHandlerOptions, resolve) + @task.on 'scan:result-found', options.didMatch + @task.on 'scan:file-error', options.didError + @task.on 'scan:paths-searched', options.didSearchPaths # Public: Implementation of `then()` to satisfy the *thenable* contract. # This makes it possible to use a `DirectorySearch` with `Promise.all()`. @@ -67,11 +75,13 @@ class DefaultDirectorySearcher # Public: Performs a text search for files in the specified `Directory`, subject to the # specified parameters. # - # Results are streamed back to the caller by invoking methods on the specified `delegate`. + # Results are streamed back to the caller by invoking methods on the specified `options`, + # such as `didMatch` and `didError`. # # * `directory` {Directory} that has been accepted by this provider's `canSearchDirectory()` # predicate. - # * `delegate` {Object} with the following properties: + # * `regex` {RegExp} to search with. + # * `options` {Object} with the following properties: # * `didMatch` {Function} call with a search result structured as follows: # * `searchResult` {Object} with the following keys: # * `filePath` {String} absolute path to the matching file. @@ -82,9 +92,6 @@ class DefaultDirectorySearcher # * `range` {Range} Identifies the matching region in the file. (Likely as an array of numeric arrays.) # * `didError` {Function} call with an Error if there is a problem during the search. # * `didSearchPaths` {Function} periodically call with the number of paths searched thus far. - # * `options` {Object} with the following properties: - # * `regexSource` {String} regex to search with. Produced via `RegExp::source`. - # * `ignoreCase` {boolean} reflects whether the regex should be run with the `i` option. # * `inclusions` {Array} of glob patterns (as strings) to search within. Note that this # array may be empty, indicating that all files should be searched. # @@ -98,10 +105,10 @@ class DefaultDirectorySearcher # # Returns a *thenable* `DirectorySearch` that includes a `cancel()` method. If `cancel()` is # invoked before the `DirectorySearch` is determined, it will reject the `DirectorySearch`. - search: (directory, delegate, options) -> + search: (directory, regex, options) -> id = nextId nextId += 1 - directorySearch = new DirectorySearch(directory, delegate, options, id) + directorySearch = new DirectorySearch(directory, regex, options, id) directorySearch.then(onSearchFinished, onSearchFinished) enqueue(id) directorySearch diff --git a/src/workspace.coffee b/src/workspace.coffee index 77706fd69..e9ac46bd2 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -47,7 +47,8 @@ class Workspace extends Model @paneContainer ?= new PaneContainer() @paneContainer.onDidDestroyPaneItem(@didDestroyPaneItem) - @directorySearchers = [new DefaultDirectorySearcher()] + @directorySearchers = [] + @defaultDirectorySearcher = new DefaultDirectorySearcher() atom.packages.serviceHub.consume( 'atom.directory-searcher', '^0.1.0', @@ -810,35 +811,15 @@ class Workspace extends Model iterator = options options = {} - searchOptions = - regexSource: regex.source - ignoreCase: regex.ignoreCase - inclusions: options.paths or [] - includeHidden: true - excludeVcsIgnores: atom.config.get('core.excludeVcsIgnoredPaths') - exclusions: atom.config.get('core.ignoredNames') - follow: atom.config.get('core.followSymlinks') - # Find a searcher for every Directory in the project. searchersAndDirectories = [] for directory in atom.project.getDirectories() - searcher = null + searcher = @defaultDirectorySearcher for directorySearcher in @directorySearchers if directorySearcher.canSearchDirectory(directory) searcher = directorySearcher break - if searcher - searchersAndDirectories.push({searcher, directory}) - else - throw Error("Could not find directory searcher for #{directory.getPath()}") - - # Now that we are sure every Directory has a searcher, construct the search options. - delegateProto = { - didMatch: (result) -> - iterator(result) unless atom.project.isPathModified(result.filePath) - didError: (error) -> - iterator(null, error) - } + searchersAndDirectories.push({searcher, directory}) # Define the onPathsSearched callback. if _.isFunction(options.onPathsSearched) @@ -861,13 +842,18 @@ class Workspace extends Model allSearches = [] for entry in searchersAndDirectories {searcher, directory} = entry - recordNumberOfPathsSearched = onPathsSearched.bind(undefined, directory) - delegate = Object.create(delegateProto, { - didSearchPaths: { - value: recordNumberOfPathsSearched, - } - }) - directorySearcher = searcher.search(directory, delegate, searchOptions) + searchOptions = + inclusions: options.paths or [] + includeHidden: true + excludeVcsIgnores: atom.config.get('core.excludeVcsIgnoredPaths') + exclusions: atom.config.get('core.ignoredNames') + follow: atom.config.get('core.followSymlinks') + didMatch: (result) -> + iterator(result) unless atom.project.isPathModified(result.filePath) + didError: (error) -> + iterator(null, error) + didSearchPaths: onPathsSearched.bind(undefined, directory) + directorySearcher = searcher.search(directory, regex, searchOptions) allSearches.push(directorySearcher) searchPromise = Promise.all(allSearches) From 36123faf5d33c5aba6f1a11d2a20d6cc4b2348e4 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 3 Jun 2015 21:54:41 -0400 Subject: [PATCH 14/21] Changed DefaultDirectorySearcher to take multiple directories. Still need to update `workspace.coffee` to make better use of this. --- src/default-directory-searcher.coffee | 63 ++++++++++----------------- src/workspace.coffee | 2 +- 2 files changed, 25 insertions(+), 40 deletions(-) diff --git a/src/default-directory-searcher.coffee b/src/default-directory-searcher.coffee index 912ad818f..5db5db8c2 100644 --- a/src/default-directory-searcher.coffee +++ b/src/default-directory-searcher.coffee @@ -1,35 +1,12 @@ -{EventEmitter} = require 'events' Task = require './task' -# Maintain a queue of ids of searches to run. When a search is complete, the active -# search is cleared and the next search (if any) in the queue is notified to run. -# This ensures there is at most one scan-handler task running at a time. -searchQueue = [] -nextId = 1 -activeSearchId = 0 -emitter = new EventEmitter - -onSearchFinished = -> - activeSearchId = null - runNextSearch() - -runNextSearch = -> - unless activeSearchId - activeSearchId = searchQueue.shift() - emitter.emit(activeSearchId, null) if activeSearchId - -enqueue = (id) -> - searchQueue.push(id) - runNextSearch() - - # Public: Searches local files for lines matching a specified regex. # # Implements thenable so it can be used with `Promise.all()`. class DirectorySearch # Public: Creates a new DirectorySearch that will not start running until the # `emitter` that is private to this file emits an event with the specified `id`. - constructor: (directory, regex, options, id) -> + constructor: (rootPath, regex, options) -> scanHandlerOptions = ignoreCase: regex.ignoreCase inclusions: options.inclusions @@ -37,16 +14,13 @@ class DirectorySearch excludeVcsIgnores: options.excludeVcsIgnores exclusions: options.exclusions follow: options.follow - @task = new Task(require.resolve('./scan-handler')) - rootPaths = [directory.getPath()] - @promise = new Promise (resolve, reject) => - @task.on('task:cancelled', reject) - emitter.once id, => - @task.start(rootPaths, regex.source, scanHandlerOptions, resolve) @task.on 'scan:result-found', options.didMatch @task.on 'scan:file-error', options.didError @task.on 'scan:paths-searched', options.didSearchPaths + @promise = new Promise (resolve, reject) => + @task.on('task:cancelled', reject) + @task.start([rootPath], regex.source, scanHandlerOptions, resolve) # Public: Implementation of `then()` to satisfy the *thenable* contract. # This makes it possible to use a `DirectorySearch` with `Promise.all()`. @@ -78,8 +52,8 @@ class DefaultDirectorySearcher # Results are streamed back to the caller by invoking methods on the specified `options`, # such as `didMatch` and `didError`. # - # * `directory` {Directory} that has been accepted by this provider's `canSearchDirectory()` - # predicate. + # * `directories` {Array} of {Directory} objects to search, all of which have been accepted by + # this searcher's `canSearchDirectory()` predicate. # * `regex` {RegExp} to search with. # * `options` {Object} with the following properties: # * `didMatch` {Function} call with a search result structured as follows: @@ -105,10 +79,21 @@ class DefaultDirectorySearcher # # Returns a *thenable* `DirectorySearch` that includes a `cancel()` method. If `cancel()` is # invoked before the `DirectorySearch` is determined, it will reject the `DirectorySearch`. - search: (directory, regex, options) -> - id = nextId - nextId += 1 - directorySearch = new DirectorySearch(directory, regex, options, id) - directorySearch.then(onSearchFinished, onSearchFinished) - enqueue(id) - directorySearch + search: (directories, regex, options) -> + rootPaths = directories.map (directory) -> directory.getPath() + isCancelled = false + promise = new Promise (resolve, reject) -> + run = -> + if isCancelled + reject() + else if rootPaths.length + rootPath = rootPaths.shift() + thenable = new DirectorySearch(rootPath, regex, options) + thenable.then(run, reject) + else + resolve() + run() + return { + then: promise.then.bind(promise) + cancel: -> isCancelled = true + } diff --git a/src/workspace.coffee b/src/workspace.coffee index e9ac46bd2..a2d3059de 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -853,7 +853,7 @@ class Workspace extends Model didError: (error) -> iterator(null, error) didSearchPaths: onPathsSearched.bind(undefined, directory) - directorySearcher = searcher.search(directory, regex, searchOptions) + directorySearcher = searcher.search([directory], regex, searchOptions) allSearches.push(directorySearcher) searchPromise = Promise.all(allSearches) From 7dc3d07f8aa3e8251bcc6b8caf0a87f48dbc2bff Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 3 Jun 2015 22:20:39 -0400 Subject: [PATCH 15/21] Changed the contract of `DefaultDirectorySearcher` in two significant ways: * `search()` takes an array of `Directory` objects rather than an individual object. * `options.didSearchPaths` now takes the `Directory` in addition to the `count` as an argument. --- spec/workspace-spec.coffee | 3 +- src/default-directory-searcher.coffee | 18 ++++++------ src/workspace.coffee | 40 +++++++++++++++------------ 3 files changed, 34 insertions(+), 27 deletions(-) diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index 26e348f72..bfada8b43 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -978,7 +978,8 @@ describe "Workspace", -> ] onFakeSearchCreated = (fakeSearch) -> fakeSearch.options.didMatch(searchResult) - fakeSearch.options.didSearchPaths(numPathsToPretendToSearchInCustomDirectorySearcher) + directory1 = atom.project.getDirectories()[atom.project.getPaths().indexOf(dir1)] + fakeSearch.options.didSearchPaths(directory1, numPathsToPretendToSearchInCustomDirectorySearcher) fakeSearch.hoistedResolve() resultPaths = [] diff --git a/src/default-directory-searcher.coffee b/src/default-directory-searcher.coffee index 5db5db8c2..4767a0313 100644 --- a/src/default-directory-searcher.coffee +++ b/src/default-directory-searcher.coffee @@ -4,9 +4,7 @@ Task = require './task' # # Implements thenable so it can be used with `Promise.all()`. class DirectorySearch - # Public: Creates a new DirectorySearch that will not start running until the - # `emitter` that is private to this file emits an event with the specified `id`. - constructor: (rootPath, regex, options) -> + constructor: (directory, regex, options) -> scanHandlerOptions = ignoreCase: regex.ignoreCase inclusions: options.inclusions @@ -17,10 +15,10 @@ class DirectorySearch @task = new Task(require.resolve('./scan-handler')) @task.on 'scan:result-found', options.didMatch @task.on 'scan:file-error', options.didError - @task.on 'scan:paths-searched', options.didSearchPaths + @task.on 'scan:paths-searched', (count) -> options.didSearchPaths(directory, count) @promise = new Promise (resolve, reject) => @task.on('task:cancelled', reject) - @task.start([rootPath], regex.source, scanHandlerOptions, resolve) + @task.start([directory.getPath()], regex.source, scanHandlerOptions, resolve) # Public: Implementation of `then()` to satisfy the *thenable* contract. # This makes it possible to use a `DirectorySearch` with `Promise.all()`. @@ -66,6 +64,7 @@ class DefaultDirectorySearcher # * `range` {Range} Identifies the matching region in the file. (Likely as an array of numeric arrays.) # * `didError` {Function} call with an Error if there is a problem during the search. # * `didSearchPaths` {Function} periodically call with the number of paths searched thus far. + # This takes two arguments: the `Directory` and the count. # * `inclusions` {Array} of glob patterns (as strings) to search within. Note that this # array may be empty, indicating that all files should be searched. # @@ -80,15 +79,16 @@ class DefaultDirectorySearcher # Returns a *thenable* `DirectorySearch` that includes a `cancel()` method. If `cancel()` is # invoked before the `DirectorySearch` is determined, it will reject the `DirectorySearch`. search: (directories, regex, options) -> - rootPaths = directories.map (directory) -> directory.getPath() + # Make a mutable copy of the directories array. + directories = directories.slice(0) isCancelled = false promise = new Promise (resolve, reject) -> run = -> if isCancelled reject() - else if rootPaths.length - rootPath = rootPaths.shift() - thenable = new DirectorySearch(rootPath, regex, options) + else if directories.length + directory = directories.shift() + thenable = new DirectorySearch(directory, regex, options) thenable.then(run, reject) else resolve() diff --git a/src/workspace.coffee b/src/workspace.coffee index a2d3059de..638fa8a0a 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -811,15 +811,20 @@ class Workspace extends Model iterator = options options = {} - # Find a searcher for every Directory in the project. - searchersAndDirectories = [] + # Find a searcher for every Directory in the project. Each searcher that is matched + # will be associated with an Array of Directory objects in the Map. + directoriesForSearcher = new Map() for directory in atom.project.getDirectories() searcher = @defaultDirectorySearcher for directorySearcher in @directorySearchers if directorySearcher.canSearchDirectory(directory) searcher = directorySearcher break - searchersAndDirectories.push({searcher, directory}) + directories = directoriesForSearcher.get(searcher) + unless directories + directories = [] + directoriesForSearcher.set(searcher, directories) + directories.push(directory) # Define the onPathsSearched callback. if _.isFunction(options.onPathsSearched) @@ -838,22 +843,23 @@ class Workspace extends Model else onPathsSearched = -> + # Build up the options object that will be shared by all searchers. + searchOptions = + inclusions: options.paths or [] + includeHidden: true + excludeVcsIgnores: atom.config.get('core.excludeVcsIgnoredPaths') + exclusions: atom.config.get('core.ignoredNames') + follow: atom.config.get('core.followSymlinks') + didMatch: (result) -> + iterator(result) unless atom.project.isPathModified(result.filePath) + didError: (error) -> + iterator(null, error) + didSearchPaths: onPathsSearched + # Kick off all of the searches and unify them into one Promise. allSearches = [] - for entry in searchersAndDirectories - {searcher, directory} = entry - searchOptions = - inclusions: options.paths or [] - includeHidden: true - excludeVcsIgnores: atom.config.get('core.excludeVcsIgnoredPaths') - exclusions: atom.config.get('core.ignoredNames') - follow: atom.config.get('core.followSymlinks') - didMatch: (result) -> - iterator(result) unless atom.project.isPathModified(result.filePath) - didError: (error) -> - iterator(null, error) - didSearchPaths: onPathsSearched.bind(undefined, directory) - directorySearcher = searcher.search([directory], regex, searchOptions) + directoriesForSearcher.forEach (directories, searcher) -> + directorySearcher = searcher.search(directories, regex, searchOptions) allSearches.push(directorySearcher) searchPromise = Promise.all(allSearches) From 028ac79836ac4e326230bc89f76aa4af3c0e3a01 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 3 Jun 2015 22:41:58 -0400 Subject: [PATCH 16/21] Changed the behavior so that if a searcher rejects, then the thenable returned by `atom.workspace.scan()` rejects. --- spec/workspace-spec.coffee | 6 +++--- src/workspace.coffee | 9 ++++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index bfada8b43..7e7ee406a 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -1014,12 +1014,12 @@ describe "Workspace", -> cancelableSearch = atom.workspace.scan /aaaa/, -> fakeSearch.hoistedReject() - resultOfPromiseSearch = null + didReject = false waitsForPromise -> - cancelableSearch.then (promiseResult) -> resultOfPromiseSearch = promiseResult + cancelableSearch.catch -> didReject = true runs -> - expect(resultOfPromiseSearch).toBe('cancelled') + expect(didReject).toBe(true) describe "::replace(regex, replacementText, paths, iterator)", -> [filePath, commentFilePath, sampleContent, sampleCommentContent] = [] diff --git a/src/workspace.coffee b/src/workspace.coffee index 638fa8a0a..924ad3bc5 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -874,15 +874,18 @@ class Workspace extends Model # with the existing behavior, instead of cancel() rejecting the promise, it should # resolve it with the special value 'cancelled'. At least the built-in find-and-replace # package relies on this behavior. + isCancelled = false cancellablePromise = new Promise (resolve, reject) -> onSuccess = -> resolve(null) - return onFailure = -> - resolve('cancelled') - return + if isCancelled + resolve('cancelled') + else + reject() searchPromise.then(onSuccess, onFailure) cancellablePromise.cancel = -> + isCancelled = true # Note that cancelling all (or actually, any) of the members of allSearches # will cause searchPromise to reject, which will cause cancellablePromise to resolve # in the desired way. From 18ac7d0cbcbafd0ffabb019839c8e0dff3d8b3d5 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 3 Jun 2015 23:01:02 -0400 Subject: [PATCH 17/21] Comment excised, as recommended by @maxbrunsfeld. --- src/workspace.coffee | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/workspace.coffee b/src/workspace.coffee index 924ad3bc5..61c16a5c1 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -52,8 +52,6 @@ class Workspace extends Model atom.packages.serviceHub.consume( 'atom.directory-searcher', '^0.1.0', - # New providers are added to the front of @directorySearchers because - # DefaultDirectorySearcher is a catch-all that will always claim to search a Directory. (provider) => @directorySearchers.unshift(provider)) @panelContainers = From 0630bce95c6dadc6f212f4129528043ecef47edd Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 3 Jun 2015 23:29:56 -0400 Subject: [PATCH 18/21] Two things: * Removed the `Directory` argument to `didSearchPaths`. Now each searcher gets its own instance of `didSearchPaths` that is parameterized by provider. * Simplified `DefaultDirectorySearcher.search()` so it creates one `DirectorySearch` rather than one per `Directory` passed to `search()`. --- spec/workspace-spec.coffee | 7 ++++--- src/default-directory-searcher.coffee | 26 ++++++++++------------- src/workspace.coffee | 30 +++++++++++++-------------- 3 files changed, 29 insertions(+), 34 deletions(-) diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index 7e7ee406a..4666177c6 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -952,7 +952,9 @@ describe "Workspace", -> onFakeSearchCreated?(this) then: (args...) -> @promise.then.apply(@promise, args) - cancel: -> @cancelled = true + cancel: -> + @cancelled = true + @hoistedReject() beforeEach -> fakeSearch = null @@ -978,8 +980,7 @@ describe "Workspace", -> ] onFakeSearchCreated = (fakeSearch) -> fakeSearch.options.didMatch(searchResult) - directory1 = atom.project.getDirectories()[atom.project.getPaths().indexOf(dir1)] - fakeSearch.options.didSearchPaths(directory1, numPathsToPretendToSearchInCustomDirectorySearcher) + fakeSearch.options.didSearchPaths(numPathsToPretendToSearchInCustomDirectorySearcher) fakeSearch.hoistedResolve() resultPaths = [] diff --git a/src/default-directory-searcher.coffee b/src/default-directory-searcher.coffee index 4767a0313..a5f113eeb 100644 --- a/src/default-directory-searcher.coffee +++ b/src/default-directory-searcher.coffee @@ -4,7 +4,7 @@ Task = require './task' # # Implements thenable so it can be used with `Promise.all()`. class DirectorySearch - constructor: (directory, regex, options) -> + constructor: (rootPaths, regex, options) -> scanHandlerOptions = ignoreCase: regex.ignoreCase inclusions: options.inclusions @@ -15,10 +15,10 @@ class DirectorySearch @task = new Task(require.resolve('./scan-handler')) @task.on 'scan:result-found', options.didMatch @task.on 'scan:file-error', options.didError - @task.on 'scan:paths-searched', (count) -> options.didSearchPaths(directory, count) + @task.on 'scan:paths-searched', options.didSearchPaths @promise = new Promise (resolve, reject) => @task.on('task:cancelled', reject) - @task.start([directory.getPath()], regex.source, scanHandlerOptions, resolve) + @task.start(rootPaths, regex.source, scanHandlerOptions, resolve) # Public: Implementation of `then()` to satisfy the *thenable* contract. # This makes it possible to use a `DirectorySearch` with `Promise.all()`. @@ -64,7 +64,6 @@ class DefaultDirectorySearcher # * `range` {Range} Identifies the matching region in the file. (Likely as an array of numeric arrays.) # * `didError` {Function} call with an Error if there is a problem during the search. # * `didSearchPaths` {Function} periodically call with the number of paths searched thus far. - # This takes two arguments: the `Directory` and the count. # * `inclusions` {Array} of glob patterns (as strings) to search within. Note that this # array may be empty, indicating that all files should be searched. # @@ -79,21 +78,18 @@ class DefaultDirectorySearcher # Returns a *thenable* `DirectorySearch` that includes a `cancel()` method. If `cancel()` is # invoked before the `DirectorySearch` is determined, it will reject the `DirectorySearch`. search: (directories, regex, options) -> - # Make a mutable copy of the directories array. - directories = directories.slice(0) + rootPaths = directories.map (directory) -> directory.getPath() isCancelled = false + directorySearch = new DirectorySearch(rootPaths, regex, options) promise = new Promise (resolve, reject) -> - run = -> + directorySearch.then resolve, -> if isCancelled - reject() - else if directories.length - directory = directories.shift() - thenable = new DirectorySearch(directory, regex, options) - thenable.then(run, reject) - else resolve() - run() + else + reject() return { then: promise.then.bind(promise) - cancel: -> isCancelled = true + cancel: -> + isCancelled = true + directorySearch.cancel() } diff --git a/src/workspace.coffee b/src/workspace.coffee index 61c16a5c1..23cf25620 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -831,32 +831,30 @@ class Workspace extends Model onPathsSearchedOption = options.onPathsSearched totalNumberOfPathsSearched = 0 numberOfPathsSearchedForDirectory = new Map() - onPathsSearched = (directory, numberOfPathsSearched) -> - oldValue = numberOfPathsSearchedForDirectory.get(directory) + onPathsSearched = (searcher, numberOfPathsSearched) -> + oldValue = numberOfPathsSearchedForDirectory.get(searcher) if oldValue totalNumberOfPathsSearched -= oldValue - numberOfPathsSearchedForDirectory.set(directory, numberOfPathsSearched) + numberOfPathsSearchedForDirectory.set(searcher, numberOfPathsSearched) totalNumberOfPathsSearched += numberOfPathsSearched onPathsSearchedOption(totalNumberOfPathsSearched) else onPathsSearched = -> - # Build up the options object that will be shared by all searchers. - searchOptions = - inclusions: options.paths or [] - includeHidden: true - excludeVcsIgnores: atom.config.get('core.excludeVcsIgnoredPaths') - exclusions: atom.config.get('core.ignoredNames') - follow: atom.config.get('core.followSymlinks') - didMatch: (result) -> - iterator(result) unless atom.project.isPathModified(result.filePath) - didError: (error) -> - iterator(null, error) - didSearchPaths: onPathsSearched - # Kick off all of the searches and unify them into one Promise. allSearches = [] directoriesForSearcher.forEach (directories, searcher) -> + searchOptions = + inclusions: options.paths or [] + includeHidden: true + excludeVcsIgnores: atom.config.get('core.excludeVcsIgnoredPaths') + exclusions: atom.config.get('core.ignoredNames') + follow: atom.config.get('core.followSymlinks') + didMatch: (result) -> + iterator(result) unless atom.project.isPathModified(result.filePath) + didError: (error) -> + iterator(null, error) + didSearchPaths: (count) -> onPathsSearched(searcher, count) directorySearcher = searcher.search(directories, regex, searchOptions) allSearches.push(directorySearcher) searchPromise = Promise.all(allSearches) From 6b1b57c89cd43b21487b5d8d0fd130797395ec04 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 3 Jun 2015 23:33:10 -0400 Subject: [PATCH 19/21] Rename a variable to reflect a change in the previous commit. --- src/workspace.coffee | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/workspace.coffee b/src/workspace.coffee index 23cf25620..7a0787dcd 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -830,12 +830,12 @@ class Workspace extends Model # replace the entry in the map and update the total. onPathsSearchedOption = options.onPathsSearched totalNumberOfPathsSearched = 0 - numberOfPathsSearchedForDirectory = new Map() + numberOfPathsSearchedForSearcher = new Map() onPathsSearched = (searcher, numberOfPathsSearched) -> - oldValue = numberOfPathsSearchedForDirectory.get(searcher) + oldValue = numberOfPathsSearchedForSearcher.get(searcher) if oldValue totalNumberOfPathsSearched -= oldValue - numberOfPathsSearchedForDirectory.set(searcher, numberOfPathsSearched) + numberOfPathsSearchedForSearcher.set(searcher, numberOfPathsSearched) totalNumberOfPathsSearched += numberOfPathsSearched onPathsSearchedOption(totalNumberOfPathsSearched) else From 1a6c542b45a8a0868c94629acedd6277fe84a594 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 3 Jun 2015 23:38:52 -0400 Subject: [PATCH 20/21] Update comment. --- src/default-directory-searcher.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/default-directory-searcher.coffee b/src/default-directory-searcher.coffee index a5f113eeb..ebe3a35f9 100644 --- a/src/default-directory-searcher.coffee +++ b/src/default-directory-searcher.coffee @@ -76,7 +76,7 @@ class DefaultDirectorySearcher # * `follow` {boolean} whether symlinks should be followed. # # Returns a *thenable* `DirectorySearch` that includes a `cancel()` method. If `cancel()` is - # invoked before the `DirectorySearch` is determined, it will reject the `DirectorySearch`. + # invoked before the `DirectorySearch` is determined, it will resolve the `DirectorySearch`. search: (directories, regex, options) -> rootPaths = directories.map (directory) -> directory.getPath() isCancelled = false From 535c0e2f3441bfd3bad81b1d38f57626aef9456b Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 4 Jun 2015 20:15:49 -0400 Subject: [PATCH 21/21] Fix documentation bug. --- src/workspace.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/workspace.coffee b/src/workspace.coffee index 7a0787dcd..6311cdd6d 100644 --- a/src/workspace.coffee +++ b/src/workspace.coffee @@ -802,7 +802,7 @@ class Workspace extends Model # * `onPathsSearched` (optional) {Function} # * `iterator` {Function} callback on each file found # - # Returns a *thenable* object with a `cancel()` method that will cancel all + # Returns a `Promise` with a `cancel()` method that will cancel all # of the underlying searches that were started as part of this scan. scan: (regex, options={}, iterator) -> if _.isFunction(options)