Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Use file watchers instead of document saved event #6510

Open
wants to merge 1 commit into from

4 participants

@jasonsanjose

Thanks to file watchers, we already improved live preview for workflows that include a preprocessor and an associated watch task. For example, while editing SASS or LESS in live preview, if the user has appropriate tools in place to watch and compile files on the fly, we can reload the live preview browser to show changes. This is new behavior in sprint 36.

The infrastructure was already in place where we have the gather related documents for the live preview URL and create live documents, e.g. CSSDocument. These documents already watch for file changes on disk and gave us this new behavior basically for free.

So...this pull request uses file watching in place of the documentSaved event from DocumentManager to trigger a browser reload for document types that we don't account for as a "related" document. This allows external editors such as image editors, other text editors (ahem! :cold_sweat:) and background jobs to trigger live preview to reload, which should users very simple "live reload" behavior.

@peterflynn
Owner

@jasonsanjose I wonder if the DocumentManager "documentRefreshed" event is actually the best match here. That will trigger any time the document is saved, reloaded from disk, or reverted. So for exampe, if external changes trigger a conflict dialog and the user chooses to ignore the external changes, this won't trigger a refresh.

I'm also not sure still if watcher changes may have some redundancy, due to the artificial change that's initially sent followed by the "real" watchers picking up the change a moment later. Whereas "documentRefreshed" is based on a timestamp check, so it's less likely to fire extra times.

@MarcelGerber

@peterflynn But if I understood you right, you have to go back to Brackets and have to accept the external changes to fire the event? That wouldn't really improve the workflow.

@peterflynn
Owner

@SAPlayer No, that's only in the event that there's a conflict between unsaved changes in Brackets and changes made by an external app. And in that case, it does seem more correct to not automatically reload until the user decides which version they want to use.

@peterflynn peterflynn was assigned
@jasonsanjose

@peterflynn back to your first comment (only took me 2 months...). A few concerns:

  • documentRefreshed will only fire for text files if I follow the usage correctly. I figured FileSystem change is the safest bet for all types of files including binary.
  • I forgot about the conflict dialog. I can add a check to see if the in-memory document is dirty and skip the call to Page.reload() in that case. Sound good? So, if the browser is the active window, and say we make an external change but Brackets still has in-memory changes, then it's a no-op.
  • Are you still unsure about redundant change events? I suppose there's some potential harm in reloading the browser unexpectedly. Can't we filter out extra change events with stat?
@dangoor
Owner

@peterflynn we could likely reassign this one to reduce what's on your plate. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 16 additions and 17 deletions.
  1. +16 −17 src/LiveDevelopment/LiveDevelopment.js
View
33 src/LiveDevelopment/LiveDevelopment.js
@@ -81,6 +81,7 @@ define(function LiveDevelopment(require, exports, module) {
DefaultDialogs = require("widgets/DefaultDialogs"),
DocumentManager = require("document/DocumentManager"),
EditorManager = require("editor/EditorManager"),
+ FileSystem = require("filesystem/FileSystem"),
FileServer = require("LiveDevelopment/Servers/FileServer").FileServer,
FileSystemError = require("filesystem/FileSystemError"),
FileUtils = require("file/FileUtils"),
@@ -1270,34 +1271,32 @@ define(function LiveDevelopment(require, exports, module) {
}
/**
- * Triggered by a documentSaved event from DocumentManager.
+ * Triggered by a changed event from FileSystem.
* @param {$.Event} event
* @param {Document} doc
*/
- function _onDocumentSaved(event, doc) {
+ function _onFileSystemChange(event, entry, added, removed) {
if (!Inspector.connected() || !_server) {
return;
}
- var absolutePath = doc.file.fullPath,
- liveDocument = absolutePath && _server.get(absolutePath),
- liveEditingEnabled = liveDocument && liveDocument.isLiveEditingEnabled && liveDocument.isLiveEditingEnabled();
+ var fullPath = entry && entry.isFile && entry.fullPath,
+ url = fullPath && _server.pathToUrl(fullPath),
+ liveDocument = fullPath && _server.get(fullPath),
+ liveEditingEnabled = liveDocument && liveDocument.isLiveEditingEnabled && liveDocument.isLiveEditingEnabled(),
+ wasRequested = agents.network.wasURLRequested(url);
- // Skip reload if the saved document has live editing enabled
- if (liveEditingEnabled) {
+ // Skip reload if the saved document has live editing enabled or if
+ // the changed file was never requested
+ if (liveEditingEnabled || !wasRequested) {
return;
}
- var documentUrl = _server.pathToUrl(absolutePath),
- wasRequested = agents.network && agents.network.wasURLRequested(documentUrl);
-
- if (wasRequested) {
- // Unload and reload agents before reloading the page
- reconnect();
+ // Unload and reload agents before reloading the page
+ reconnect();
- // Reload HTML page
- Inspector.Page.reload();
- }
+ // Reload HTML page
+ Inspector.Page.reload();
}
/** Triggered by a change in dirty flag from the DocumentManager */
@@ -1330,8 +1329,8 @@ define(function LiveDevelopment(require, exports, module) {
exports.config = theConfig;
$(Inspector).on("error", _onError);
$(Inspector.Inspector).on("detached", _onDetached);
+ FileSystem.on("change", _onFileSystemChange);
$(DocumentManager).on("currentDocumentChange", _onDocumentChange)
- .on("documentSaved", _onDocumentSaved)
.on("dirtyFlagChange", _onDirtyFlagChange);
$(ProjectManager).on("beforeProjectClose beforeAppClose", close);
Something went wrong with that request. Please try again.