-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Big bag o' minor code cleanups & JSLint fixes #2058
Conversation
annotations. Fix lots of JSLint errors in CSSUtils-test. Remove unused dependencies in WorkingSetView.
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. |
@@ -182,7 +182,7 @@ define(function (require, exports, module) { | |||
} | |||
|
|||
forceMargins(elementSize); | |||
EditorManager.resizeEditor(); | |||
EditorManager.resizeEditor(); // V resize affects editor directly; H resize could change ht of top toolbar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems long enough that it should be put on a separate line above this line of code. That will also give you more space to expand abbreviations V, H, and ht.
Same comment applies to 2 other places in this file.
Done with review. 1 minor comment. |
@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. |
@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. |
@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. |
@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. |
@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? |
Looks good. Merging. |
Big bag o' minor code cleanups & JSLint fixes
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 :-)