Fixes handling of CSS file name to theme name

[MiguelCastillo]

Fixed issue with regex not properly handling dot in file names without extensions

Here is a link to test the regex http://regex101.com/r/iI6oQ0/2

[MiguelCastillo]

#8673

cc @dangoor @marcelgerber

[marcelgerber]

You can remove both the options (g and i).
And I'm not quite sure, should this only replace the last extension (like .min .css) or all (like .min .css)?

[peterflynn]

There's already a FileUtils._getFilenameWithoutExtension() utility... maybe we should just make that public and use it here?

[marcelgerber]

This method only removes the last extension, while I still think we should remove all the extensions.
A regex like

fileName.replace(/\..+$/, "");

could do that.

[peterflynn]

@MiguelCastillo Why is it that not stripping the extension breaks raw CodeMirror themes? Or did you just mean (in #8673) that the display in the dropdown is uglier? (Seems like the answer to @marcelgerber's question sort of depends on why the extension was a problem in the first place...)

[MiguelCastillo]

@peterflynn So, here is a quick rundown. When we process themes as a extensions, there is a package.json with a name field. This is the general use case. But for people that want to extend the functionality in ThemeManager, I ended up exposing the loadFile interface, and the only way to give the theme a name is via the filename.

So, the theme name itself is what's used as the css class that wraps the entire theme. So, loading themes from CodeMirror wouldn't work because the class name would contain the extension.

For example, /~https://github.com/adobe/CodeMirror2/blob/master/theme/ambiance.css would generate a class ambiance-css, which breaks the theme because CodeMirror is expecting it to be ambiance.

[MiguelCastillo]

@peterflynn You are right, we should just expose _getFilenameWithoutExtension.

[marcelgerber]

@MiguelCastillo It looks like CodeMirror expects a class of cm-s-ambiance on the .CodeMirror div, doesn't it?

[MiguelCastillo]

Yup, it expects that. How themes work in CM is that you give CM the name of the theme you want to set as the theme... For example you tell CM to set its theme to ambiance. CM will in turn add the class cm-s-ambiance to the .CodeMirror div. So, without the regex to strip out the extension, we are telling CM to load ambiance-css, which causes the css rules in ambiance.css to not be matched; because CM will add cm-s-ambiance-css.

So, I misspoke. We don't wrap the entire theme with the name of the class. CM handles that.

[MiguelCastillo]

I guess the part of confusion which I am having trouble articulating is that we tell CM to load the name of a theme, and CM will take the name and automatically prefix it with cm-s-. So, this is a bit unintuitive because the theme name is not exactly what you see in the style file.

So, when I say CM expects that I am really saying that it is expecting that to be in the style file. E.g. ambiance.css will have rules with cm-s-ambiance in it. But the actual value you are passing to CM is ambiance. If we pass ambiance-css, then CM will basically expect cm-s-ambiance-css.

[MiguelCastillo]

@peterflynn Yes this PR is to strip off the extension to generate "correct" theme names from style files. I will add a comment to explain that. Also, This PR is also to compliment #8673 because I didn't escape the . file extension delimiter. :-/ As far as the check for valid extensions goes... I added that to be bullet proof so that we only strip off things that we know we can. If the file does not have a css/less extension, it does not necessarily mean that it is not a proper style file. Honestly, that's just to deal with people that shouldn't be allowed near computers... So, I am OK just removing that extra check and not worry about extreme edge cases. It cleans out the code.

@marcelgerber .min in a file name is not an extension or part of the extension... And since CM themes convention is that the file name is what's used for the name of the theme, I can only safely remove the proper file extension.

[MiguelCastillo]

@peterflynn I added some more comments around the whole file extension treatment... I also exposed the getFilenameWithoutExtension so that we don't use a regex.

[marcelgerber]

You need to remove the @private JSDoc then.

[marcelgerber]

nit: Should be name (singular)

[dangoor]

@MiguelCastillo I think that @marcelgerber's point about .min.css is reasonable, though likely makes no difference in practice if no one minifies their CSS for CodeMirror themes. It's easy enough to guard against:

options.name = FileUtils.getFilenameWithoutExtension(fileName)
                          .replace(/\.min$/, "");

...but this is an API that has few consumers (your Themes extension being one of them), so I leave it to you to decide if it's worth handling that special case.

[MiguelCastillo]

@dangoor It's not very common to minify theme files, but it can only help if we handled such cases.

[MiguelCastillo]

Should I rename the PR to some a less obscure?

[dangoor]

Looks good. Merging.

[MiguelCastillo]

Thanks @dangoor!