Fixes handling of CSS file name to theme name #8677

Merged
merged 5 commits into from Aug 7, 2014

Projects

None yet

4 participants

@MiguelCastillo
Member

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

@MarcelGerber
Member

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 peterflynn and 1 other commented on an outdated diff Aug 6, 2014
src/view/ThemeManager.js
@@ -83,7 +83,7 @@ define(function (require, exports, module) {
// the name of the file to be unique
this.file = file;
- this.name = options.name || (options.title || fileName.replace(/.[\w]+$/gi, '')).toLocaleLowerCase().replace(/[\W]/g, '-');
+ this.name = options.name || (options.title || fileName.replace(/\.[\w]+$/gi, '')).toLocaleLowerCase().replace(/[\W]/g, '-');
@peterflynn
peterflynn Aug 6, 2014 Member

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

@MarcelGerber
MarcelGerber Aug 6, 2014 Member

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
Member

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

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

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

@MarcelGerber
Member

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

@MiguelCastillo
Member

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
Member

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
Member

@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 MiguelCastillo Exposed getFilenameWithoutExtension
Also added appropriate comments to explain why we need to remove the
file extension when the filename is used as the name of the theme
3b12b48
@MiguelCastillo
Member

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

You need to remove the @private JSDoc then.

@MarcelGerber MarcelGerber commented on an outdated diff Aug 7, 2014
src/view/ThemeManager.js
- // Portip: If no options.name or options.title is provided, then we will rely solely on
- // the name of the file to be unique
+ // If no options.name is provided, then we derive the name of the theme from whichever we find
+ // first, the options.title or the filename.
+ if (!options.name) {
+ if (options.title) {
+ options.name = options.title;
+ } else {
+ // Remove the file extension when the filename is used as the theme name. This is to
+ // follow CodeMirror conventions where themes are just a CSS file and the filename
+ // (wihtout the extension) is used to build CSS rules.
+ options.name = FileUtils.getFilenameWithoutExtension(fileName);
+ }
+
+ // We do a bit of string treatment here to make sure we generate theme names that can be
+ // used as a CSS class names by CodeMirror.
@MarcelGerber
MarcelGerber Aug 7, 2014 Member

nit: Should be name (singular)

@MarcelGerber MarcelGerber commented on an outdated diff Aug 7, 2014
src/view/ThemeManager.js
@@ -79,11 +79,25 @@ define(function (require, exports, module) {
options = options || {};
var fileName = file.name;
- // Portip: If no options.name or options.title is provided, then we will rely solely on
- // the name of the file to be unique
+ // If no options.name is provided, then we derive the name of the theme from whichever we find
+ // first, the options.title or the filename.
+ if (!options.name) {
+ if (options.title) {
+ options.name = options.title;
+ } else {
+ // Remove the file extension when the filename is used as the theme name. This is to
+ // follow CodeMirror conventions where themes are just a CSS file and the filename
+ // (wihtout the extension) is used to build CSS rules.
@MarcelGerber
MarcelGerber Aug 7, 2014 Member

typo: without

@dangoor dangoor self-assigned this Aug 7, 2014
@dangoor
Contributor
dangoor commented Aug 7, 2014

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

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

@MiguelCastillo
Member

Should I rename the PR to some a less obscure?

@dangoor
Contributor
dangoor commented Aug 7, 2014

Looks good. Merging.

@MiguelCastillo
Member

Thanks @dangoor!

@dangoor dangoor changed the title from Fixed issue with regex not properly handling dot in file names without e... to Fixes handling of CSS file name to theme name Aug 7, 2014
@dangoor dangoor merged commit bcf91c5 into adobe:master Aug 7, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment