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

[core-ai-bot]

Issue by WebsiteDeveloper
Thursday Jan 31, 2013 at 15:32 GMT
Originally opened as adobe/brackets#2750

---

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

---

WebsiteDeveloper included the following code: https://github.com/adobe/brackets/pull/2750/commits

[core-ai-bot]

Comment by WebsiteDeveloper
Friday Feb 01, 2013 at 20:56 GMT

---

@dangoor could you look at this

[core-ai-bot]

Comment by dangoor
Saturday Feb 02, 2013 at 03:01 GMT

---

@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.

[core-ai-bot]

Comment by dangoor
Wednesday Feb 06, 2013 at 21:38 GMT

---

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.

[core-ai-bot]

Comment by WebsiteDeveloper
Thursday Feb 07, 2013 at 16:49 GMT

---

@dangoor ready for final review

[core-ai-bot]

Comment by peterflynn
Thursday Feb 07, 2013 at 19:19 GMT

---

@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.

[core-ai-bot]

Comment by WebsiteDeveloper
Thursday Feb 07, 2013 at 19:31 GMT

---

@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

[core-ai-bot]

Comment by dangoor
Monday Feb 11, 2013 at 20:34 GMT

---

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.

[core-ai-bot]

Comment by WebsiteDeveloper
Tuesday Feb 12, 2013 at 06:53 GMT

---

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

[core-ai-bot]

Comment by dangoor
Tuesday Feb 12, 2013 at 12:35 GMT

---

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.

[core-ai-bot]

Comment by dangoor
Wednesday Feb 20, 2013 at 13:47 GMT

---

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.

[core-ai-bot]

Comment by WebsiteDeveloper
Wednesday Feb 20, 2013 at 21:03 GMT

---

@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

[core-ai-bot]

Comment by dangoor
Wednesday Feb 20, 2013 at 21:07 GMT

---

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.

[core-ai-bot]

Comment by WebsiteDeveloper
Wednesday Feb 20, 2013 at 21:15 GMT

---

interestingly it works for me with disabled cache and multiple reloads

[core-ai-bot]

Comment by dangoor
Wednesday Feb 20, 2013 at 21:28 GMT

---

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

[core-ai-bot]

Comment by dangoor
Thursday Feb 21, 2013 at 14:03 GMT

---

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.