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

Issue #4409: Incorrect sort order for untitled files #4524

Merged
merged 3 commits into from
Jul 22, 2013

Conversation

TomMalbran
Copy link
Contributor

This PR fixes the second issue mentioned in #4409 by comparing the numbers when the filenames are the same for the exception of the number at the end of the string.

@RaymondLim I was able to get it done today.

@ghost ghost assigned RaymondLim Jul 20, 2013
@RaymondLim
Copy link
Contributor

Thanks @TomMalbran

You're so quick! I was about to suggest localeCompare with options argument. That is,

cmpNames = filename1.toLocaleLowerCase().localeCompare(filename2.toLocaleLowerCase(), "en", {numeric: true});

But localeCompare function with optional 2nd and 3rd arguments are not supported in all browsers. So we may need to check for browser support as in this https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare#Example:_Checking_for_support_for_locales_and_options_arguments example and use it only if we have the browser support (required for in-browser version of Brackets).

@TomMalbran
Copy link
Contributor Author

I never knew that localeCompare could have more arguments. This works in Chrome, so it will work inside brackets, but will fail once we go to the browser. Maybe is ok to have the previous sorting in FireFox until this is supported by FireFox? I don't think it should take them too long to support it.

I just checked in the debugger tools and using:
cmpNames = filename1.toLocaleLowerCase().localeCompare(filename2.toLocaleLowerCase(), undefined, {numeric: true});
Will work better.

@RaymondLim
Copy link
Contributor

@TomMalbran
I know that you may want to keep your implementation for other browsers, but your solution (based on my suggestion) is still not correct. I know it's my mistake to assume that numbers are always suffix to the filenames. You can try out with filenames that have numbers as prefix (eg. 7untitled.js and 12untitled.js), and realize that our solution does not work.

So I think we should switch to use options argument if it is supported by the browser, and keep using the original localeCompare with one argument for browsers like FireFox.

@TomMalbran
Copy link
Contributor Author

@RaymondLim Actually I prefer to use the options in the localeCompare. Since is already implemented is better to use it. It is also a visual fix, nothing is really broken if we use the old option.

We would still need to use it as I posted it, with undefined as the locale, so that is works for any locale and not just Enlgish. Since JavaScript ditches the extra parameters passed to a function, it works in FireFox passing the options. It just returns the result as if there wasn't options. We can just use he function with the options for every browser.

@RaymondLim
Copy link
Contributor

I totally agree with you to use undefined for locale argument. I used "en" just for a quick testing and not sure that it can take undefined when I posted my comment.

@TomMalbran
Copy link
Contributor Author

The second and third argument are optional, so when they are not given, they get the value of undefined. I also tested it and it works.

@TomMalbran
Copy link
Contributor Author

@RaymondLim updated the fix using the options parameter.

@RaymondLim
Copy link
Contributor

Your changes look good! But we still need two more changes.

  1. It seems like the original issue in [Core][Untitled files][Win only]: Incorrect sort order for untitled files. #4409 was fixed when we switched to localeCompare() and we don't need the if (brackets.platform === "win") { ...} any more. So try it out without it and if you don't see the original issue, then please remove it.
  2. We may need to change the other localeCompare() call with options argument for consistency. This is necessary if the user has custom extensions (like .j7 and .j12), then we need to sort it the same way as we sort by name.

@TomMalbran
Copy link
Contributor Author

  1. We kind of still need the if (brackets.platform === "win") { ...} since without it Untitled.js will appear after Untitled-1.js. But since we don't add extensions after the Untitled files anymore, this doesn't show when generating them. Is still a problem when files have extensions.
  2. Fixed

@RaymondLim
Copy link
Contributor

You're right that we still need your if (brackets.platform === "win") { ...}. I was testing w/o extensions and thought we don't need it.

@RaymondLim
Copy link
Contributor

Looks good. Merging.

RaymondLim added a commit that referenced this pull request Jul 22, 2013
Issue #4409: Incorrect sort order for untitled files
@RaymondLim RaymondLim merged commit fc20756 into adobe:master Jul 22, 2013
@TomMalbran TomMalbran deleted the tom/issue-4409 branch July 22, 2013 23:40
@TomMalbran
Copy link
Contributor Author

Great. Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants