-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Simplify dirty bit management #161
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,18 +100,11 @@ define(function(require, exports, module) { | |
|
||
// Dirty-bit tracking | ||
editor.setOption("onChange", this._updateDirty.bind(this)); | ||
this._savedUndoPosition = editor.historySize().undo; // should always be 0, but just to be safe... | ||
this.isDirty = false; | ||
} | ||
|
||
/** | ||
* @private | ||
* TODO: we should close on whether private fields are declared on the prototype like this | ||
* @type {number} | ||
*/ | ||
Document.prototype._savedUndoPosition = 0; | ||
|
||
/** | ||
* @return {string} The editor's current contents; may not be saved to disk | ||
* @return {string} The document's current contents; may not be saved to disk | ||
* yet. Returns null if the file was not yet read and no editor was | ||
* created. | ||
*/ | ||
|
@@ -121,6 +114,16 @@ define(function(require, exports, module) { | |
return this._editor.getValue(); | ||
} | ||
|
||
/** | ||
* Sets the contents of the document. | ||
* @param {!string} text The text to replace the contents of the document with. | ||
*/ | ||
Document.prototype.setText = function(text) { | ||
console.assert(this._editor != null); | ||
|
||
this._editor.setValue(text); | ||
} | ||
|
||
/** | ||
* @private | ||
*/ | ||
|
@@ -129,24 +132,17 @@ define(function(require, exports, module) { | |
return; | ||
} | ||
|
||
// If we've undone past the undo position at the last save, and there is no redo stack, | ||
// then we can never get back to a non-dirty state. | ||
var historySize = this._editor.historySize(); | ||
if (historySize.undo < this._savedUndoPosition && historySize.redo == 0) { | ||
this._savedUndoPosition = -1; | ||
} | ||
var newIsDirty = (this._editor.historySize().undo != this._savedUndoPosition); | ||
|
||
if (this.isDirty != newIsDirty) { | ||
this.isDirty = newIsDirty; | ||
// On any change, mark the file dirty. In the future, we should make it so that if you | ||
// undo back to the last saved state, we mark the file clean. | ||
var wasDirty = this.isDirty; | ||
this.isDirty = true; | ||
|
||
// Dispatch event | ||
$(exports).triggerHandler("dirtyFlagChange", this); | ||
// Dispatch event | ||
$(exports).triggerHandler("dirtyFlagChange", this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should go within the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. |
||
|
||
// If file just became dirty, add it to working set (if not already there) | ||
if (newIsDirty) | ||
addToWorkingSet(this); | ||
} | ||
// If file just became dirty, add it to working set (if not already there) | ||
if (!wasDirty) | ||
addToWorkingSet(this); | ||
} | ||
|
||
/** Marks the document not dirty. Should be called after the document is saved to disk. */ | ||
|
@@ -155,8 +151,8 @@ define(function(require, exports, module) { | |
return; | ||
} | ||
|
||
this._savedUndoPosition = this._editor.historySize().undo; | ||
this._updateDirty(); | ||
this.isDirty = false; | ||
$(exports).triggerHandler("dirtyFlagChange", this); | ||
} | ||
|
||
/* (pretty toString(), to aid debugging) */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -146,6 +146,10 @@ define(function(require, exports, module) { | |
|
||
describe("Dirty File Handling", function() { | ||
|
||
// This test is not currently valid. For issue 152, we have temporarily decided to make it so | ||
// the file is always dirty as soon as you make a change, regardless of whether you undo back to | ||
// its last saved state. | ||
/* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you spin off a new bug (or add a story) for restoring the nicer behavior we had before? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, a story got sent to Adam (and Jay added it to Brainstorm). |
||
it("should report not dirty after undo", function() { | ||
var didOpen = false, gotError = false; | ||
|
||
|
@@ -167,8 +171,8 @@ define(function(require, exports, module) { | |
expect(DocumentManager.getCurrentDocument().isDirty).toBe(false); | ||
}); | ||
}); | ||
|
||
it("should report dirty when modified", function() { | ||
*/ | ||
beforeEach(function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice refactoring! Want to fix the commented-out test too? (And then I guess it'd make sense to just move this to the top of the describe() block...) |
||
var didOpen = false, gotError = false; | ||
|
||
runs(function() { | ||
|
@@ -177,7 +181,15 @@ define(function(require, exports, module) { | |
.fail(function() { gotError = true; }); | ||
}); | ||
waitsFor(function() { return didOpen && !gotError; }, "FILE_OPEN timeout", 1000); | ||
}); | ||
|
||
it("should report clean immediately after opening a file", function() { | ||
runs(function() { | ||
expect(DocumentManager.getCurrentDocument().isDirty).toBe(false); | ||
}); | ||
}); | ||
|
||
it("should report dirty when modified", function() { | ||
runs(function() { | ||
// change editor content | ||
var editor = DocumentManager.getCurrentDocument()._editor; | ||
|
@@ -199,15 +211,6 @@ define(function(require, exports, module) { | |
}); | ||
|
||
it("should report dirty after undo and redo", function() { | ||
var didOpen = false, gotError = false; | ||
|
||
runs(function() { | ||
CommandManager.execute(Commands.FILE_OPEN, {fullPath: testPath + "/test.js"}) | ||
.done(function() { didOpen = true; }) | ||
.fail(function() { gotError = true; }); | ||
}); | ||
waitsFor(function() { return didOpen && !gotError; }, "FILE_OPEN timeout", 1000); | ||
|
||
runs(function() { | ||
// change editor content, followed by undo and redo | ||
var editor = DocumentManager.getCurrentDocument()._editor; | ||
|
@@ -222,6 +225,15 @@ define(function(require, exports, module) { | |
expect(DocumentManager.getCurrentDocument().isDirty).toBe(true); | ||
}); | ||
}); | ||
|
||
it("should report clean after being marked clean", function() { | ||
runs(function() { | ||
var document = DocumentManager.getCurrentDocument(); | ||
document.setText(TEST_JS_NEW_CONTENT); | ||
document.markClean(); | ||
expect(document.isDirty).toBe(false); | ||
}); | ||
}); | ||
}); | ||
|
||
// TODO (jasonsj): experiment with mocks instead of real UI | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always go back and forth on whether it's worth it to have an assert() if the next line of code is guaranteed to crash anyway (NPE in this case)... but feel free to keep it in if you like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just following the existing code from getText()...I'm fine with removing it.