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
Show file tree
Hide file tree
Changes from all commits
Commits
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
37 changes: 36 additions & 1 deletion src/file/FileUtils.js
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,40 @@ define(function (require, exports, module) {

return baseName.substr(idx);
}

/**
* @private
* Get the file name without the extension.
* @param {string} filename File name of a file or directory
* @return {string} Returns the file name without the extension
*/
function _getFilenameWithoutExtension(filename) {
var extension = getFilenameExtension(filename);
return extension ? filename.replace(new RegExp(extension + "$"), "") : filename;
}

/**
* Compares 2 filenames in lowercases. In Windows it compares the names without the
* extension first and then the extensions to fix issue #4409
* @param {string} filename1
* @param {string} filename2
* @param {boolean} extFirst If true it compares the extensions first and then the file names.
* @return {number} The result of the local compare function
*/
function compareFilenames(filename1, filename2, extFirst) {
var ext1 = getFilenameExtension(filename1),
ext2 = getFilenameExtension(filename2),
cmpExt = ext1.toLocaleLowerCase().localeCompare(ext2.toLocaleLowerCase()),
cmpNames;

if (brackets.platform === "win") {
filename1 = _getFilenameWithoutExtension(filename1);
filename2 = _getFilenameWithoutExtension(filename2);
}
cmpNames = filename1.toLocaleLowerCase().localeCompare(filename2.toLocaleLowerCase());

return extFirst ? (cmpExt || cmpNames) : (cmpNames || cmpExt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice implementation!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Kind of hard to read and get it, but it looks nice and simple.

}


// Define public API
Expand All @@ -417,4 +451,5 @@ define(function (require, exports, module) {
exports.getDirectoryPath = getDirectoryPath;
exports.getBaseName = getBaseName;
exports.getFilenameExtension = getFilenameExtension;
exports.compareFilenames = compareFilenames;
});
30 changes: 16 additions & 14 deletions src/project/ProjectManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -485,14 +485,15 @@ 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 = $(a).text(),
b1 = $(b).text();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you replace this.get_text() with $().text()? I know both work the same way, just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They aren't the same this.get_text() is closer to $().html() since it returns the content of the node with the html. I noticed that it was returning name<span class='extension'>.js</span> and I needed just name.js.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, good to know they're not the same.


// Windows: prepend folder names with a '0' and file names with a '1' so folders are listed first
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;
} else {
return this.get_text(a).toLowerCase() > this.get_text(b).toLowerCase() ? 1 : -1;
a1 = ($(a).hasClass("jstree-leaf") ? "1" : "0") + a1;
b1 = ($(b).hasClass("jstree-leaf") ? "1" : "0") + b1;
}
return FileUtils.compareFilenames(a1, b1, false);
}
}).bind(
"before.jstree",
Expand Down Expand Up @@ -764,6 +765,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 +834,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 +1523,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
14 changes: 4 additions & 10 deletions src/project/WorkingSetSort.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,29 +293,22 @@ 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());
return FileUtils.compareFilenames(file1.name, file2.name, false);
},
"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);

if (cmp === 0) {
return file1.name.toLocaleLowerCase().localeCompare(file2.name.toLocaleLowerCase());
} else {
return cmp;
}
return FileUtils.compareFilenames(file1.name, file2.name, true);
},
"workingSetAdd workingSetAddList"
);
Expand Down