[CLOSED] Couzteau issue1241

[core-ai-bot]

Issue by couzteau
Friday Sep 07, 2012 at 16:40 GMT
Originally opened as adobe/brackets#1583

---

Hello Brackets people,
Happy to make a stab at my first contribution. :)

All keycode references in src should be updated. All tests are passing.
I have not updated all tests to use the new module yet, but would be happy to update them soon.
Got to get to my day job ;)

Not sure if you like that I added some KeyCode definitions that are not defined here:
http://www.javascripter.net/faq/keyeventconstantsfirefox.htm

Those listed below are not defined in the , as IE deviates from other browsers. Since brackets uses them I added them following the same naming convention as other key codes. We may want to use a different prefix rather than DOM_VK? to stay consisten Happy to change if you prefer.
DOM_VK_SEMICOLON: 186,
DOM_VK_EQUALS: 187,
DOM_VK_COMMA: 188,
DOM_VK_DASH: 189

Adding them in required to remove definition defined the reference doc above:
DOM_VK_EQUALS: 61,
DOM_VK_SEMICOLON: 59,

I have not updated any key code references in sources in 3rd party or extensions.

Jochen

---

couzteau included the following code: https://github.com/adobe/brackets/pull/1583/commits

[core-ai-bot]

Comment by jasonsanjose
Friday Sep 07, 2012 at 17:54 GMT

---

Reviewing

[core-ai-bot]

Comment by jasonsanjose
Friday Sep 07, 2012 at 18:24 GMT

---

Looks like there are some additional references that use e.which instead of e.keyCode in widgets/Dialogs. Would you update those as well?

Initial review complete.

[core-ai-bot]

Comment by couzteau
Friday Sep 07, 2012 at 18:47 GMT

---

Sure - will make the changes.

[core-ai-bot]

Comment by couzteau
Friday Sep 07, 2012 at 18:49 GMT

---

Thanks for the through review - i was rushing it a bit this morning

[core-ai-bot]

Comment by couzteau
Friday Sep 07, 2012 at 20:03 GMT

---

@RaymondLim : I did think about that, but I looked it up. Turns out the original definition
per also defines the code as VK_EQUALS
http://www.javascripter.net/faq/keyeventconstantsfirefox.htm

Note - I redefined the code as the brackets uses 187.
KeyEvent.DOM_VK_EQUALS = 61

•    DOM_VK_EQUALS: 187,
  

Also, why plural here? should be VK_EQUAL.

[core-ai-bot]

Comment by RaymondLim
Monday Sep 10, 2012 at 03:07 GMT

---

@couzteau

I googled on VK_EQUAL and found out that there are two camps. So I'm okay to keep it as is.

BTW, there are some hard-coded key codes in CodeHintManager.js that you need to replace with your changes too.

[core-ai-bot]

Comment by couzteau
Monday Sep 10, 2012 at 16:09 GMT

---

Thanks Ray - I'll take a look at Code hinte manger as well as the tests and update the pull request.

[core-ai-bot]

Comment by jasonsanjose
Tuesday Sep 11, 2012 at 22:02 GMT

---

Hi Jochen. Looks like you need to sync up with master before we can merge.

[core-ai-bot]

Comment by couzteau
Tuesday Sep 11, 2012 at 22:05 GMT

---

Thanks for the info - won't get ti it before tonight.

[core-ai-bot]

Comment by jasonsanjose
Wednesday Sep 12, 2012 at 21:39 GMT

---

@couzteau please also sign the CLA before I merge this pull request http://dev.brackets.io/brackets-contributor-license-agreement.html

[core-ai-bot]

Comment by couzteau
Wednesday Sep 12, 2012 at 22:28 GMT

---

done

[core-ai-bot]

Comment by jasonsanjose
Wednesday Sep 12, 2012 at 22:48 GMT

---

Thanks. Merging.

[core-ai-bot]

Comment by couzteau
Wednesday Sep 12, 2012 at 23:02 GMT

---

Yay! Thanks -my pleasure.