Fixed: #2278 Editor should not have upward dependencies on EditorManager

[WebsiteDeveloper]

Fixed #2278
added focus listener and moved _handleSelectAll() to EditorCommandHandler
@dangoor i opened a new pull request

[peterflynn]

I don't think this has any effect: you'll need to modify Editor to actually dispatch a "focus" event, replacing the line that calls _notifyActiveEditorChanged(). (And then stop exporting _notifyActiveEditorChanged() from EditorManager).

[peterflynn]

Also, once you remove the call from Editor, please also remove the require("editor/EditorManager") call at top.

[WebsiteDeveloper]

I already included this in a pull request before but had some serious issues with git so i forgot that

[dangoor]

This line causes a JSLint error because _notifyActiveEditorChanged is defined after this call to it. Also, as Peter mentions you can delete line 778:

    exports._notifyActiveEditorChanged = _notifyActiveEditorChanged;

[WebsiteDeveloper]

@dangoor could you look at this

[dangoor]

@WebsiteDeveloper, I will take a look but it may be a couple of days. I appreciate your help here and will try to get through the review as soon as I can.

[dangoor]

Review complete. Sorry about the delay! I'm at a conference right now and hadn't had a chance to go through this until now.

Just the minor changes I mentioned above and this is ready to merge.

[WebsiteDeveloper]

@dangoor ready for final review

[peterflynn]

@WebsiteDeveloper looks like there are a lot of files with big no-op diffs (line endings changing?). It might be best if you put up a new pull request with cleaner commits.

Also, what are the ramifications of the text=auto directive that you're adding in .gitattributes? Could that be what caused all your line ending problems? If so we wouldn't want to merge that change.

[WebsiteDeveloper]

@peterflynn the line ending changes are to normalize all line endings to LF endings
as to the .gitattributes file i added this to avoid issues with the GitHub client because in some files it
changes all my line endings to windows line endings which differ from those in the repo

[dangoor]

I have reviewed the changes and everything seems okay. I think the LF normalization for the Brackets repository is fine, but I've asked to see if there are other opinions on the Brackets team. I may need to wait until later in the week to do the merge, because we're in the process of preparing releases.

For future pull requests, I'd suggest making a separate branch for individual changes. Git+GitHub makes working with branches pretty easy and it makes merging and reviewing more straightforward.

[WebsiteDeveloper]

Yeah you're right i should have made to branches but i am in the process learning to work effectivly with git
so i'll remember your advice for future pull requests

[dangoor]

No worries. In some ways, it's harder from your end because you have to keep stuff off of your master until this lands (though you can keep working from a branch easily enough!)

The "create a branch for every little thing" workflow is definitely new if you're used to a system like Subversion, but it works quite well.

[dangoor]

No one seemed opposed to the idea of normalized line endings in our repository, so I was going to go ahead and merge in your change. However, a merge with master needs to happen. A couple of things to note:

1. the call that _handleSelectAll needs to make has changed (be sure select all works after merging)
2. find does not work (the unit tests catch this). Offhand, I don't know what's wrong here.

Thanks for submitting your more recent pull requests from branches. It's definitely much easier to work with. Hopefully, we'll be all set with this one soon.

[WebsiteDeveloper]

@dangoor i merged master but to the failing unit tests for find i have currently no idea why they fail but find does work on my machine only the unit tests seem to fail

[dangoor]

Does find work on your machine after you've reloaded Brackets with your merged repo (and with caching disabled via the devtools)?

For me, find was failing with that code, but not when I switched back to master.

[WebsiteDeveloper]

interestingly it works for me with disabled cache and multiple reloads

[dangoor]

Huh, OK. I'll give your updated version a try in the morning. Maybe I merged something incorrectly.

[dangoor]

I just tried it out, and you're correct. With your updates, the Find feature works fine (I'm not sure what I did wrong!).

I have a theory about the test failure. The FindReplace tests invoke the find command and then look for the search bar. The find command calls search/FindRepace._launchFind:

    function _launchFind() {
        var editor = EditorManager.getActiveEditor();

My theory is that the old mechanism for setting the active editor worked automatically with the SpecRunnerUtils.createMockEditor function and that the new mechanism is not getting the active editor set up during the test.

[WebsiteDeveloper]

somehow that seems to be the problem but i have no idea how to fix it

[dangoor]

I think the issue is this:

        $(editor).on("focus", function () {
            _notifyActiveEditorChanged(this);
        });

That's in EditorManager._createEditorForDocument. There needs to be a way for SpecRunnerUtil to create an editor and have it properly registered with the EditorManager like that.

[WebsiteDeveloper]

@dangoor we should expose the _notifyActiveEditorChanged function for unit tests only so the createMockEditor can call this function. with this change everything works fine

[dangoor]

Yes, you're right. That was a simple fix. I've gone ahead and made the change and merged this pull request at last. Thanks for working through it with me.

[WebsiteDeveloper]

yeah i'm happy it's finally done.