[CLOSED] More Editor Option Handlers Tests

[core-ai-bot]

Issue by TomMalbran
Saturday Mar 23, 2013 at 21:36 GMT
Originally opened as adobe/brackets#3231

---

I added tests for show line numbers and auto close brackets and refactored the code to reduce the code repetition. The difference is all wrong, since I added new indentation and new helper functions, but the functionality is all the same as before for the older tests.

---

TomMalbran included the following code: https://github.com/adobe/brackets/pull/3231/commits

[core-ai-bot]

Comment by jbalsas
Thursday Apr 04, 2013 at 21:31 GMT

---

Reviewing

[core-ai-bot]

Comment by jbalsas
Friday Apr 05, 2013 at 01:00 GMT

---

@TomMalbran Done with review. Looks good overall, and nice refactors out there ;)

I'll have some sleep time, so don't go rushing to fix everything!

[core-ai-bot]

Comment by TomMalbran
Friday Apr 05, 2013 at 03:53 GMT

---

@jbalsas Thanks again for the review! I think I covered all your comments.

I'll need some explaining here, as I'd expect to have var myContent = "This is awesome!";[ after typing [ with auto-close-braces off. Is this because of how codemirror or the addon handles the keypresses?

I am not sure why it doesn't add the key. I do know that the function that handles the keypresses in CodeMirror searches through the keymaps and applies the function that it finds, so that makes the auto close braces work, but I do not know why it does nothing when it doesn't find a function. I still haven't find how CodeMirror handles the inputs, and I did tried several simulation and this is the one that works better.

[core-ai-bot]

Comment by jbalsas
Friday Apr 05, 2013 at 18:30 GMT

---

@TomMalbran Re-review done, just two minor comments and we can merge this.

I'm still not happy that strange missing bracket character issue... I'll try to spend some time on if afterwards.

[core-ai-bot]

Comment by TomMalbran
Sunday Apr 07, 2013 at 03:03 GMT

---

I think passing the editor as a parameter gives us better control, and keeps the helper methods API clean, but at this point I feel like we're dancing around for some little gain.

Done, and since all the test were running a bit slowing as they had to open an close several windows, I managed to make them run all in the same window, which makes it faster and nicer, but let me know if there is any problem with this.

I'm still not happy that strange missing bracket character issue... I'll try to spend some time on if afterwards.

I think the problem here is not with the code but is the fact that the event simulation creates a new event that can be handled with a listener but won't make a key input in a textarea or input field. So the text isn't added to the textarea that CodeMirror uses, but CodeMirror does listen to the event and calls the addon to it's job.

[core-ai-bot]

Comment by jbalsas
Sunday Apr 07, 2013 at 10:16 GMT

---

Thanks for working on this. Some of the runs could probably be removed, but we could address that in a later cleanup if desired.

Looks good! Merging.

[core-ai-bot]

Comment by TomMalbran
Sunday Apr 07, 2013 at 10:45 GMT

---

Great, thanks.

Which runs are you referring to? If is the ones that check the current selection, I tried removing them, but couldn't, since it was getting executed before the check functions where run and it then expected 0 to be the given position.

[core-ai-bot]

Comment by jbalsas
Sunday Apr 07, 2013 at 10:48 GMT

---

I was thinking of the ones inside the check... methods, since nothing should have async issues in there anymore, but it's just a minor nit.

[core-ai-bot]

Comment by TomMalbran
Sunday Apr 07, 2013 at 10:52 GMT

---

I see. I am planning on add an option to toggle the auto close tags, and add tests, so I might add it there if everything works without them.