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
RaymondLim 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 1 commit
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
18 changes: 16 additions & 2 deletions src/file/FileUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,13 +359,26 @@ define(function (require, exports, module) {

/**
* Get the parent directory of a file. If a directory is passed in the directory is returned.
* @param {string} 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
* @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
*/
function getDirectoryPath(fullPath) {
return fullPath.substr(0, fullPath.lastIndexOf("/") + 1);
}

/**
* 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 getFileNameExtension(filename) {
Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Jul 16, 2013

Choose a reason for hiding this comment

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

You may need to rename this api since this is already taken by pull request #4379. Renaming it to getFilenameAndExtension or splitFilenameAndExtension may be better.

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

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


// Define public API
exports.LINE_ENDINGS_CRLF = LINE_ENDINGS_CRLF;
exports.LINE_ENDINGS_LF = LINE_ENDINGS_LF;
Expand All @@ -386,4 +399,5 @@ define(function (require, exports, module) {
exports.isStaticHtmlFileExt = isStaticHtmlFileExt;
exports.isServerHtmlFileExt = isServerHtmlFileExt;
exports.getDirectoryPath = getDirectoryPath;
exports.getFileNameExtension = getFileNameExtension;
Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Jul 16, 2013

Choose a reason for hiding this comment

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

@TomMalbran

This api is already taken by this pull request #4379 that I landed in master yesterday.

Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Jul 16, 2013

Choose a reason for hiding this comment

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

I just saw it. Although it doesn't do what I need here. I want to retrieve from a file name (not a path), both the file name without the extension and the extension. So I still need that function, but will need to rename it.

By checking #4379, it looks like getFilenameExtension does the same as _getFileExtension. Also a FileEntry has the file base name stored on it. So not sure why getBaseName is required. I could make a new issue for this 2 things.

Copy link
Copy Markdown
Contributor

@busykai busykai Jul 17, 2013

Choose a reason for hiding this comment

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

@TomMalbran #4379 was my pull request. You're right, getFilenameExtension could re-use _getFileExtension. Haven't noticed it. getFilenameExtension should return extension including . (See the usage). It's due to how PathUtils behaved. It has to be fixed.

On your FileEntry comment: that one I actually considered. However, getting the base name is a string operation (see the usage included in the same pull request). Creating an object each time one needs to parse a string is not a good idea.

Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Jul 17, 2013

Choose a reason for hiding this comment

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

Ok, didn't noticed that one returned the "." and the other didn't. But yes, merging them or having one use the other might be good.

About the other one, I actually checked the uses, and in several places you already have a FileEntry to work with, but there is a place where you don't. Could be made possible to receive a FileEntry, but anyway, i guess it can be useful to have it.

});
12 changes: 8 additions & 4 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.getFileNameExtension(a1).name;
b2 = FileUtils.getFileNameExtension(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
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
Copy Markdown
Contributor

@RaymondLim RaymondLim Jul 17, 2013

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.getFileNameExtension(file1.name).name;
name2 = FileUtils.getFileNameExtension(file2.name).name;
} else {
name1 = file1.name;
name2 = file2.name;
}
return name1.toLocaleLowerCase().localeCompare(name2.toLocaleLowerCase());
Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Jul 17, 2013

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
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Jul 17, 2013

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.getFileNameExtension(file1.name),
name2 = FileUtils.getFileNameExtension(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
Copy Markdown
Contributor

@RaymondLim RaymondLim Jul 17, 2013

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
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Jul 17, 2013

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
Copy Markdown
Contributor

@RaymondLim RaymondLim Jul 17, 2013

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
Copy Markdown
Contributor

@RaymondLim RaymondLim Jul 17, 2013

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
Copy Markdown
Contributor

@RaymondLim RaymondLim Jul 17, 2013

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
⚡