Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix live preview reload for documentSaved events #5207

Merged
merged 7 commits into from
Sep 17, 2013

Conversation

jasonsanjose
Copy link
Member

Fix for #5185. Instead of relying on doc.url, simply use the Document.file.fullPath and let the server provide the expected URL.

@jasonsanjose
Copy link
Member Author

Updated with fix for #5222. @gruehle ready for review.

agents.network && agents.network.wasURLRequested(doc.url)) {
var absolutePath = Inspector.connected() && doc.file.fullPath,
liveDocument = _server && absolutePath && _server.get(absolutePath),
instrumentationEnabled = liveDocument && liveDocument.isInstrumentationEnabled && liveDocument.isInstrumentationEnabled();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming nit: the term "instrumentationEnabled" doesn't really match the functionality. HTML documents are instrumented, but CSS documents aren't. How about using liveEditingEnabled instead? The function on CSSDocument/HTMLDocument would be called isLiveEditingEnabled().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@gruehle
Copy link
Member

gruehle commented Sep 16, 2013

I'm seeing a failure in the Live Development unit tests: "should reapply in-memory css changes after saving changes in html document".

@gruehle
Copy link
Member

gruehle commented Sep 16, 2013

Initial review complete.

@@ -104,7 +121,7 @@ define(function (require, exports, module) {

var liveDoc;
waitsFor(function () {
liveDoc = LiveDevelopment.getLiveDocForPath(testPath + "/" + cssFile);
liveDoc = LiveDevelopment.getLiveDocForPath(SpecRunnerUtils.getTempDirectory() + "/" + cssFile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SpecRunnerUtils.getTempDirectory() is used several times in this file. This value should be in a variable.

@gruehle
Copy link
Member

gruehle commented Sep 17, 2013

@jasonsanjose re-review complete. Still have one failing unit test.

@jasonsanjose
Copy link
Member Author

@gruehle went a different route to fix the async bug in that test. Changed the tests to wait for the Inspector.Page loadEventFired event instead.

@@ -688,13 +729,6 @@ define(function (require, exports, module) {
// Verify that we still have modified text
expect(updatedNode.value).toBe("Live Preview in Brackets is awesome!");
});

// Save original content back to the file after this test passes/fails
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to restore file content now that we read the project from test/temp

@gruehle
Copy link
Member

gruehle commented Sep 17, 2013

Everything is working well. It's too bad we need to wait 3 seconds for each of the tests that is expecting the page to not reload, but I can't think of an alternative.

Merging.

gruehle added a commit that referenced this pull request Sep 17, 2013
Fix live preview reload for documentSaved events
@gruehle gruehle merged commit 8edfd23 into master Sep 17, 2013
@jasonsanjose jasonsanjose deleted the jasonsanjose/issue-5185 branch September 17, 2013 21:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants