[CLOSED] Fix code hint provider selection

[core-ai-bot]

Issue by iwehrman
Thursday Mar 21, 2013 at 23:17 GMT
Originally opened as adobe/brackets#3208

---

Commit 5878608 replaced a number of instances of $.each with Array.forEach. But those functions are not interchangeable: only the former supports early break-out. Both the CodeHintManager and the CodeHintList relied on this feature. In particular, that commit broke:

1. CodeHintManager provider selection, by always choosing the last provider instead of stopping at the first, highest-priority provider

2. the CodeHintList, by adding all the available hints instead of of just the first maxResults hints.

The first issue also breaks Edge Web Fonts in particular.

This pull reverts just those two changes. I haven't checked all the other replacements from that commit to see if there are other related problems. (Want to have a look,@TomMalbran?)

---

iwehrman included the following code: https://github.com/adobe/brackets/pull/3208/commits

[core-ai-bot]

Comment by TomMalbran
Thursday Mar 21, 2013 at 23:22 GMT

---

Oh, I didn't checked the break in the $.each, sorry. You could still use Array.some instead of Array.forEach and break the loop returning true instead of false as with $.each.

There is another $.each that was broken in PreferenceManager, but that one doesn't really needs the break and is using CollectionUtils.forEach. I could fix it in #3117 using CollectionUtils.some.

[core-ai-bot]

Comment by jbalsas
Friday Mar 22, 2013 at 08:25 GMT

---

@TomMalbran Don't worry, I also missed this on review :(

An important issue here, then, is that we don't have this covered with any test. It would be nice to add one now, so this doesn't slip again.

[core-ai-bot]

Comment by redmunds
Friday Mar 22, 2013 at 18:04 GMT

---

I verified this fixed Edge Web Fonts. Merging.

[core-ai-bot]

Comment by peterflynn
Friday Mar 22, 2013 at 18:36 GMT

---

@iwehrman I think it's worth filing a spinoff bug to wrap up the loose ends mentioned above: switching this to Array.some() (achieving the goals of the cleanup #3087); and adding a unit test (especially since it breaks a prominent extension and isn't something we'd be able to spot with pure Brackets scenario testing).

[core-ai-bot]

Comment by iwehrman
Friday Mar 22, 2013 at 19:17 GMT

---

See #3223 regarding unit tests for the CodeHintManager. I'm not sure replacing $.each() with Array.some() is worth spending any time on. What's so bad about $.each()?

[core-ai-bot]

Comment by peterflynn
Friday Mar 22, 2013 at 20:04 GMT

---

We've standardized on using Array.forEach()/.some() instead, and it's what all other code in the product uses (in general we prefer to use modern browser APIs where available). $.each() uses args in the opposite order those other APIs, making it bug-prone to mix them. $.each() is also buggy for iterating object keys, so I'd worry a bit that continuing to use it for (some) arrays would encourage people to erroneously use it for objects too.