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

Fix Live Preview for Docs not in Working Set #8605

Merged
merged 6 commits into from
Aug 6, 2014
Merged

Fix Live Preview for Docs not in Working Set #8605

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e02e71d
handle live preview for docs not in working set
redmunds Jul 31, 2014
ba79595
jsdoc info
redmunds Aug 4, 2014
1f799cf
refactor to use a _removeHighlight() method
redmunds Aug 4, 2014
15285fa
more changes for code review
redmunds Aug 4, 2014
e058969
Merge remote-tracking branch 'origin/master' into randy/issue-7886
redmunds Aug 6, 2014
00fa553
code cleanup
redmunds Aug 6, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 71 additions & 22 deletions src/LiveDevelopment/Documents/HTMLDocument.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ define(function HTMLDocumentModule(require, exports, module) {

var DocumentManager = require("document/DocumentManager"),
DOMAgent = require("LiveDevelopment/Agents/DOMAgent"),
EditorManager = require("editor/EditorManager"),
HighlightAgent = require("LiveDevelopment/Agents/HighlightAgent"),
HTMLInstrumentation = require("language/HTMLInstrumentation"),
Inspector = require("LiveDevelopment/Inspector/Inspector"),
Expand All @@ -64,36 +65,26 @@ define(function HTMLDocumentModule(require, exports, module) {
var self = this;

this.doc = doc;
if (!editor) {
return;
if (this.doc) {
this.doc.addRef();
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

}

this.editor = editor;
this._instrumentationEnabled = false;

// Performance optimization to use closures instead of Function.bind()
// to improve responsiveness during cursor movement and keyboard events
$(this.editor).on("cursorActivity.HTMLDocument", function (event, editor) {
self._onCursorActivity(event, editor);
});

$(this.editor).on("change.HTMLDocument", function (event, editor, change) {
self._onChange(event, editor, change);
});
this.onActiveEditorChange = this.onActiveEditorChange.bind(this);
$(EditorManager).on("activeEditorChange", this.onActiveEditorChange);

// Experimental code
if (LiveDevelopment.config.experimental) {
$(HighlightAgent).on("highlight.HTMLDocument", function (event, node) {
self._onHighlight(event, node);
});
}
// Attach now
this.attachToEditor(editor);
};

/**
* Enable instrumented HTML
* @param enabled {boolean}
*/
HTMLDocument.prototype.setInstrumentationEnabled = function setInstrumentationEnabled(enabled) {
if (enabled && !this._instrumentationEnabled) {
if (enabled && !this._instrumentationEnabled && this.editor) {
HTMLInstrumentation.scanDocument(this.doc);
HTMLInstrumentation._markText(this.editor);
}
Expand All @@ -115,7 +106,7 @@ define(function HTMLDocumentModule(require, exports, module) {
*/
HTMLDocument.prototype.getResponseData = function getResponseData(enabled) {
var body;
if (this._instrumentationEnabled) {
if (this._instrumentationEnabled && this.editor) {
body = HTMLInstrumentation.generateInstrumentedHTML(this.editor);
}

Expand All @@ -126,11 +117,15 @@ define(function HTMLDocumentModule(require, exports, module) {

/** Close the document */
HTMLDocument.prototype.close = function close() {
Copy link
Contributor

Choose a reason for hiding this comment

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

good time to add some JSDoc here

Copy link
Contributor

Choose a reason for hiding this comment

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

n/m i guess this is fully documented :)

if (!this.editor) {
return;
if (this.editor) {
$(this.editor).off(".HTMLDocument");
}

if (this.doc) {
this.doc.releaseRef();
}

$(this.editor).off(".HTMLDocument");
$(EditorManager).off("activeEditorChange", this.onActiveEditorChange);

// Experimental code
if (LiveDevelopment.config.experimental) {
Expand All @@ -139,8 +134,53 @@ define(function HTMLDocumentModule(require, exports, module) {
}
};

/** Attach new editor */
HTMLDocument.prototype.attachToEditor = function (editor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs JSDoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. FYI, I made a few other updates to to jsdocs beyond the places you mentioned.

var self = this;
this.editor = editor;

if (this.editor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this check from all the the ctor to here because it seemed safer to always do it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

you still don't need it. attachEditor is only called when it has an editor and the docs for the ctor say that the editor argument is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// Performance optimization to use closures instead of Function.bind()
// to improve responsiveness during cursor movement and keyboard events
$(this.editor).on("cursorActivity.HTMLDocument", function (event, editor) {
self._onCursorActivity(event, editor);
});

$(this.editor).on("change.HTMLDocument", function (event, editor, change) {
self._onChange(event, editor, change);
});

// Experimental code
if (LiveDevelopment.config.experimental) {
$(HighlightAgent).on("highlight.HTMLDocument", function (event, node) {
self._onHighlight(event, node);
});
}

if (this._instrumentationEnabled) {
// Update instrumentation for new editor
HTMLInstrumentation.scanDocument(this.doc);
HTMLInstrumentation._markText(this.editor);
}
}
};

/** Detach current editor */
HTMLDocument.prototype.detachFromEditor = function () {
if (this.editor) {
HighlightAgent.hide();
$(this.editor).off(".HTMLDocument");
this._onHighlight();
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't call a handler without event data, you should move the top of the event handler to a new function _removeHighlight() and call that from here and from _onHighlight() to avoid problems in the future,

here's the top of _onHighlight()

if (!node || !node.location || !this.editor) {
   this._removeHighlight();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea -- much cleaner.

this.editor = null;
}
};

/** Update the highlight */
HTMLDocument.prototype.updateHighlight = function () {
if (!this.editor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you shouldn't need this. It will not get called unless there is an editor.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need this. updateHighlight is only called from _onCursorActivity which is an editor event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

return;
}

var editor = this.editor,
codeMirror = editor._codeMirror,
ids = [];
Expand Down Expand Up @@ -294,6 +334,15 @@ define(function HTMLDocumentModule(require, exports, module) {
// }
};

/** Triggered when the active editor changes */
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to keep all the 2 code blocks that are commented out in _onChange around? I know that isn't part of this pr but we should try to clean that up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not my old code, so I left it in.

HTMLDocument.prototype.onActiveEditorChange = function (event, newActive, oldActive) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs JSDoc

Copy link
Contributor

Choose a reason for hiding this comment

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

Move this up above the private handlers for better organization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also private, to renamed to _onActiveEditorChange.

this.detachFromEditor();

if (newActive && newActive.document === this.doc) {
this.attachToEditor(newActive);
}
};

/** Triggered by the HighlightAgent to highlight a node in the editor */
HTMLDocument.prototype._onHighlight = function (event, node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should clean this up with some JSDoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if (!node || !node.location || !this.editor) {
Expand Down