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

Replace uses of doc.url with _server.pathToUrl() #6126

Merged
merged 4 commits into from
Dec 3, 2013
Merged

Conversation

njx
Copy link

@njx njx commented Nov 27, 2013

For #5317. doc.url was an old field that used to get set on the document before we refactored the live dev server management in 1f6b5aa, but there were still a few places referencing it.

@ghost ghost assigned jasonsanjose Nov 27, 2013
@njx
Copy link
Author

njx commented Nov 27, 2013

To @jasonsanjose since he's already looked at this branch :)

@jasonsanjose
Copy link
Member

Confirmed this fixes the issue with .js files and the out-of-sync status.

#5317 also mentions this is broken for HTML-files from a user server. That bug still exists here. The offending code is in LiveDevelopment _docIsOutOfSync. I have a fix:

    function _docIsOutOfSync(doc) {
        var docClass    = _classForDocument(doc),
            liveDoc     = _server && _server.get(doc.file.fullPath),
            isLiveEditingEnabled = liveDoc && liveDoc.isLiveEditingEnabled();

        return doc.isDirty && !isLiveEditingEnabled;
    }

@jasonsanjose
Copy link
Member

@njx review complete

@njx
Copy link
Author

njx commented Dec 3, 2013

Ah, I missed that other part of the bug. I'll look at your proposed fix.

@njx
Copy link
Author

njx commented Dec 3, 2013

I added your fix and added a few unit tests to make sure the dirty dot does show up in the JS case and doesn't show up in the live HTML/CSS cases. I didn't add a test for the user-server-HTML case since I wasn't sure of a good way to do that, and it doesn't seem like as critical a case anyway.

@njx
Copy link
Author

njx commented Dec 3, 2013

Ready for re-review.

@jasonsanjose
Copy link
Member

Looks good. Merging.

jasonsanjose added a commit that referenced this pull request Dec 3, 2013
Replace uses of doc.url with _server.pathToUrl()
@jasonsanjose jasonsanjose merged commit d46fe5d into master Dec 3, 2013
@jasonsanjose jasonsanjose deleted the nj/issue-5317 branch December 3, 2013 04:05
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