[CLOSED] Big bag o' minor code cleanups & JSLint fixes

[core-ai-bot]

Issue by peterflynn
Monday Nov 05, 2012 at 23:16 GMT
Originally opened as adobe/brackets#2058

---

• Tweaks to docs in various files: clarifications & added/fixed type annotations.
• Fix lots of JSLint errors in CSSUtils-test.
• Remove unused dependencies in WorkingSetView.
• Fix grammar in TokenUtils log message.

Shouldn't contain any functional changes, so hopefully this is pretty safe to merge... though it never hurts to double-check the accuracy of the docs first :-)

---

peterflynn included the following code: https://github.com/adobe/brackets/pull/2058/commits

[core-ai-bot]

Comment by TomMalbran
Monday Nov 05, 2012 at 23:28 GMT

---

Hi. Just saw the comment on WorkingSetSort and WorkingSetReorder events.

I am using the workingSetReorder listener inside the sort workingSet functionality to disable the automatic sort. If automatic sort is enable and a user drags and drops to manually sort files, it will disable the automatic sort, since it will undo the changes manually made by the user when the next file is added.

So to combine them we might need to add additional information to the event to know when is a drag and drop sort and when isn't, just to support the previous case.

I could do that later if is better having both events combined.

[core-ai-bot]

Comment by redmunds
Tuesday Nov 06, 2012 at 00:39 GMT

---

Done with review. 1 minor comment.

[core-ai-bot]

Comment by peterflynn
Tuesday Nov 06, 2012 at 07:52 GMT

---

@redmunds: changes pushed.

@TomMalbran: I think the current WorkingSetSort code might still work as-is if the two events were merged, because all sorting it does is bracketed by _removeListeners()/_addListeners() calls. So it would ignore its own sorting changes but turn off automatic sorting if anything else caused a sort event.

[core-ai-bot]

Comment by TomMalbran
Tuesday Nov 06, 2012 at 10:16 GMT

---

@peterflynn actually the real sort is done after _addListeners() when this.sort() is executed and will trigger the event. There is also the case when the automatic sort triggers a sort and it then only executes this.sort() without adding or removing listeners.

[core-ai-bot]

Comment by redmunds
Tuesday Nov 06, 2012 at 16:03 GMT

---

@peterflynn I don't like the idea of having an event mean different things depending on when it's triggered. That makes the code harder to understand and ripe for future breakage. Are additional events that much overhead? If so, then we'll need to pass additional info with the combined event.

@TomMalbran With that said, I think the events could be named better. The existing names sound like synonyms to me. Maybe "workingSetSortAll" and "workingSetSortOne" ?

So if it's OK like it is let's remove the TODO comment now, otherwise this will have to be done in a future pull request.

[core-ai-bot]

Comment by TomMalbran
Tuesday Nov 06, 2012 at 19:15 GMT

---

@peterflynn I think it might still be possible to merge them by moving the content of the Sort.prototype.sort inside the _removeListeners()/_addListeners() calls of Sort.prototype.execute and use Sort.prototype.execute when the automatic sort is triggered. This would make the automatic sort do stuff that is not needed, but it wouldn't break the current implementation. Doing this will also make it behave as you mentioned.

@redmunds Renaming like that would be fine to.

[core-ai-bot]

Comment by peterflynn
Thursday Nov 08, 2012 at 07:49 GMT

---

@redmunds,@TomMalbran: I filed #2076 for the issue about the events so we can unblock this pull request. I left the TODO in (with a reference to the new bug #) as a warning that the event names might change in the near future.

In the same commit, also added some comment tidying in Resizer. This resulted in spinning off another TODO-linked bug, #2079.

@redmunds, let me know if you think this needs any other changes?

[core-ai-bot]

Comment by redmunds
Thursday Nov 08, 2012 at 18:44 GMT

---

Looks good. Merging.