Merge pull request #3080 from TomMalbran/tom/fix-issue-2076

redmunds · redmunds · commit 358b264440e5 · 2013-06-14T15:26:18.000-07:00
Fix #2076: Two indistinguishable events for different cases of working set reordering
diff --git a/src/document/DocumentManager.js b/src/document/DocumentManager.js
@@ -70,9 +70,10 @@
  *      2nd arg to the listener is the removed FileEntry.
  *    - workingSetRemoveList -- When multiple files are removed from the working set (e.g. project close).
  *      The 2nd arg to the listener is the array of removed FileEntry objects.
- *    - workingSetReorder -- When the indexes of 2 files are swapped. Listener receives no arguments.
- *    - workingSetSort -- When the workingSet array is sorted. Listener receives no arguments.
- *      TODO (#2076): combine workingSetSort & workingSetReorder since they convey nearly identical information.
+ *    - workingSetSort -- When the workingSet array is sorted. Notifies the working set view to redraw
+ *      the new sorted list. Listener receives no arguments.
+ *    - workingSetDisableAutoSorting -- When working set is manually re-sorted via dragging and dropping
+ *      a file to disable automatic sorting. Listener receives no arguments.
  *
  *    - fileNameChange -- When the name of a file or folder has changed. The 2nd arg is the old name.
  *      The 3rd arg is the new name.
@@ -343,9 +344,7 @@ define(function (require, exports, module) {
             temp = _workingSet[index1];
             _workingSet[index1] = _workingSet[index2];
             _workingSet[index2] = temp;
-            
-            // Dispatch event
-            $(exports).triggerHandler("workingSetReorder");
+            $(exports).triggerHandler("workingSetDisableAutoSorting");
         }
     }
     
@@ -362,6 +361,7 @@ define(function (require, exports, module) {
         $(exports).triggerHandler("workingSetSort");
     }
     
+    
     /**
      * Indicate that changes to currentDocument are temporary for now, and should not update the MRU
      * ordering of the working set. Useful for next/previous keyboard navigation (until Ctrl is released)
diff --git a/src/project/WorkingSetSort.js b/src/project/WorkingSetSort.js
@@ -37,11 +37,11 @@ define(function (require, exports, module) {
         PreferencesManager      = require("preferences/PreferencesManager"),
         AppInit                 = require("utils/AppInit"),
         Strings                 = require("strings");
-        
+    
     var defaultPrefs = {
-            currentSort:   Commands.SORT_WORKINGSET_BY_ADDED,
-            automaticSort: false
-        };
+        currentSort:   Commands.SORT_WORKINGSET_BY_ADDED,
+        automaticSort: false
+    };
     
     /**
      * @private
@@ -76,10 +76,21 @@ define(function (require, exports, module) {
     
     /**
      * Retrieves a Sort object by id
-     * @param {string} commandID
-     * @return {Sort}
+     * @param {(string|Command)} command A command ID or a command object.
+     * @return {?Sort}
      */
-    function get(commandID) {
+    function get(command) {
+        var commandID;
+        if (!command) {
+            console.error("Attempting to get a Sort method with a missing required parameter: command");
+            return;
+        }
+        
+        if (typeof command === "string") {
+            commandID = command;
+        } else {
+            commandID = command.getID();
+        }
         return _sorts[commandID];
     }
     
@@ -90,75 +101,46 @@ define(function (require, exports, module) {
         return _automaticSort;
     }
     
-    
-    /**
-     * @private
-     * Sets the value of Automatic Sort and updates the menu item.
-     * @param {boolean} value
-     */
-    function _setAutomatic(value) {
-        _automaticSort = value;
-        _prefs.setValue("automaticSort", _automaticSort);
-        CommandManager.get(Commands.SORT_WORKINGSET_AUTO).setChecked(_automaticSort);
-    }
-    
     /**
      * @private
-     * Removes current sort DocumentManager listeners.
+     * Removes the sort DocumentManager listeners.
      */
     function _removeListeners() {
-        if (_currentSort && _currentSort.getEvents()) {
-            $(DocumentManager).off(".sort");
-        }
+        $(DocumentManager).off(".sort");
     }
     
     /**
-     * @private
-     * Disables Automatic Sort.
+     * Enables/Disables Automatic Sort depending on the value.
+     * @param {boolean} enable True to enable, false to disable.
      */
-    function _disableAutomatic() {
-        _setAutomatic(false);
-        _removeListeners();
+    function setAutomatic(enable) {
+        _automaticSort = enable;
+        _prefs.setValue("automaticSort", _automaticSort);
+        CommandManager.get(Commands.SORT_WORKINGSET_AUTO).setChecked(_automaticSort);
+        
+        if (enable) {
+            _currentSort.sort();
+        } else {
+            _removeListeners();
+        }
     }
     
     /**
      * @private
-     * Adds current sort DocumentManager listeners.
+     * Adds the current sort DocumentManager listeners.
      */
     function _addListeners() {
         if (_automaticSort && _currentSort && _currentSort.getEvents()) {
             $(DocumentManager)
-                .on(_currentSort.getEvents(), function (event) {
-                    _currentSort.callAutomaticFn(event);
+                .on(_currentSort.getEvents(), function () {
+                    _currentSort.sort();
                 })
-                .on("workingSetReorder.sort", function () {
-                    _disableAutomatic();
+                .on("workingSetDisableAutoSorting.sort", function () {
+                    setAutomatic(false);
                 });
         }
     }
     
-    /**
-     * @private
-     * Enables Automatic Sort.
-     */
-    function _enableAutomatic() {
-        _setAutomatic(true);
-        _addListeners();
-        _currentSort.callAutomaticFn();
-    }
-    
-    /**
-     * Enables/Disables Automatic Sort depending on the value.
-     * @param {boolean} value
-     */
-    function setAutomatic(value) {
-        if (value) {
-            _enableAutomatic();
-        } else {
-            _disableAutomatic();
-        }
-    }
-    
     
     /**
      * @private
@@ -190,87 +172,88 @@ define(function (require, exports, module) {
      * @constructor
      * @private
      *
-     * @param {!string} commandID - valid command identifier.
-     * @param {!function(FileEntry, FileEntry)} compareFn - a valid sort function (see register for a longer explanation).
-     * @param {!string} events - space-separated DocumentManager possible events ending on ".sort".
-     * @param {!function($.Event)} automaticFn - the function that will be called when an automatic sort event is triggered.
+     * @param {string} commandID A valid command identifier.
+     * @param {function(FileEntry, FileEntry): number} compareFn A valid sort
+     *      function (see register for a longer explanation).
+     * @param {string} events Space-separated DocumentManager possible events
+     *      ending with ".sort".
      */
     function Sort(commandID, compareFn, events, automaticFn) {
-        this._commandID   = commandID;
-        this._compareFn   = compareFn;
-        this._events      = events;
-        this._automaticFn = automaticFn;
+        this._commandID = commandID;
+        this._compareFn = compareFn;
+        this._events    = events;
     }
     
-    /** @return {CommandID} */
+    /** @return {string} The Command ID */
     Sort.prototype.getCommandID = function () {
         return this._commandID;
     };
     
-    /** @return {CompareFn} */
+    /** @return {function(FileEntry, FileEntry): number} The compare function */
     Sort.prototype.getCompareFn = function () {
         return this._compareFn;
     };
     
-    /** @return {Events} */
+    /** @return {string} The DocumentManager events */
     Sort.prototype.getEvents = function () {
         return this._events;
     };
     
-    /** Calls automaticFn */
-    Sort.prototype.callAutomaticFn = function (event) {
-        return this._automaticFn(event);
-    };
-    
     /**
-     * Performs the sort and makes it the current sort method
+     * Performs the sort and makes it the current sort method.
      */
     Sort.prototype.execute = function () {
-        _removeListeners();
         _setCurrentSort(this);
-        _addListeners();
         this.sort();
     };
     
     /**
-     * Only performs the working set sort if this is the current sort
+     * Only performs the working set sort if this is the current sort.
      */
     Sort.prototype.sort = function () {
         if (_currentSort === this) {
+            _removeListeners();
             DocumentManager.sortWorkingSet(this._compareFn);
+            _addListeners();
         }
     };
     
     
     /**
      * Registers a working set sort method.
-     * @param {!string} commandID - valid command identifier used for the sort method.
-     *      Core commands in Brackets use a simple command title as an id, for example "open.file".
-     *      Extensions should use the following format: "author.myextension.mycommandname". 
-     *      For example, "lschmitt.csswizard.format.css".
-     * @param {!function(FileEntry, FileEntry)} compareFn - the function that will be used inside JavaScript's
-     *      sort function. The return a value should be >0 (sort a to a lower index than b), =0 (leaves a and b
-     *      unchanged with respect to each other) or <0 (sort b to a lower index than a) and must always returns
-     *      the same value when given a specific pair of elements a and b as its two arguments.
-     *      Documentation: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Array/sort
-     * @param {?string} events - one or more space-separated event types that DocumentManger uses.
-     *      Each event passed will trigger the automatic sort. If no events are passed, the automatic
-     *      sort will be disabled for that sort method.
-     * @param {?function($.Event)} automaticFn - the function that will be called when an automatic sort
-     *      event is triggered. If no function is passed the automatic sort will just call the sort function.
+     * @param {(string|Command)} command A command ID or a command object
+     * @param {function(FileEntry, FileEntry): number} compareFn The function that
+     *      will be used inside JavaScript's sort function. The return a value
+     *      should be >0 (sort a to a lower index than b), =0 (leaves a and b
+     *      unchanged with respect to each other) or <0 (sort b to a lower index
+     *      than a) and must always returns the same value when given a specific
+     *      pair of elements a and b as its two arguments. Documentation at:
+     *      https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Array/sort
+     * @param {?string} events One or more space-separated event types that
+     *      DocumentManger uses. Each event passed will trigger the automatic
+     *      sort. If no events are passed, the automatic sort will be disabled
+     *      for that sort method.
      * @return {?Sort}
      */
-    function register(commandID, compareFn, events, automaticFn) {
-        if (_sorts[commandID]) {
-            console.log("Attempting to register an already-registered sort: " + commandID);
+    function register(command, compareFn, events) {
+        var commandID = "";
+        
+        if (!command || !compareFn) {
+            console.log("Attempting to register a Sort method with a missing required parameter: command or compare function");
             return;
         }
-        if (!commandID || !compareFn) {
-            console.log("Attempting to register a sort with a missing id, or compare function: " + commandID);
+        if (typeof command === "string") {
+            commandID = command;
+        } else {
+            commandID = command.getID();
+        }
+        
+        if (_sorts[commandID]) {
+            console.log("Attempting to register an already-registered Sort method: " + command);
             return;
         }
         
-        // Adds ".sort" to the end of each event to make them specific for the automatic sort
+        // Adds ".sort" to the end of each event to make them specific for the automatic sort.
         if (events) {
             events = events.split(" ");
             events.forEach(function (event, index) {
@@ -279,11 +262,7 @@ define(function (require, exports, module) {
             events = events.join(" ");
         }
         
-        automaticFn = automaticFn || function (event) {
-            _currentSort.sort();
-        };
-        
-        var sort = new Sort(commandID, compareFn, events, automaticFn);
+        var sort = new Sort(commandID, compareFn, events);
         _sorts[commandID] = sort;
         return sort;
     }
@@ -362,7 +341,7 @@ define(function (require, exports, module) {
             _setCurrentSort(curSort);
         }
         if (autoSort) {
-            _enableAutomatic();
+            setAutomatic(true);
         }
         if (curSort && autoSort) {
             curSort.sort();
diff --git a/src/project/WorkingSetView.js b/src/project/WorkingSetView.js
@@ -244,14 +244,16 @@ define(function (require, exports, module) {
                 } else {
                     CommandManager.execute(Commands.FILE_CLOSE, {file: $listItem.data(_FILE_KEY)});
                 }
-            } else if (moved) {
+            
+            } else {
+                // Update the file selection
                 if (selected) {
-                    // Update the file selection
                     _fireSelectionChanged();
                     ViewUtils.scrollElementIntoView($openFilesContainer, $listItem, false);
                 }
+                
+                // Restore the shadow
                 if (addBottomShadow) {
-                    // Restore the shadows
                     ViewUtils.addScrollerShadow($openFilesContainer[0], null, true);
                 }
             }
@@ -544,7 +546,6 @@ define(function (require, exports, module) {
      */
     function _handleWorkingSetSort() {
         _rebuildWorkingSet(true);
-        _scrollSelectedDocIntoView();
     }
 
     /** 

Original file line number	Diff line number	Diff line change
`@@ -244,14 +244,16 @@ define(function (require, exports, module) {`
`244`	`244`	`} else {`
`245`	`245`	`CommandManager.execute(Commands.FILE_CLOSE, {file: $listItem.data(_FILE_KEY)});`
`246`	`246`	`}`
`247`		`- } else if (moved) {`
	`247`	`+`
	`248`	`+ } else {`
	`249`	`+ // Update the file selection`
`248`	`250`	`if (selected) {`
`249`		`- // Update the file selection`
`250`	`251`	`_fireSelectionChanged();`
`251`	`252`	`ViewUtils.scrollElementIntoView($openFilesContainer, $listItem, false);`
`252`	`253`	`}`
	`254`	`+`
	`255`	`+ // Restore the shadow`
`253`	`256`	`if (addBottomShadow) {`
`254`		`- // Restore the shadows`
`255`	`257`	`ViewUtils.addScrollerShadow($openFilesContainer[0], null, true);`
`256`	`258`	`}`
`257`	`259`	`}`
`@@ -544,7 +546,6 @@ define(function (require, exports, module) {`
`544`	`546`	`*/`
`545`	`547`	`function _handleWorkingSetSort() {`
`546`	`548`	`_rebuildWorkingSet(true);`
`547`		`- _scrollSelectedDocIntoView();`
`548`	`549`	`}`
`549`	`550`
`550`	`551`	`/**`