Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from Mar 5, 2013

Conversation

@WebsiteDeveloper
Copy link
Contributor

commented Jan 31, 2013

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

@ghost ghost assigned dangoor Jan 31, 2013

return new Editor(doc, makeMasterEditor, mode, container, range);
$(editor).on("focus", _notifyActiveEditorChanged());

This comment has been minimized.

Copy link
@peterflynn

peterflynn Jan 31, 2013

Member

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

This comment has been minimized.

Copy link
@peterflynn

peterflynn Jan 31, 2013

Member

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

This comment has been minimized.

Copy link
@WebsiteDeveloper

WebsiteDeveloper Jan 31, 2013

Author Contributor

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

This comment has been minimized.

Copy link
@dangoor

dangoor Feb 6, 2013

Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2013

@dangoor could you look at this

@dangoor

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2013

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2013

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2013

@dangoor ready for final review

@peterflynn

This comment has been minimized.

Copy link
Member

commented Feb 7, 2013

@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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2013

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2013

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2013

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

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2013

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

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2013

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2013

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2013

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2013

interestingly it works for me with disabled cache and multiple reloads

@dangoor

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2013

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

@dangoor

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2013

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2013

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

@dangoor

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2013

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2013

@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 dangoor merged commit d14d7a8 into adobe:master Mar 5, 2013
1 check passed
1 check passed
default The Travis build passed
Details
@dangoor

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2013

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2013

yeah i'm happy it's finally done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.