Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

[OPEN] Fix #4332 and #4409: Sort issues fixed on the Working Set and Project Tree #4463

Merged
merged 5 commits into from
Jul 18, 2013
Merged

[OPEN] Fix #4332 and #4409: Sort issues fixed on the Working Set and Project Tree #4463

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a81d8c3
Sort issues fixed on the Working Set and Project Tree
TomMalbran Jul 15, 2013
9331daf
Merge with Master and fix Conflict Issue
TomMalbran Jul 17, 2013
f65acfe
Fixes after review
TomMalbran Jul 17, 2013
49c48ba
Fixes after second review
TomMalbran Jul 17, 2013
121ab4c
Fixes in getFilenameWithoutExtension
TomMalbran Jul 18, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/file/FileUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ define(function (require, exports, module) {
/**
* Get the parent directory of a file. If a directory is passed in the directory is returned.
* @param {string} fullPath full path to a file or directory
* @return {string} Returns the path to the parent directory of a file or the path of a directory
* @return {string} Returns the path to the parent directory of a file or the path of a directory
*/
function getDirectoryPath(fullPath) {
return fullPath.substr(0, fullPath.lastIndexOf("/") + 1);
Expand Down Expand Up @@ -393,6 +393,18 @@ define(function (require, exports, module) {

return baseName.substr(idx);
}

/**
* Get the file name without the extension and the file extension from a file name.
* @param {string} filename file name of a file or directory
* @return {{name: string, extension: string}} Returns the file real name and extension
*/
function getFilenameAndExtension(filename) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is only used for sorting, it might be a good idea to implement it in-place as a special string treatment instead of FileUtils method. Just a thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now is only used for sorting since I created it for it, but it could have more uses. Lots of the other functions here actually deal with string too, but they know is a path or file name and treat them as one. Moving it to StringUtils or some other place like doesn't sound good.

Maybe it would be better to rename this one as getFilenameName or something like that, that only returns the file name without the extension to make it less specific.

var extension = _getFileExtension(filename),
name = filename.replace(new RegExp("." + extension + "$"), "");

return {name: name, extension: extension};
}


// Define public API
Expand All @@ -417,4 +429,5 @@ define(function (require, exports, module) {
exports.getDirectoryPath = getDirectoryPath;
exports.getBaseName = getBaseName;
exports.getFilenameExtension = getFilenameExtension;
exports.getFilenameAndExtension = getFilenameAndExtension;
});
29 changes: 17 additions & 12 deletions src/project/ProjectManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,14 +485,18 @@ define(function (require, exports, module) {
//(note: our actual jsTree theme CSS lives in brackets.less; we specify an empty .css
// file because jsTree insists on loading one itself)
sort : function (a, b) {
var a1, b1, a2, b2;
if (brackets.platform === "win") {
// Windows: prepend folder names with a '0' and file names with a '1' so folders are listed first
var a1 = ($(a).hasClass("jstree-leaf") ? "1" : "0") + this.get_text(a).toLowerCase(),
b1 = ($(b).hasClass("jstree-leaf") ? "1" : "0") + this.get_text(b).toLowerCase();
return (a1 > b1) ? 1 : -1;
a1 = ($(a).hasClass("jstree-leaf") ? "1" : "0") + $(a).text();
b1 = ($(b).hasClass("jstree-leaf") ? "1" : "0") + $(b).text();
a2 = FileUtils.getFilenameAndExtension(a1).name;
b2 = FileUtils.getFilenameAndExtension(b1).name;
} else {
return this.get_text(a).toLowerCase() > this.get_text(b).toLowerCase() ? 1 : -1;
a2 = $(a).text();
b2 = $(b).text();
}
return a2.toLocaleLowerCase().localeCompare(b2.toLocaleLowerCase());
}
}).bind(
"before.jstree",
Expand Down Expand Up @@ -764,6 +768,15 @@ define(function (require, exports, module) {

}

/**
* Forces createNewItem() to complete by removing focus from the rename field which causes
* the new file to be written to disk
*/
function forceFinishRename() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved due to a JSLint issue (used before defined) from a previous commit.

$(".jstree-rename-input").blur();
}


/** Returns the full path to the welcome project, which we open on first launch.
* @private
* @return {!string} fullPath reference
Expand Down Expand Up @@ -824,7 +837,6 @@ define(function (require, exports, module) {
* project is loaded and tree is rendered, or rejected if the project path
* fails to load.
*/

function _loadProject(rootPath, isUpdating) {
forceFinishRename(); // in case we're in the middle of renaming a file in the project

Expand Down Expand Up @@ -1514,13 +1526,6 @@ define(function (require, exports, module) {
return result.promise();
}

/**
* Forces createNewItem() to complete by removing focus from the rename field which causes
* the new file to be written to disk
*/
function forceFinishRename() {
$(".jstree-rename-input").blur();
}

// Initialize variables and listeners that depend on the HTML DOM
AppInit.htmlReady(function () {
Expand Down
29 changes: 21 additions & 8 deletions src/project/WorkingSetSort.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@


/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*global define, $, window */
/*global define, $, window, brackets */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need brackets any more.


/**
* Manages the workingSet sort methods.
Expand All @@ -35,6 +35,7 @@ define(function (require, exports, module) {
CommandManager = require("command/CommandManager"),
DocumentManager = require("document/DocumentManager"),
PreferencesManager = require("preferences/PreferencesManager"),
FileUtils = require("file/FileUtils"),
AppInit = require("utils/AppInit"),
Strings = require("strings");

Expand Down Expand Up @@ -292,28 +293,40 @@ define(function (require, exports, module) {
function (file1, file2) {
var index1 = DocumentManager.findInWorkingSetAddedOrder(file1.fullPath),
index2 = DocumentManager.findInWorkingSetAddedOrder(file2.fullPath);

return index1 - index2;
},
"workingSetAdd workingSetAddList"
);
register(
Commands.SORT_WORKINGSET_BY_NAME,
function (file1, file2) {
return file1.name.toLocaleLowerCase().localeCompare(file2.name.toLocaleLowerCase());
var name1, name2;
if (brackets.platform === "win") {
name1 = FileUtils.getFilenameAndExtension(file1.name).name;
name2 = FileUtils.getFilenameAndExtension(file2.name).name;
} else {
name1 = file1.name;
name2 = file2.name;
}
return name1.toLocaleLowerCase().localeCompare(name2.toLocaleLowerCase());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know these three sorting functions have some differences, but I still think we can at least refactor the code here to a function as follows and share it here and the other one below.

How about moving your code here to a local function

    function _filenameSort(file1, file2) {
        var name1 = file1.name,
            name2 = file2.name;
        if (brackets.platform === "win") {
            name1 = FileUtils.getFilenameAndExtension(file1.name).name;
            name2 = FileUtils.getFilenameAndExtension(file2.name).name;
        }
        return name1.toLocaleLowerCase().localeCompare(name2.toLocaleLowerCase());
    }

And then replace the anonymous function here with _filenameSort

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I am working on doing something like this. But passing just the name and not the files, and making it a utils function in FileUtils, so I can use the same function for the project manager too.

},
"workingSetAdd workingSetAddList"
);
register(
Commands.SORT_WORKINGSET_BY_TYPE,
function (file1, file2) {
var ext1 = file1.name.split('.').pop(),
ext2 = file2.name.split('.').pop(),
cmp = ext1.localeCompare(ext2);
var name1 = FileUtils.getFilenameAndExtension(file1.name),
name2 = FileUtils.getFilenameAndExtension(file2.name),
cmpExt = name1.extension.localeCompare(name2.extension);

name1 = brackets.platform === "win" ? name1.name : file1.name;
name2 = brackets.platform === "win" ? name2.name : file2.name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you need to conditionally assign name1 and name2, I think it is better to avoid using ternary assignments. Instead, just wrap these two assignments with if (brackets.platform === "win") { } and also initialize name1 = file1.name and name2 = file2.name in their declaration. This way we can save the unnecessary assignments on Mac.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the reuse of the variables name1 and name2 wasn't the best idea. But the idea is that on windows I wasn't name to be the name key of the result of FileUtils.getFilenameAndExtension and on mac, the original file.name.

I can change it to an if, but I still need to assign the names in both branches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I just notice that you need to compare extensions first and therefore you need to call FileUtils.getFilenameAndExtension() in name1 and name2 initialization. So no need to make any changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To share the refactored code, change the anonymous function here as follows:

        function (file1, file2) {
            var ext1 = file1.name.split('.').pop(),
                ext2 = file2.name.split('.').pop(),
                cmpExt  = ext1.localeCompare(ext2); 

            if (cmpExt === 0) {
                return _filenameSort(file1, file2);
            } else {
                return cmpExt;
            }
        },

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction: Don't use the above file1.name.split('.').pop(). It is assuming that every filename always has an extension. Instead use one of the functions from FileUtils to get the correct extension. Also convert to locale lower case before comparing the extensions.


if (cmp === 0) {
return file1.name.toLocaleLowerCase().localeCompare(file2.name.toLocaleLowerCase());
if (cmpExt === 0) {
return name1.toLocaleLowerCase().localeCompare(name2.toLocaleLowerCase());
} else {
return cmp;
return cmpExt;
}
},
"workingSetAdd workingSetAddList"
Expand Down