This repository has been archived by the owner on Sep 6, 2021. It is now read-only.
Live development: handle deleting CSS file on disk & discarding unsaved changes to CSS file #558
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…owser accordingly) - Fix exception in Document when unsaved changes are discarded but live development still has a ref to the doc (bug #546). More comprehensive fix probably to follow, though - Add warning in DocumentManager when deleting a doc does not clear all refs
…ussion with Glenn. When a CSSDocument gets a Document "deleted" event, it now dispatches another "deleted" event of its own rather than pinging LiveDevelopment directly to unregister itself; LiveDevelopment is now responsible for listening for this CSSDocument event and unregistering the CSSDocument automatically.
…t to whatever's on disk - because other parts of the UI might still be showing the contents of the Document, and we don't want them to keep reflecting the unsaved (now discarded) changes.
…This was breaking many unit tests, which frequently throw away unsaved Documents. It also required somewhat hacky code in notifyFileDeleted() to avoid hitting the assertion in that case. The assertion was sort of out of place anyway: there's nothing intrinsic about destroying _masterEditor and moving the text back into a string that makes unsaved changes not ok; the idea that unsaved changes should be dealt with before closing the main editor is really just a UI feature/rule. - Fix bug where notifyFileDeleted() crashed if a file was deleted that was only open in the main editor, or in the working set and not open at all. In those cases 'doc' is null and the refCount check at the end would crash. - Update unit test comment
…c-from-disk * origin/master: Remove the project reloading for now. When we do the file watchers story we can do a more fine grained approach for this
|
@gruehle: Would you like to review this one? |
|
reviewing |
| CSSDocument.prototype.onChange = function onChange(event, editor, change) { | ||
| // brute force: update the CSS | ||
| CSSAgent.reloadDocument(this.doc); | ||
| }; | ||
| /** Triggered if the Document's file is deleted */ | ||
| CSSDocument.prototype.onDeleted = function onChange(event, editor, change) { |
There was a problem hiding this comment.
This looks like a copy/paste error: the name after function is "onChange" instead of "onDeleted"
|
Initial review complete |
| /** Empties a CSS style sheet given a document that has been deleted | ||
| * @param {Document} document | ||
| */ | ||
| function reloadDeletedDocument(doc) { |
There was a problem hiding this comment.
Name is a little weird--you're not really "reloading" the deleted document. Maybe "handleDeletedDocument" or some such.
There was a problem hiding this comment.
It was named in symmetry with reloadDocument() above... which, granted, is not reloading the Document either. Really, they're both reloading the CSS in the browser for a given Document. I'll rename both of them.
- unregister LiveDevelopment's listeners on CSSDocument when LiveDevelopment stops caring about a particular document - fix misnamed function
- Rename CSSAgent methods for clarity - When discarding unsaved changes, revert Document contents after closing main editor instead of before. As a bonus, this makes it easy to skip the revert if the main editor was the only view attached to the Document.
…c-from-disk * origin/master: Fix html extension check. Make clicks in dead space in inline CSS widget set focus back to the actual editor and set the selection to the beginning/end Update CodeMirror SHA Correctly handle scenario where no document is active. Make sure in-memory changes to CSS docs are pushed when starting live development connection. Fail silently when trying to connect to stale connection. Reverted close() back to onClosed() since close() already means something Code review cleanup: changed onClosed() to close(), other minor fixes. Made Editor.setInlineWidgetHeight() also take a widget instead of an ID Get rid of EditorManager._addInlineWidget, an dmake it so Editor.addInlineWidget just takes an actual inlineWidget and stores the widget itself in the _inlineWidgets list.
|
Looks good. |
gruehle
added a commit
that referenced
this pull request
Apr 5, 2012
Live development: handle deleting CSS file on disk & discarding unsaved changes to CSS file
gideonthomas
added a commit
to gideonthomas/brackets
that referenced
this pull request
Aug 30, 2016
…mes (adobe#558) * Trigger different events with different data for file vs. folder renames * Review fixes
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This replaces the
refreshText("")hack we used to have in Document._makeNonEditable().