Enhance deleted state

spec/text-buffer-io-spec.js

@@ -14,7 +14,7 @@ const winattr = require('winattr')
 process.on('unhandledRejection', console.error)
 
 async function wait (ms) {
-  return new Promise(r => setTimeout(r, ms));
+  return new Promise(r => setTimeout(r, ms))
 }
 
 describe('TextBuffer IO', () => {
@@ -34,7 +34,7 @@ describe('TextBuffer IO', () => {
     // `await` briefly to allow the file watcher to clean up. This is a
     // `pathwatcher` requirement that we can fix by updating its API — but
     // that's a can of worms we don't want to open yet.
-    await wait(10);
+    await wait(10)
   })
 
   describe('.load', () => {
@@ -408,6 +408,13 @@ describe('TextBuffer IO', () => {
         expect(buffer.isInConflict()).toBe(true)
         await buffer.save()
         expect(buffer.isInConflict()).toBe(false)
+        // Ensure we don't get flipped into conflicted status after the
+        // `onDidChange` handler comes through…
+        await wait(1000)
+        expect(buffer.isInConflict()).toBe(false)
+        buffer.setText('q')
+        // …and the buffer is modified again.
+        expect(buffer.isInConflict()).toBe(false)
         done()
       })
     })
@@ -574,8 +581,9 @@ describe('TextBuffer IO', () => {
   })
 
   describe('.isModified', () => {
+    let filePath
     beforeEach(async done => {
-      const filePath = temp.openSync('atom').path
+      filePath = temp.openSync('atom').path
       fs.writeFileSync(filePath, '')
       buffer = await TextBuffer.load(filePath)
       done()
@@ -600,10 +608,23 @@ describe('TextBuffer IO', () => {
         buffer.undo()
         buffer.undo()
         expect(buffer.isModified()).toBe(false)
+        expect(buffer.isDeleted()).toBe(false)
         await stopChangingPromise()
         expect(modifiedStatusChanges).toEqual([true, false])
         done()
       })
+
+      describe('and the file is deleted', () => {
+        it('reports the modified status as true', async () => {
+          buffer.setText(`lorem ipsum`)
+          await buffer.save()
+          buffer.setText(`lorem ipsum dolor`)
+          fs.unlinkSync(filePath)
+          await wait(500)
+          expect(buffer.isModified()).toBe(true)
+          expect(buffer.isDeleted()).toBe(true)
+        })
+      })
     })
 
     describe('when the buffer is saved', () => {
@@ -620,6 +641,54 @@ describe('TextBuffer IO', () => {
         expect(modifiedStatusChanges).toEqual([false])
         done()
       })
+
+      describe('and the file is deleted', () => {
+        it('reports the modified status as false', async () => {
+          buffer.setText(`lorem ipsum`)
+          await buffer.save()
+          fs.unlinkSync(filePath)
+          await wait(500)
+          expect(buffer.isModified()).toBe(false)
+          expect(buffer.isDeleted()).toBe(true)
+        })
+
+        it('initially reports the modified status as false, but flips it back to true if the user makes further changes', async () => {
+          buffer.setText(`lorem ipsum`)
+          await buffer.save()
+          fs.unlinkSync(filePath)
+          await wait(500)
+          expect(buffer.isModified()).toBe(false)
+          expect(buffer.isDeleted()).toBe(true)
+
+          buffer.insert([0, 0], '! ')
+          expect(buffer.isModified()).toBe(true)
+          expect(buffer.isDeleted()).toBe(true)
+
+          // `isModified` should return `true` even if we revert the buffer's
+          // contents to what they were at the time of deletion.
+          buffer.setText(`lorem ipsum`)
+          expect(buffer.isModified()).toBe(true)
+          expect(buffer.isDeleted()).toBe(true)
+        })
+
+        describe('and re-saved', () => {
+          it('results in isModified and isDeleted no longer returning true', async () => {
+            buffer.setText(`lorem ipsum`)
+            await buffer.save()
+            fs.unlinkSync(filePath)
+            await wait(500)
+            buffer.insert([0, 0], '! ')
+            expect(buffer.isModified()).toBe(true)
+            expect(buffer.isDeleted()).toBe(true)
+
+            await buffer.saveAs(filePath)
+
+            expect(buffer.isModified()).toBe(false)
+            expect(buffer.isDeleted()).toBe(false)
+          })
+        })
+      })
+
     })
 
     describe('when the buffer is reloaded', () => {
@@ -1080,7 +1149,18 @@ describe('TextBuffer IO', () => {
           fs.removeSync(filePath)
           buffer.file.onDidDelete(() => {
             expect(buffer.getPath()).toBe(filePath)
-            expect(buffer.isModified()).toBeTruthy()
+            // `buffer.isModified` used to report `true` automatically whenever
+            // a buffer does not have a backing file. Now it depends on whether
+            // the file ever existed – and, if so, whether the buffer was in a
+            // modified state when the file was deleted.
+            //
+            // The narrow exception we're carving out is one where the file
+            // contents were in sync with the buffer contents at the moment of
+            // file deletion. If so, its `isModified=false` status will persist
+            // until even one edit is made, at which point it will flip back to
+            // `isModified=true` until the buffer is destroyed or once again
+            // saved to disk.
+            expect(buffer.isModified()).toBeFalsy()
             done()
           })
         })

src/text-buffer.js

@@ -102,6 +102,15 @@ class TextBuffer {
     this.cachedHasAstral = null
     this._emittedWillChangeEvent = false
 
+    // Whether a buffer has ever had a backing file, whether or not it exists
+    // now.
+    this.didHaveFileOnDisk = false
+
+    // When a buffer's backing file is deleted while the file is unmodified,
+    // this trait flips to `true`… and then flips back to `false` if any
+    // further edits are made.
+    this.retainsUnmodifiedTraitAfterDeletion = false
+
     this.setEncoding(params.encoding)
     this.setPreferredLineEnding(params.preferredLineEnding)
 
@@ -524,15 +533,32 @@ class TextBuffer {
   //
   // Returns a {Boolean}.
   isModified () {
+    if (this.isDeleted()) {
+      // We typically consider a deleted file to be modified… unless it was
+      // unmodified at the time of deletion and has not been modified since.
+      return !this.retainsUnmodifiedTraitAfterDeletion
+    }
     if (this.file) {
       return !this.file.existsSync() || this.buffer.isModified()
     } else {
       return this.buffer.getLength() > 0
     }
   }
 
-  // Public: Determine if the in-memory contents of the buffer conflict with the
-  // on-disk contents of its associated file.
+  // Public: Determine if the buffer is in a deleted state — meaning that it
+  // was previously backed by a file on disk, but is no longer.
+  isDeleted () {
+    let hasNoFile = !this.file || !this.file.existsSync()
+    return hasNoFile && this.didHaveFileOnDisk
+  }
+
+  // Public: Determine if the in-memory contents of the buffer conflict with
+  // the on-disk contents of its associated file.
+  //
+  // This happens if the contents of a buffer’s backing file change while the
+  // editor has uncommitted changes. Those uncommitted changes build upon a
+  // state that is now stale; if those changes were committed to disk, it could
+  // clobber the changes made by the external program.
   //
   // Returns a {Boolean}.
   isInConflict () {
@@ -845,6 +871,7 @@ class TextBuffer {
     if (undo != null) {
       Grim.deprecate('The `undo` option is deprecated. Call groupLastChanges() on the TextBuffer afterward instead.')
     }
+    this.retainsUnmodifiedTraitAfterDeletion = false
 
     if (this.transactCallDepth === 0) {
       const newRange = this.transact(() => this.setTextInRange(range, newText, {normalizeLineEndings}))
@@ -1929,6 +1956,7 @@ class TextBuffer {
 
       try {
         await this.buffer.save(destination, this.getEncoding())
+        this.didHaveFileOnDisk = true
       } catch (error) {
         if (error.code !== 'EACCES' || destination !== filePath) throw error
 
@@ -2101,6 +2129,8 @@ class TextBuffer {
       Grim.deprecate('The .load instance method is deprecated. Create a loaded buffer using TextBuffer.load(filePath) instead.')
     }
 
+    this.didHaveFileOnDisk = true
+
     const source = this.file instanceof File
       ? this.file.getPath()
       : this.file.createReadStream()
@@ -2273,9 +2303,20 @@ class TextBuffer {
         if (this.isModified()) {
           const source = this.file.getPath()
           if (!(await this.buffer.baseTextMatchesFile(source, this.getEncoding()))) {
+            // Emit `did-conflict` and take no other action. We will keep the
+            // current buffer contents so that the user's changes are not lost.
             this.emitter.emit('did-conflict')
+          } else {
+            // Despite being modified, we're once again in alignment with what
+            // is on disk. This file is not in conflict.
+            this.fileHasChangedSinceLastLoad = false
           }
         } else {
+          // This buffer was previously in sync with what was on disk, so we
+          // can update its contents to match the new contents on disk. By
+          // definition, this means there is no conflict, so we'll reset the
+          // appropriate flag.
+          this.fileHasChangedSinceLastLoad = false
           return this.load({internal: true})
         }
       }, this.fileChangeDelay)))
@@ -2284,6 +2325,7 @@ class TextBuffer {
     if (this.file.onDidDelete) {
       this.fileSubscriptions.add(this.file.onDidDelete(() => {
         const modified = this.buffer.isModified()
+        this.retainsUnmodifiedTraitAfterDeletion = !modified
         this.emitter.emit('did-delete')
         if (!modified && this.shouldDestroyOnFileDelete()) {
           return this.destroy()