Merge pull request #12694 from atom/mb-ns-avoid-adding-non-existent-project-directories

Avoid adding non-existent project directories

package.json

@@ -54,7 +54,7 @@
     "scrollbar-style": "^3.2",
     "season": "^5.3",
     "semver": "^4.3.3",
-    "service-hub": "^0.7.0",
+    "service-hub": "^0.7.1-0",
     "sinon": "1.17.4",
     "source-map-support": "^0.3.2",
     "temp": "0.8.1",

spec/async-spec-helpers.js

@@ -29,23 +29,20 @@ export function afterEach (fn) {
   }
 })
 
-export function conditionPromise (condition)  {
-  const timeoutError = new Error("Timed out waiting on condition")
-  Error.captureStackTrace(timeoutError, conditionPromise)
+export async function conditionPromise (condition)  {
+  const startTime = Date.now()
 
-  return new Promise(function (resolve, reject) {
-    const interval = global.setInterval(function () {
-      if (condition()) {
-        global.clearInterval(interval)
-        global.clearTimeout(timeout)
-        resolve()
-      }
-    }, 100)
-    const timeout = global.setTimeout(function () {
-      global.clearInterval(interval)
-      reject(timeoutError)
-    }, 5000)
-  })
+  while (true) {
+    await timeoutPromise(100)
+
+    if (await condition()) {
+      return
+    }
+
+    if (Date.now() - startTime > 5000) {
+      throw new Error("Timed out waiting on condition")
+    }
+  }
 }
 
 export function timeoutPromise (timeout) {

spec/atom-environment-spec.coffee

@@ -392,8 +392,9 @@ describe "AtomEnvironment", ->
     describe "when the opened path is a uri", ->
       it "adds it to the project's paths as is", ->
         pathToOpen = 'remote://server:7644/some/dir/path'
+        spyOn(atom.project, 'addPath')
         atom.openLocations([{pathToOpen}])
-        expect(atom.project.getPaths()[0]).toBe pathToOpen
+        expect(atom.project.addPath).toHaveBeenCalledWith(pathToOpen)
 
   describe "::updateAvailable(info) (called via IPC from browser process)", ->
     subscription = null

spec/fixtures/packages/package-with-directory-provider/index.js

@@ -0,0 +1,47 @@
+'use strict'
+
+class FakeRemoteDirectory {
+  constructor (uri) {
+    this.uri = uri
+  }
+
+  relativize (uri) {
+    return uri
+  }
+
+  getPath () {
+    return this.uri
+  }
+
+  isRoot () {
+    return true
+  }
+
+  getSubdirectory () {
+    return {
+      existsSync () {
+        return false
+      }
+    }
+  }
+
+  existsSync () {
+    return true
+  }
+
+  contains () {
+    return false
+  }
+}
+
+exports.provideDirectoryProvider = function () {
+  return {
+    name: 'directory provider from package-with-directory-provider',
+
+    directoryForURISync (uri) {
+      if (uri.startsWith('remote://')) {
+        return new FakeRemoteDirectory(uri)
+      }
+    }
+  }
+}

spec/fixtures/packages/package-with-directory-provider/package.json

@@ -0,0 +1,12 @@
+{
+  "name": "package-with-directory-provider",
+
+  "providedServices": {
+    "atom.directory-provider": {
+      "description": "Provides custom Directory instances",
+      "versions": {
+        "0.1.1": "provideDirectoryProvider"
+      }
+    }
+  }
+}

spec/main-process/atom-application.test.js

@@ -188,6 +188,8 @@ describe('AtomApplication', function () {
       reusedWindow = atomApplication.launch(parseCommandLine([dirBPath, '-a']))
       assert.equal(reusedWindow, window1)
       assert.deepEqual(atomApplication.windows, [window1])
+
+      await conditionPromise(async () => (await getTreeViewRootDirectories(reusedWindow)).length === 3)
       assert.deepEqual(await getTreeViewRootDirectories(window1), [dirAPath, dirCPath, dirBPath])
     })
 
@@ -246,7 +248,7 @@ describe('AtomApplication', function () {
 
       const reusedWindow = atomApplication.launch(parseCommandLine([tempDirPath]))
       assert.equal(reusedWindow, window1)
-      assert.deepEqual(await getTreeViewRootDirectories(window1), [tempDirPath])
+      await conditionPromise(async () => (await getTreeViewRootDirectories(reusedWindow)).length > 0)
     })
 
     it('opens a new window with a single untitled buffer when launched with no path, even if windows already exist', async function () {
@@ -303,33 +305,33 @@ describe('AtomApplication', function () {
       assert.deepEqual(await getTreeViewRootDirectories(window), [path.dirname(newFilePath)])
     })
 
-    it('opens an empty text editor and loads its parent directory in the tree-view when launched with a new file path in a remote directory', async function () {
-      // Disable the tree-view because it will try to enumerate the contents of
-      // the remote directory and, since it doesn't exist, throw an error.
-      const configPath = path.join(process.env.ATOM_HOME, 'config.cson')
-      const config = season.readFileSync(configPath)
-      if (!config['*'].core) config['*'].core = {}
-      config['*'].core.disabledPackages = ['tree-view']
-      season.writeFileSync(configPath, config)
+    it('adds a remote directory to the project when launched with a remote directory', async function () {
+      const packagePath = path.join(__dirname, '..', 'fixtures', 'packages', 'package-with-directory-provider')
+      const packagesDirPath = path.join(process.env.ATOM_HOME, 'packages')
+      fs.mkdirSync(packagesDirPath)
+      fs.symlinkSync(packagePath, path.join(packagesDirPath, 'package-with-directory-provider'))
 
       const atomApplication = buildAtomApplication()
-      const newRemoteFilePath = 'remote://server:3437/some/directory/path'
-      const window = atomApplication.launch(parseCommandLine([newRemoteFilePath]))
+      atomApplication.config.set('core.disabledPackages', ['fuzzy-finder'])
+
+      const remotePath = 'remote://server:3437/some/directory/path'
+      let window = atomApplication.launch(parseCommandLine([remotePath]))
+
       await focusWindow(window)
-      const {projectPaths, editorTitle, editorText} = await evalInWebContents(window.browserWindow.webContents, function (sendBackToMainProcess) {
-        atom.workspace.observeActivePaneItem(function (editor) {
-          if (editor) {
-            sendBackToMainProcess({
-              projectPaths: atom.project.getPaths(),
-              editorTitle: editor.getTitle(),
-              editorText: editor.getText()
-            })
-          }
+      await conditionPromise(async () => (await getProjectDirectories()).length > 0)
+      let directories = await getProjectDirectories()
+      assert.deepEqual(directories, [{type: 'FakeRemoteDirectory', path: remotePath}])
+
+      await window.reload()
+      await focusWindow(window)
+      directories = await getProjectDirectories()
+      assert.deepEqual(directories, [{type: 'FakeRemoteDirectory', path: remotePath}])
+
+      function getProjectDirectories () {
+        return evalInWebContents(window.browserWindow.webContents, function (sendBackToMainProcess) {
+          sendBackToMainProcess(atom.project.getDirectories().map(d => ({ type: d.constructor.name, path: d.getPath() })))
         })
-      })
-      assert.deepEqual(projectPaths, [newRemoteFilePath])
-      assert.equal(editorTitle, path.basename(newRemoteFilePath))
-      assert.equal(editorText, '')
+      }
     })
 
     it('reopens any previously opened windows when launched with no path', async function () {
@@ -342,6 +344,9 @@ describe('AtomApplication', function () {
       const app1Window2 = atomApplication1.launch(parseCommandLine([tempDirPath2]))
       await app1Window2.loadedPromise
 
+      await app1Window1.saveState()
+      await app1Window2.saveState()
+
       const atomApplication2 = buildAtomApplication()
       const [app2Window1, app2Window2] = atomApplication2.launch(parseCommandLine([]))
       await app2Window1.loadedPromise

spec/package-manager-spec.coffee

@@ -120,6 +120,16 @@ describe "PackageManager", ->
         state: state2
       }
 
+    it "early-activates any atom.directory-provider or atom.repository-provider services that the package provide", ->
+      jasmine.useRealClock()
+
+      providers = []
+      atom.packages.serviceHub.consume 'atom.directory-provider', '^0.1.0', (provider) ->
+        providers.push(provider)
+
+      atom.packages.loadPackage('package-with-directory-provider')
+      expect(providers.map((p) -> p.name)).toEqual(['directory provider from package-with-directory-provider'])
+
     describe "when there are view providers specified in the package's package.json", ->
       model1 = {worksWithViewProvider1: true}
       model2 = {worksWithViewProvider2: true}

spec/project-spec.coffee

@@ -25,7 +25,6 @@ describe "Project", ->
       deserializedProject = new Project({notificationManager: atom.notifications, packageManager: atom.packages, confirm: atom.confirm})
       state = atom.project.serialize()
       state.paths.push('/directory/that/does/not/exist')
-      state.paths.push(path.join(__dirname, 'fixtures', 'sample.js'))
       deserializedProject.deserialize(state, atom.deserializers)
       expect(deserializedProject.getPaths()).toEqual(atom.project.getPaths())
 
@@ -231,31 +230,22 @@ describe "Project", ->
       expect(directories[1].getPath()).toBe remotePath
       expect(directories[1] instanceof DummyDirectory).toBe true
 
-      # It does not add new remote paths if their directories do not exist
-      # and they are contained by existing remote paths.
-      childRemotePath = remotePath + "/subdirectory/that/does-not-exist"
-      atom.project.addPath(childRemotePath)
+      # It does not add new remote paths that do not exist
+      nonExistentRemotePath = "ssh://another-directory:8080/does-not-exist"
+      atom.project.addPath(nonExistentRemotePath)
       expect(atom.project.getDirectories().length).toBe 2
 
-      # It does add new remote paths if their directories exist.
-      childRemotePath = remotePath + "/subdirectory/that/does-exist"
-      atom.project.addPath(childRemotePath)
+      # It adds new remote paths if their directories exist.
+      newRemotePath = "ssh://another-directory:8080/does-exist"
+      atom.project.addPath(newRemotePath)
       directories = atom.project.getDirectories()
-      expect(directories[2].getPath()).toBe childRemotePath
+      expect(directories[2].getPath()).toBe newRemotePath
       expect(directories[2] instanceof DummyDirectory).toBe true
 
-      # It does add new remote paths to be added if they are not contained by
-      # previous remote paths.
-      otherRemotePath = "ssh://other-foreign-directory:8080/"
-      atom.project.addPath(otherRemotePath)
-      directories = atom.project.getDirectories()
-      expect(directories[3].getPath()).toBe otherRemotePath
-      expect(directories[3] instanceof DummyDirectory).toBe true
-
     it "stops using the provider when the service is removed", ->
       serviceDisposable.dispose()
       atom.project.setPaths(["ssh://foreign-directory:8080/does-exist"])
-      expect(atom.project.getDirectories()[0] instanceof Directory).toBe true
+      expect(atom.project.getDirectories().length).toBe(0)
 
   describe ".open(path)", ->
     [absolutePath, newBufferHandler] = []
@@ -429,13 +419,6 @@ describe "Project", ->
       expect(atom.project.getPaths()[0]).toEqual path.dirname(require.resolve('./fixtures/dir/a'))
       expect(atom.project.getDirectories()[0].path).toEqual path.dirname(require.resolve('./fixtures/dir/a'))
 
-    it "only normalizes the directory path if it isn't on the local filesystem", ->
-      nonLocalFsDirectory = "custom_proto://abc/def"
-      atom.project.setPaths([nonLocalFsDirectory])
-      directories = atom.project.getDirectories()
-      expect(directories.length).toBe 1
-      expect(directories[0].getPath()).toBe path.normalize(nonLocalFsDirectory)
-
   describe ".addPath(path)", ->
     it "calls callbacks registered with ::onDidChangePaths", ->
       onDidChangePathsSpy = jasmine.createSpy('onDidChangePaths spy')
@@ -470,6 +453,11 @@ describe "Project", ->
       expect(atom.project.getPaths()).toEqual([oldPath, newPath])
       expect(onDidChangePathsSpy).toHaveBeenCalled()
 
+    it "doesn't add non-existent directories", ->
+      previousPaths = atom.project.getPaths()
+      atom.project.addPath('/this-definitely/does-not-exist')
+      expect(atom.project.getPaths()).toEqual(previousPaths)
+
   describe ".removePath(path)", ->
     onDidChangePathsSpy = null
 

spec/window-event-handler-spec.coffee

@@ -53,6 +53,7 @@ describe "WindowEventHandler", ->
   describe "beforeunload event", ->
     beforeEach ->
       jasmine.unspy(TextEditor.prototype, "shouldPromptToSave")
+      spyOn(atom, 'destroy')
       spyOn(ipcRenderer, 'send')
 
     describe "when pane items are modified", ->
@@ -66,12 +67,14 @@ describe "WindowEventHandler", ->
         window.dispatchEvent(new CustomEvent('beforeunload'))
         expect(atom.workspace.confirmClose).toHaveBeenCalled()
         expect(ipcRenderer.send).not.toHaveBeenCalledWith('did-cancel-window-unload')
+        expect(atom.destroy).toHaveBeenCalled()
 
       it "cancels the unload if the user selects cancel", ->
         spyOn(atom.workspace, 'confirmClose').andReturn(false)
         window.dispatchEvent(new CustomEvent('beforeunload'))
         expect(atom.workspace.confirmClose).toHaveBeenCalled()
         expect(ipcRenderer.send).toHaveBeenCalledWith('did-cancel-window-unload')
+        expect(atom.destroy).not.toHaveBeenCalled()
 
   describe "when a link is clicked", ->
     it "opens the http/https links in an external application", ->

src/main-process/atom-window.coffee

@@ -74,8 +74,7 @@ class AtomWindow
 
     @browserWindow.loadSettings = loadSettings
 
-    @browserWindow.once 'window:loaded', =>
-      @loaded = true
+    @browserWindow.on 'window:loaded', =>
       @emit 'window:loaded'
       @resolveLoadedPromise()
 
@@ -194,10 +193,7 @@ class AtomWindow
     @openLocations([{pathToOpen, initialLine, initialColumn}])
 
   openLocations: (locationsToOpen) ->
-    if @loaded
-      @sendMessage 'open-locations', locationsToOpen
-    else
-      @browserWindow.once 'window:loaded', => @openLocations(locationsToOpen)
+    @loadedPromise.then => @sendMessage 'open-locations', locationsToOpen
 
   replaceEnvironment: (env) ->
     @browserWindow.webContents.send 'environment', env
@@ -255,6 +251,9 @@ class AtomWindow
 
   isSpecWindow: -> @isSpec
 
-  reload: -> @browserWindow.reload()
+  reload: ->
+    @loadedPromise = new Promise((@resolveLoadedPromise) =>)
+    @saveState().then => @browserWindow.reload()
+    @loadedPromise
 
   toggleDevTools: -> @browserWindow.toggleDevTools()

src/package.coffee

@@ -85,6 +85,7 @@ class Package
         @loadMenus()
         @loadStylesheets()
         @registerDeserializerMethods()
+        @activateCoreStartupServices()
         @configSchemaRegisteredOnLoad = @registerConfigSchemaFromMetadata()
         @settingsPromise = @loadSettings()
         if @shouldRequireMainModuleOnLoad() and not @mainModule?
@@ -290,6 +291,15 @@ class Package
             @mainModule[methodName](state, atomEnvironment)
       return
 
+  activateCoreStartupServices: ->
+    if directoryProviderService = @metadata.providedServices?['atom.directory-provider']
+      @requireMainModule()
+      servicesByVersion = {}
+      for version, methodName of directoryProviderService.versions
+        if typeof @mainModule[methodName] is 'function'
+          servicesByVersion[version] = @mainModule[methodName]()
+      @packageManager.serviceHub.provide('atom.directory-provider', servicesByVersion)
+
   registerViewProviders: ->
     if @metadata.viewProviders? and not @registeredViewProviders
       @requireMainModule()

src/project.coffee

@@ -56,7 +56,6 @@ class Project extends Model
 
   deserialize: (state) ->
     state.paths = [state.path] if state.path? # backward compatibility
-    state.paths = state.paths.filter (directoryPath) -> fs.isDirectorySync(directoryPath)
 
     @buffers = _.compact state.buffers.map (bufferState) ->
       # Check that buffer's file path is accessible
@@ -181,10 +180,9 @@ class Project extends Model
       break if directory = provider.directoryForURISync?(projectPath)
     directory ?= @defaultDirectoryProvider.directoryForURISync(projectPath)
 
-    directoryExists = directory.existsSync()
-    for rootDirectory in @getDirectories()
-      return if rootDirectory.getPath() is directory.getPath()
-      return if not directoryExists and rootDirectory.contains(directory.getPath())
+    return unless directory.existsSync()
+    for existingDirectory in @getDirectories()
+      return if existingDirectory.getPath() is directory.getPath()
 
     @rootDirectories.push(directory)
 

src/window-event-handler.coffee

@@ -10,9 +10,7 @@ class WindowEventHandler
     @reloadRequested = false
     @subscriptions = new CompositeDisposable
 
-    @previousOnbeforeunloadHandler = @window.onbeforeunload
-    @window.onbeforeunload = @handleWindowBeforeunload
-    @addEventListener(@window, 'unload', @handleWindowUnload)
+    @addEventListener(@window, 'beforeunload', @handleWindowBeforeunload)
     @addEventListener(@window, 'focus', @handleWindowFocus)
     @addEventListener(@window, 'blur', @handleWindowBlur)
 
@@ -64,7 +62,6 @@ class WindowEventHandler
     bindCommandToAction('core:cut', 'cut')
 
   unsubscribe: ->
-    @window.onbeforeunload = @previousOnbeforeunloadHandler
     @subscriptions.dispose()
 
   on: (target, eventName, handler) ->
@@ -152,7 +149,7 @@ class WindowEventHandler
   handleLeaveFullScreen: =>
     @document.body.classList.remove("fullscreen")
 
-  handleWindowBeforeunload: =>
+  handleWindowBeforeunload: (event) =>
     confirmed = @atomEnvironment.workspace?.confirmClose(windowCloseRequested: true)
     if confirmed and not @reloadRequested and not @atomEnvironment.inSpecMode() and @atomEnvironment.getCurrentWindow().isWebViewFocused()
       @atomEnvironment.hide()
@@ -161,14 +158,10 @@ class WindowEventHandler
     @atomEnvironment.storeWindowDimensions()
     if confirmed
       @atomEnvironment.unloadEditorWindow()
+      @atomEnvironment.destroy()
     else
       @applicationDelegate.didCancelWindowUnload()
-
-    # Returning any non-void value stops the window from unloading
-    return true unless confirmed
-
-  handleWindowUnload: =>
-    @atomEnvironment.destroy()
+      event.returnValue = false
 
   handleWindowToggleFullScreen: =>
     @atomEnvironment.toggleFullScreen()