[CLOSED] Add theme option addModeClass (for language-aware themes)

[core-ai-bot]

Issue by MarcelGerber
Saturday Nov 29, 2014 at 11:15 GMT
Originally opened as adobe/brackets#10039

---

Adds the theme option addModeClass, which enables the CM option addModeClass if set.
When enabled, themes can apply styles language-aware with rules like .cm-m-javascript.cm-variable.
CMs addModeClass uses innerMode for class name determination, so modes like htmlmixed, php or gfm work correctly, too.

See also #8579.
cc@MiguelCastillo

---

MarcelGerber included the following code: https://github.com/adobe/brackets/pull/10039/commits

[core-ai-bot]

Comment by TomMalbran
Saturday Nov 29, 2014 at 17:31 GMT

---

This is great. But does it needs to be an option? Does it slow things or make is something that users might want it not active.

With this active by default we can fix issues like #2842 and fix the syntax styles for other too, so I think that we should just have it always enabled.

[core-ai-bot]

Comment by MarcelGerber
Saturday Nov 29, 2014 at 18:13 GMT

---

I haven't tested it yet, but I think it will make CM performance worse as it applies many classes (namely to every single token).
Also, I don't think we should have mode styles in Brackets core stylesheets as they're hard to override.

[core-ai-bot]

Comment by TomMalbran
Saturday Nov 29, 2014 at 18:32 GMT

---

Wait, is an option added from the theme in the package.json. That seems fine then, since the theme author knows if it uses it or not. I was thinking that it was an option added for the users.

We will still need to use this on the light default theme. But I guess that we can do that on the theme extension.

[core-ai-bot]

Comment by MiguelCastillo
Monday Dec 01, 2014 at 15:22 GMT

---

Without specific metrics, I am only speculating that performance won't be noticeably impacted.

I am curious to see a theme to test this puppy out though. Particularly a theme that has mixed mode with html, javascript, and css which is why we didn't move forward with my initial attempt at supporting this. I will adjust my visual studio theme, which is what I use to test this.

@MarcelGerber if you can also get another theme started to test this, I will be happy to help you where needed. It can be just a repo in your account and we can install from the URL

[core-ai-bot]

Comment by MarcelGerber
Monday Dec 01, 2014 at 15:29 GMT

---

Yup, I'm thinking the same about performance now, but I think it should stay opt-in, as when a theme doesn't need it, it's pretty much useless.
I've actually never done a theme myself, but I'm interested in seeing this in action, too. So I guess I'll just take a theme I like (I won't do one from ground up) and apply some language-specific styles.

[core-ai-bot]

Comment by MarcelGerber
Monday Dec 01, 2014 at 15:58 GMT

---

So I've just created a "Custom Lion" theme based on@larz0's Lion theme I kind of like.
@larz0 is definitely better at creating themes, but that's not what it's all about. I tried it in a htmlmixed doc and it worked ;)
Gist: https://gist.github.com/MarcelGerber/f6bddd17cbc86516083c
Screen:

You can see the HTML tags (like <style>) and the CSS tags (like h1) are styled differently.

[core-ai-bot]

Comment by MiguelCastillo
Monday Dec 01, 2014 at 19:31 GMT

---

@MarcelGerber Awesome it works! I haven't had a chance to test it out... Will do that a bit later today.

As far as the setting goes... Putting it in the package.json is not really making it opt-in... It is just making it more difficult if you want to enable/disable it. Opt-in means that it is disabled by default and you can opt-in to enable the functionality.

There should be a way to disable this globally and I can also see this being a setting in brackets.json. I just really don't think that enabling/disabling CodeMirror functionality should go in a package.json. If there is a bug in the CodeMirror feature, you are now stuck forcing every user to change a package.json in each theme, and unless you go through each package.json you will not know which theme is affected. -- Too cumbersome.

[core-ai-bot]

Comment by TomMalbran
Tuesday Dec 02, 2014 at 03:04 GMT

---

I think that we should check the performance first. If there is no difference or is not noticeable, I this that this should be enabled by default on the core. Beside performance I don't see why someone would disable this. If there is an issue with codemirror, that should be fixed on the core, and not have the users change the preference to fix an issue. Not every codemirror option needs to be a preference.

There are already syntax highlight issues, in some languages with the core themes that could be fixed with this change, and people want a fix for that.

[core-ai-bot]

Comment by MiguelCastillo
Tuesday Dec 02, 2014 at 03:28 GMT

---

@TomMalbran Yup, not every CodeMirror option needs to be a Brackets preferences, not what I meant. My point is that if we are going to provide a way to disable a CodeMirror feature, it shouldn't be done with a flag in the extension package.json.

I also don't really see a reason why users would want to disable this feature anyways. Not sure we would want to make it an option -- unless there are performance side effects.

[core-ai-bot]

Comment by TomMalbran
Tuesday Dec 02, 2014 at 03:33 GMT

---

@MiguelCastillo I don't think that is a way for users to disable the preference. Is a way for theme authors that want to use different styles for different languages to enable it. Depending on the performance, this might or not work. If performance doesn't matter, I don't see why we need to make theme authors enable it if they want it. If performance matters, maybe users should be able to disable it.

[core-ai-bot]

Comment by MiguelCastillo
Wednesday Dec 03, 2014 at 01:13 GMT

---

@MarcelGerber have you had a chance to run any performance tests on this yet?

[core-ai-bot]

Comment by redmunds
Wednesday Dec 03, 2014 at 01:17 GMT

---

Start by looking at the DOM generated by CM. I seriously doubt that it applies the mode class "to every single token" as stated above. The mode class only needs to be applied to the root element of the mode, so the performance impact should be negligible.

[core-ai-bot]

Comment by MiguelCastillo
Wednesday Dec 03, 2014 at 01:26 GMT

---

@redmunds it does actually apply the style classes to roughly each tokens... Which is how CM can cleanly handle mixed mode. I will run some tests either tonight or tomorrow and report back.

[core-ai-bot]

Comment by redmunds
Wednesday Dec 03, 2014 at 01:31 GMT

---

Yeah, I guess the free form DOM structure makes it hard to create selectors in Themes otherwise.

[core-ai-bot]

Comment by MarcelGerber
Sunday Dec 07, 2014 at 15:18 GMT

---

So, I just managed to get a timeline of enabling the CM option addModeClass (in CEF 2171, btw):

I used codemirror.js (in the Brackets repo) as a test file, which consists of 7923 lines. But the line count actually doesn't matter much, as CM only renders the currently visible lines (in the test, there were only 433 tokens in the DOM).
I also checked scrolling performance and got a ~3% worse result.

Then, I checked jquery-2.1.1.min.js, a minimized file with 12139 tokens in the DOM: