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

Correctly selects filename when known extension with a dot inside is used #7242

Merged
merged 4 commits into from
Mar 24, 2014

Conversation

zaggino
Copy link
Contributor

@zaggino zaggino commented Mar 18, 2014

Follow up for #7209 in case we wanted to get fancy :-)

This is for #7265

var indexOfExtension = escapedName.lastIndexOf('.');

var indexOfExtension = escapedName.lastIndexOf("."),
language = LanguageManager.getLanguageForPath(escapedName);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should use escapedName here - it could lead to issues. Better use entry.name again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the escaped one will be shown in the input so that's the one you need to be looking for indexOf, not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I get it, use original just for the language.

@redmunds
Copy link
Contributor

@zaggino This looks good, but it does not fix a case I was expecting.

This works for a file named test.html.erb, but not test.php.js. That's because the file extension "html.erb" is explicity defined in languages.json, but "php.js" is not. "html.erb" is explicitly defined because it has to be -- it doesn't fit the accepted convention -- the files should be named test.erb.html. We don't want to have to list every combination of "php.*" (and other server-side markup languages) in languages.json -- we should automatically handle them.

I think this can be done, without getting tricked by file names such as jquery-1.10.2.min.js, by also trying to match each known file extension part. For the test.php.js case, working backwards, it would check "js", then "php". Of course, it would also have to check "php.js" to handle the "html.erb" case. For the jquery-1.10.2.min.js case it would see "js" is known, but stop searching when it hits "min".

Does that make sense? Care to give it a try?

@zaggino
Copy link
Contributor Author

zaggino commented Mar 21, 2014

Sure, I want to finish this.

  1. Do we want to have any combinations of valid extensions at the end or just some specific ones? e.g. php.js vs css.js

  2. Do we want to allow just two or any number of extensions? e.g. php.html.css

@redmunds
Copy link
Contributor

I think allowing any number of extensions in any combination would be most flexible.

@zaggino
Copy link
Contributor Author

zaggino commented Mar 22, 2014

Review again please @redmunds - added also tests for things mentioned above.

@redmunds
Copy link
Contributor

This works great!

One more thing I notice is that the project tree highlighting still only recognizes the file extension the old way while file renaming recognizes file extension the new way:

file-extension-selection

The project tree highlighting is done in ViewUtils.getFileEntryDisplay(entry). Just need to replace lastIndexOf with new API call.

@zaggino
Copy link
Contributor Author

zaggino commented Mar 24, 2014

Done :)
image

@redmunds
Copy link
Contributor

Looks great!

But, I'm seeing this JSLint unit test fail in your branch, but not in master:

"should default to the editor's indent"

Error: Expected true to be false.
    at new jasmine.ExpectationResult (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:102:32)
    at null.toBe (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1194:29)
    at toggleJSLintResults (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/src/extensions/default/JSLint/unittests.js:43:57)
    at null.<anonymous> (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/src/extensions/default/JSLint/unittests.js:107:17)
    at jasmine.Block.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1024:15)
    at jasmine.Queue.next_ (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1842:31)
    at onComplete (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:1838:18)
    at jasmine.WaitsForBlock.execute (file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2322:5)
    at file:///C:/Users/redmunds/dev/github/brackets-shell/Release/dev/test/thirdparty/jasmine-core/jasmine.js:2336:12

Seems like this is more related to your previous pull request. Are you seeing that failure?

@redmunds
Copy link
Contributor

Oh, it's because your change to JSLint has not been merged into this branch.

Looks good. Merging.

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

3 participants