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

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

Merged
RaymondLim merged 3 commits into
adobe:masterfrom
TomMalbran:tom/issue-4409
Jul 22, 2013
Merged

RaymondLim merged 3 commits into
adobe:masterfrom
TomMalbran:tom/issue-4409

Conversation

@TomMalbran

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

@RaymondLim updated the fix using the options parameter.

@RaymondLim

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants