Merge pull request #989 from atom/cj-text-buffer-cleanup

Text Buffer cleanup

spec/file-spec.coffee

@@ -25,7 +25,7 @@ describe 'File', ->
 
   describe "when the file has already been read", ->
     beforeEach ->
-      file.read()
+      file.readSync()
 
     describe "when the contents of the file change", ->
       it "triggers 'contents-changed' event handlers", ->

spec/git-spec.coffee

@@ -253,11 +253,16 @@ describe "Git", ->
       project.openSync('sample.js')
       project2 = deserialize(project.serialize())
       buffer = project2.getBuffers()[0]
-      originalContent = buffer.getText()
-      buffer.append('changes')
 
-      statusHandler = jasmine.createSpy('statusHandler')
-      project2.getRepo().on 'status-changed', statusHandler
-      buffer.save()
-      expect(statusHandler.callCount).toBe 1
-      expect(statusHandler).toHaveBeenCalledWith buffer.getPath(), 256
+      waitsFor ->
+        buffer.loaded
+
+      runs ->
+        originalContent = buffer.getText()
+        buffer.append('changes')
+
+        statusHandler = jasmine.createSpy('statusHandler')
+        project2.getRepo().on 'status-changed', statusHandler
+        buffer.save()
+        expect(statusHandler.callCount).toBe 1
+        expect(statusHandler).toHaveBeenCalledWith buffer.getPath(), 256

spec/text-buffer-replication-spec.coffee

@@ -11,6 +11,9 @@ describe "TextBuffer replication", ->
     doc1.connect(doc2)
     buffer2 = deserialize(doc2, {project})
 
+    waitsFor ->
+      buffer1.loaded and buffer2.loaded
+
   afterEach ->
     buffer1.destroy()
     buffer2.destroy()

spec/text-buffer-spec.coffee

@@ -2,6 +2,7 @@
 path = require 'path'
 temp = require 'temp'
 {Site} = require 'telepath'
+TextBuffer = require '../src/text-buffer'
 
 describe 'TextBuffer', ->
   [filePath, fileContents, buffer] = []
@@ -134,12 +135,12 @@ describe 'TextBuffer', ->
           expect(buffer.isModified()).toBeTruthy()
 
       it "fires a single contents-conflicted event", ->
-        buffer.insert([0, 0], "a change")
+        buffer.setText("a change")
         buffer.save()
         buffer.insert([0, 0], "a second change")
 
         handler = jasmine.createSpy('fileChange')
-        fs.writeSync(filePath, "second")
+        fs.writeSync(filePath, "a disk change")
         buffer.on 'contents-conflicted', handler
 
         expect(handler.callCount).toBe 0
@@ -308,6 +309,19 @@ describe 'TextBuffer', ->
       buffer.setText('\n')
       expect(buffer.isModified()).toBeTruthy()
 
+    it "returns false until the buffer is fully loaded", ->
+      buffer.release()
+      filePath = temp.openSync('atom').path
+      buffer = new TextBuffer({project, filePath})
+
+      expect(buffer.isModified()).toBeFalsy()
+
+      waitsForPromise ->
+        buffer.load()
+
+      runs ->
+        expect(buffer.isModified()).toBeFalsy()
+
   describe ".getLines()", ->
     it "returns an array of lines in the text contents", ->
       expect(buffer.getLines().length).toBe fileContents.split("\n").length

src/file.coffee

@@ -1,9 +1,9 @@
+path = require 'path'
+pathWatcher = require 'pathwatcher'
 Q = require 'q'
 {Emitter} = require 'emissary'
-path = require 'path'
-fsUtils = require './fs-utils'
-pathWatcher = require 'pathwatcher'
 _ = require 'underscore-plus'
+fsUtils = require './fs-utils'
 
 # Public: Represents an individual file in the editor.
 #
@@ -57,7 +57,7 @@ class File
     @subscribeToNativeChangeEvents() if not previouslyExisted and @subscriptionCount() > 0
 
   # Private: Deprecated
-  read: (flushCache) ->
+  readSync: (flushCache) ->
     if not @exists()
       @cachedContents = null
     else if not @cachedContents? or flushCache
@@ -72,13 +72,15 @@ class File
   #   copy is acceptable.
   #
   # Returns a promise that resovles to a String.
-  readAsync: (flushCache) ->
+  read: (flushCache) ->
     if not @exists()
       promise = Q(null)
     else if not @cachedContents? or flushCache
+      if fsUtils.statSyncNoException(@getPath()).size >= 1048576 # 1MB
+        throw new Error("Atom can only handle files < 1MB, for now.")
+
       deferred = Q.defer()
       promise = deferred.promise
-
       content = []
       bytesRead = 0
       readStream = fsUtils.createReadStream @getPath(), encoding: 'utf8'
@@ -95,7 +97,7 @@ class File
     else
       promise = Q(@cachedContents)
 
-    promise.then (contents) ->
+    promise.then (contents) =>
       @cachedContents = contents
 
   # Public: Returns whether a file exists.
@@ -112,9 +114,8 @@ class File
       @emit "moved"
     else if eventType is "change"
       oldContents = @cachedContents
-      newContents = @read(true)
-      return if oldContents == newContents
-      @emit 'contents-changed'
+      @read(true).done (newContents) =>
+        @emit 'contents-changed' unless oldContents == newContents
 
   # Private:
   detectResurrectionAfterDelay: ->

src/text-buffer.coffee

@@ -59,6 +59,7 @@ class TextBuffer
         text: @text
       @loadFromDisk = not initialText
 
+    @loaded = false
     @subscribe @text, 'changed', @handleTextChange
     @subscribe @text, 'marker-created', (marker) => @emit 'marker-created', marker
     @subscribe @text, 'markers-updated', => @emit 'markers-updated'
@@ -66,13 +67,13 @@ class TextBuffer
     @setPath(@project.resolve(filePath)) if @project
 
   loadSync: ->
-    @updateCachedDiskContents()
-    @reload() if @loadFromDisk and @isModified()
+    @updateCachedDiskContentsSync()
+    @reload() if @loadFromDisk
     @text.clearUndoStack()
 
   load: ->
-    @updateCachedDiskContentsAsync().then =>
-      @reload() if @loadFromDisk and @isModified()
+    @updateCachedDiskContents().then =>
+      @reload() if @loadFromDisk
       @text.clearUndoStack()
       this
 
@@ -116,14 +117,21 @@ class TextBuffer
   subscribeToFile: ->
     @file.on "contents-changed", =>
       @conflict = true if @isModified()
-      @updateCachedDiskContentsAsync().done =>
-        if @conflict
-          @emit "contents-conflicted"
-        else
-          @reload()
+      previousContents = @cachedDiskContents
+
+      # Synchrounously update the disk contents because the {File} has already cached them. If the
+      # contents updated asynchrounously multiple `conlict` events could trigger for the same disk
+      # contents.
+      @updateCachedDiskContentsSync()
+      return if previousContents == @cachedDiskContents
+
+      if @conflict
+        @emit "contents-conflicted"
+      else
+        @reload()
 
     @file.on "removed", =>
-      @updateCachedDiskContentsAsync().done =>
+      @updateCachedDiskContents().done =>
         @emitModifiedStatusChanged(@isModified())
 
     @file.on "moved", =>
@@ -149,12 +157,14 @@ class TextBuffer
     @emit 'reloaded'
 
   # Private: Rereads the contents of the file, and stores them in the cache.
-  updateCachedDiskContents: ->
-    @cachedDiskContents = @file?.read() ? ""
+  updateCachedDiskContentsSync: ->
+    @loaded = true
+    @cachedDiskContents = @file?.readSync() ? ""
 
   # Private: Rereads the contents of the file, and stores them in the cache.
-  updateCachedDiskContentsAsync: ->
-    Q(@file?.readAsync() ? "").then (contents) =>
+  updateCachedDiskContents: ->
+    Q(@file?.read() ? "").then (contents) =>
+      @loaded = true
       @cachedDiskContents = contents
 
   # Gets the file's basename--that is, the file without any directory information.
@@ -409,6 +419,7 @@ class TextBuffer
   #
   # Returns a {Boolean}.
   isModified: ->
+    return false unless @loaded
     if @file
       if @file.exists()
         @getText() != @cachedDiskContents