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

TomMalbran · 2013-07-15T22:18:50Z

Fixes for #4332 and #4409.

On #4409 the issue was that the sort wasn't using the locale string functions that JavaScript Implements on the Project Tree, while I did use them on the Working Set Sort.

On #4409 the problem is that windows sort the files without the file extension, and in brackets we were sorting with the file extension. So now in windows it compares only the file names without the extension.

RaymondLim · 2013-07-16T22:23:20Z

src/file/FileUtils.js

@@ -386,4 +399,5 @@ define(function (require, exports, module) {
    exports.isStaticHtmlFileExt            = isStaticHtmlFileExt;
    exports.isServerHtmlFileExt            = isServerHtmlFileExt;
    exports.getDirectoryPath               = getDirectoryPath;
+    exports.getFileNameExtension           = getFileNameExtension;


@TomMalbran

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

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.

@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.

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.

TomMalbran · 2013-07-17T02:13:51Z

src/project/ProjectManager.js

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


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

TomMalbran · 2013-07-17T02:14:42Z

@RaymondLim Merged with master and renamed the File utils function.

busykai · 2013-07-17T04:31:36Z

src/file/FileUtils.js

+     * @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) {


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.

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.

RaymondLim · 2013-07-17T06:02:23Z

@TomMalbran

You have 3 places that require file name sorting. So I suggest that we should have it as a general filename sort function in FileUtils.js file and call it from these three places.

TomMalbran · 2013-07-17T06:12:36Z

@RaymondLim All 3 sort methods are different. There are some similarities between them, but which one are you referring to?

RaymondLim · 2013-07-17T06:23:24Z

src/project/WorkingSetSort.js

+                cmpExt = name1.extension.localeCompare(name2.extension);
+
+            name1 = brackets.platform === "win" ? name1.name : file1.name;
+            name2 = brackets.platform === "win" ? name2.name : file2.name;


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.

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.

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.

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; } },

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.

TomMalbran · 2013-07-17T08:47:57Z

@RaymondLim Fixed pushed.

RaymondLim · 2013-07-17T21:27:03Z

src/project/WorkingSetSort.js

@@ -23,7 +23,7 @@


 /*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
-/*global define, $, window */
+/*global define, $, window, brackets */


You don't need brackets any more.

TomMalbran · 2013-07-17T23:33:26Z

@RaymondLim Fixed pushed. The filenames compare function now handles comparing names and then extensions or extensions and then names.

RaymondLim · 2013-07-18T02:21:02Z

src/file/FileUtils.js

+     */
+    function _getFilenameWithoutExtension(filename) {
+        var extension = getFilenameExtension(filename);
+        return filename.replace(new RegExp(extension + "$"), "");


How about checking extension here before trimming it? If no extension, just return filename without calling replace.

Sure. It should replace nothing with nothing if there is no extension, but we can avoid it.

RaymondLim · 2013-07-18T02:33:40Z

Done with re-review. Just one more comment that I should have caught in earlier reviews.

TomMalbran · 2013-07-18T02:46:43Z

@RaymondLim Fix done, plus a JSDoc that wasn't completely updated.

RaymondLim · 2013-07-18T03:01:59Z

Looks good, thanks. Merging.

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

Sort issues fixed on the Working Set and Project Tree

a81d8c3

ghost assigned RaymondLim Jul 16, 2013

RaymondLim reviewed Jul 16, 2013
View reviewed changes

Merge with Master and fix Conflict Issue

9331daf

TomMalbran reviewed Jul 17, 2013
View reviewed changes

busykai reviewed Jul 17, 2013
View reviewed changes

RaymondLim reviewed Jul 17, 2013
View reviewed changes

Fixes after review

f65acfe

RaymondLim reviewed Jul 17, 2013
View reviewed changes

Fixes after second review

49c48ba

RaymondLim reviewed Jul 18, 2013
View reviewed changes

Fixes in getFilenameWithoutExtension

121ab4c

RaymondLim added a commit that referenced this pull request Jul 18, 2013

Merge pull request #4463 from TomMalbran/issues-4332-4409

ace6a51

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

RaymondLim merged commit ace6a51 into adobe:master Jul 18, 2013

TomMalbran deleted the issues-4332-4409 branch July 18, 2013 03:04

julieyuan mentioned this pull request Jul 19, 2013

[Core][Untitled files][Win only]: Incorrect sort order for untitled files. #4409

Closed

core-ai-bot mentioned this pull request Aug 29, 2021

[CLOSED] [OPEN] Fix #4332 and #4409: Sort issues fixed on the Working Set and Project Tree brackets-archive/bracketsIssues#4134

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TomMalbran commented Jul 15, 2013

RaymondLim Jul 16, 2013

TomMalbran Jul 16, 2013

busykai Jul 17, 2013

TomMalbran Jul 17, 2013

TomMalbran Jul 17, 2013

TomMalbran commented Jul 17, 2013

busykai Jul 17, 2013

TomMalbran Jul 17, 2013

RaymondLim commented Jul 17, 2013

TomMalbran commented Jul 17, 2013

RaymondLim Jul 17, 2013

TomMalbran Jul 17, 2013

RaymondLim Jul 17, 2013

RaymondLim Jul 17, 2013

RaymondLim Jul 17, 2013

TomMalbran commented Jul 17, 2013

RaymondLim Jul 17, 2013

TomMalbran commented Jul 17, 2013

RaymondLim Jul 18, 2013

TomMalbran Jul 18, 2013

RaymondLim commented Jul 18, 2013

TomMalbran commented Jul 18, 2013

RaymondLim commented Jul 18, 2013

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

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

Conversation

TomMalbran commented Jul 15, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomMalbran commented Jul 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaymondLim commented Jul 17, 2013

TomMalbran commented Jul 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomMalbran commented Jul 17, 2013

Choose a reason for hiding this comment

TomMalbran commented Jul 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaymondLim commented Jul 18, 2013

TomMalbran commented Jul 18, 2013

RaymondLim commented Jul 18, 2013