User-configurable whitespace #31

le717 · 2015-01-14T04:50:53Z

For #26. Adds a preference so whitespace can be recolored by the user. Supersedes #30.

Implementation preferences layout

"denniskehrig.ShowWhitespace.colors": {
    "light": {
        "empty": "#ccc",
        "leading": "#ccc",
        "trailing": "#ff0000",
        "whitespace": "#ccc"
    },
    "dark": {
        "empty": "#686963",
        "leading": "#686963",
        "trailing": "#ff0000",
        "whitespace": "#686963"
    }
}

Notes

Requires Brackets v0.42 and higher
Does not overwrite colors defined by themes. This can be both good and bad.
Need good default values for the built-in Brackets dark theme.

le717 · 2015-01-14T04:52:18Z

main.js

+        var userColors = _preferences.get("colors"),
+            template = _.template(stylesTemplate),
+            css = template({
+              "whitespace": userColors.light.whitespace,


For whatever reason, this one key is not being rendered in both light and dark, but all the others work. I've checked the resulting CSS and it is all valid, so it's odd why it is not working.

le717 · 2015-01-16T16:07:57Z

@DennisKehrig (When you get back,) can you fill me in on why the stylesheet is added/removed to/from the DOM on extension toggle? Was there an historical reason for this or a design choice, and why?

@MiguelCastillo Can you recommend a theme that styles the whitespace this extension creates so I can test if this PR will overwrite that styling?

MiguelCastillo · 2015-01-16T16:15:57Z

I don't know a theme that does it as it is generally not common to add classes defined by other extensions. But I can create one for you later this evening.

le717 · 2015-01-16T17:23:49Z

Well what do you know? The Monokai theme styles the whitespace. I can use that for testing.Thanks anyway. :)

le717 · 2015-01-17T05:51:39Z

Miguel, does the whitespace key (.CodeMirror .cm-dk-whitespace-space:before, CodeMirror .cm-dk-whitespace-tab:before) render for you? For whatever reason, that one style in both light and dark does not work, but the others do. Am I doing the Lo-Dash template incorrectly or something? The compiled CSS is correct, but Brackets never renders that one part.

le717 · 2015-01-17T05:54:55Z

What I am talking about can easily been seen in this image:

There should be dots between the variable names and equals signs (as well as equal and value), but clearly, there is not.

le717 · 2015-02-12T01:04:51Z

@MiguelCastillo Except for colors, this is ready to be merged. Could you perhaps do a code review soon, keeping in mind the following points?

This is completely unable to override colors defined by the (select few) themes. Is this good or bad?
Using two stylesheets is probably not ideal. I could have the colors rendered into main.less then compile the LESS using ExtensionUtils.parseLessCode() and inject it using ExtensionUtils.addEmbeddedStyleSheet(), but this would make any color updates performed by the user follow the same process, and that could be slow. Are there any other ways to merge the stylesheets?

No real rush, though I would like to get this in soon so @DennisKehrig can release whenever he gets back. :)

le717 · 2015-03-05T01:44:51Z

Hey @larz0? I hate to ask you this, but I can't think of anyone else. Would you mind too terribly coming up with some complementary colors for the whitespace that match the built-in Brackets dark theme? There are only two colors: trailing whitespace (which is red in the light theme) and everything else (a light gray in light theme). :)

MiguelCastillo · 2015-03-05T01:46:22Z

@le717 OMG! dude I am really sorry, I saw this and somehow completely forgot to get back to you! I will take a look so that we can merge this :)

le717 · 2015-03-05T02:03:45Z

@MiguelCastillo No problem man, I'm terribly busy myself lately. I'm just going back to my open PRs, trying to get them finished up so they don't go stale. :)

le717 · 2015-03-18T20:54:10Z

Miguel, can you please take a look at this really soon? This really needs to be finished up so other pending changes and improvements can be made. :)

larz0 · 2015-03-18T21:00:22Z

@le717 sorry I missed this as well somehow. Looks good 👍

MiguelCastillo · 2015-03-19T02:24:44Z

.brackets.json

@@ -0,0 +1,4 @@
+{
+    "spaceUnits": 4,


Should this file be checked in?

I think I added this to help keep a basic consistent code style across the files. I'll probably pull it and submit a new PR with it and an equivalent .editorconfig file.

MiguelCastillo · 2015-03-19T02:55:21Z

The code looks good. The only real question I have is around the asymmetry when you are registering/unregistering the event handler for themes properties. e.g. PreferencesManager.getExtensionPrefs("themes").on("change", updateColors);

All else looks good to me. :)

le717 · 2015-03-19T04:19:51Z

@larz0 Thanks! :)

le717 · 2015-03-19T04:23:37Z

@MiguelCastillo All feedback addressed, per pending comments. :)

I do have a problem with the listeners, and this is a not a new issue, but it is heightened with the new theme listeners. Despite the presence of the unload() function that tears down and properly closes everything, as far as I can tell that function is never called. I can't seem to log any instance of it or the functions it calls being run when the extension is disabled. Unless it is an implicit run by Brackets and any logs are suppressed, the listeners leak until the program is closed or restarted. Obviously, that is not good. This is not the PR to fix that issue, but it is something I thought I should bring up.

MiguelCastillo · 2015-03-19T13:34:41Z

main.js

-    var CodeMirror = brackets.getModule("thirdparty/CodeMirror2/lib/codemirror");
+    var _                  = brackets.getModule("thirdparty/lodash"),
+        AppInit            = brackets.getModule("utils/AppInit"),
+        CodeMirror         = brackets.getModule("thirdparty/CodeMirror2/lib/codemirror"),


CodeMirror is an unused variable. I don't know how I missed this one earlier ;)

Huh. I thought it was still needed after dropping CM2. Removed.

MiguelCastillo · 2015-03-19T14:14:21Z

This is completely unable to override colors defined by the (select few) themes. Is this good or bad?
- I think it is good for themes to be able to override colors specified by this extension. The reason is that theme author can style to their heart's content to match the color scheme in the theme.
Using two stylesheets is probably not ideal. I could have the colors rendered into main.less then compile the LESS using ExtensionUtils.parseLessCode() and inject it using ExtensionUtils.addEmbeddedStyleSheet(), but this would make any color updates performed by the user follow the same process, and that could be slow. Are there any other ways to merge the stylesheets?
- I don't really think the performance concern is a big deal because it's not an operation that happens often or repeatedly. I think that having one file is good, either a less or _.template. I would say that _.template would be more performant as the parsing rules are much simpler than less.

MiguelCastillo · 2015-03-19T14:18:49Z

main.js

-    function onCheckedStateChange() {
-        _preferences.set("checked", Boolean(_command.getChecked()));
+    function onCheckedStateChange(e) {
+        _preferences.set("checked", e.target._checked);


This change seems odd... It seems like if there was a regression it would be due to a timing issue where command.getChecked() isn't set when the callback is invoked. Hopefully this will go away when your PR for updating the events is merged.

This is what happens when you do late night coding. 😛 Found the real cause and reverted this.

MiguelCastillo · 2015-03-19T14:20:15Z

The only concern I have is the regression:

function onCheckedStateChange() {

   _preferences.set("checked", Boolean(_command.getChecked()));

function onCheckedStateChange(e) {

   _preferences.set("checked", e.target._checked);

Otherwise this is ready to be merged.

MiguelCastillo · 2015-03-19T20:06:04Z

Yeah man, the changes look great!

MiguelCastillo · 2015-03-19T20:14:29Z

main.js

+            compiled = template(_preferences.get("colors"));
+
+        // Update the styling with the changes
+        _styleInline.text(compiled);


This method is so much tighter! It looks great

le717 · 2015-03-19T20:15:00Z

All review changes and feedback read and addressed. If it still looks good, I'll flatten this and merge it right in!

MiguelCastillo · 2015-03-19T20:17:07Z

main.js

+     */
+    function _applyColors() {
+        // Compile the CSS from the template
+        var template = _.template(stylesTemplate),


You can create this _.template when the extension loads so that the template does not get parsed each time. This routine will basically become:

// Update the styling with the changes _styleInline.text(template(_preferences.get("colors")));

You're catching everything, aren't you? :D

Of course! I don't like half @ssing things :D

le717 · 2015-03-19T20:37:30Z

I must have not copied my last changes for testing, because with them, I'm getting an whitespace is not defined error, when that key is present (I checked). :\

le717 · 2015-03-19T20:48:32Z

OK, forget that, suddenly it works. The final changes to _applyColors() have been made. I'm going to run this code for this rest of the day to ensure nothing comes up and I'll merge it tonight. If you see anything, let me know, and thank you for taking the time to review this! :)

MiguelCastillo · 2015-03-19T20:49:33Z

Sweet, the code looks really clean :) I like it. And you are very welcome! :)

Add user-configurable whitespace colors

MiguelCastillo · 2015-03-20T01:48:01Z

nice work @le717 !

le717 reviewed Jan 14, 2015
View reviewed changes

le717 force-pushed the colors-config branch from 9d25042 to 2e6ef3d Compare January 14, 2015 04:57

This was referenced Jan 14, 2015

User-configurable whitespace #30

Closed

Changed whitespace and tab color #24

Closed

le717 added the enhancement label Jan 22, 2015

le717 mentioned this pull request Jan 25, 2015

Update events to use v1.1 system #32

Merged

le717 force-pushed the colors-config branch from 0f4dbb7 to 498a6e8 Compare February 7, 2015 21:33

le717 mentioned this pull request Feb 11, 2015

Lodash 3.x support #34

Closed

le717 force-pushed the colors-config branch from 498a6e8 to fbe84f7 Compare February 11, 2015 02:57

le717 mentioned this pull request Mar 18, 2015

fix deprecated jQuery.fn.init on/off() #35

Closed

MiguelCastillo reviewed Mar 19, 2015
View reviewed changes

le717 mentioned this pull request Mar 19, 2015

All listeners are leaky #36

Closed

MiguelCastillo reviewed Mar 19, 2015
View reviewed changes

Implement user-configurable colors (#26)

4a46924

MiguelCastillo reviewed Mar 19, 2015
View reviewed changes

le717 force-pushed the colors-config branch from 746b607 to 4a46924 Compare March 19, 2015 20:17

Only parse the template once

5ce642a

le717 force-pushed the colors-config branch from 0aa8d2f to 5ce642a Compare March 19, 2015 20:51

le717 pushed a commit that referenced this pull request Mar 20, 2015

Merge pull request #31 from DennisKehrig/colors-config

a9ec742

Add user-configurable whitespace colors

le717 merged commit a9ec742 into master Mar 20, 2015

le717 deleted the colors-config branch March 20, 2015 00:12

le717 mentioned this pull request Mar 20, 2015

Allow users to change the color of the whitespace #26

Closed

le717 mentioned this pull request Apr 8, 2015

Opacity #40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User-configurable whitespace #31

User-configurable whitespace #31

le717 commented Jan 14, 2015

le717 Jan 14, 2015

le717 commented Jan 16, 2015

MiguelCastillo commented Jan 16, 2015

le717 commented Jan 16, 2015

le717 commented Jan 17, 2015

le717 commented Jan 17, 2015

le717 commented Feb 12, 2015

le717 commented Mar 5, 2015

MiguelCastillo commented Mar 5, 2015

le717 commented Mar 5, 2015

le717 commented Mar 18, 2015

larz0 commented Mar 18, 2015

MiguelCastillo Mar 19, 2015

le717 Mar 19, 2015

MiguelCastillo commented Mar 19, 2015

le717 commented Mar 19, 2015

le717 commented Mar 19, 2015

MiguelCastillo Mar 19, 2015

le717 Mar 19, 2015

MiguelCastillo commented Mar 19, 2015

MiguelCastillo Mar 19, 2015

le717 Mar 19, 2015

MiguelCastillo commented Mar 19, 2015

MiguelCastillo commented Mar 19, 2015

MiguelCastillo Mar 19, 2015

le717 commented Mar 19, 2015

MiguelCastillo Mar 19, 2015

le717 Mar 19, 2015

MiguelCastillo Mar 19, 2015

le717 commented Mar 19, 2015

le717 commented Mar 19, 2015

MiguelCastillo commented Mar 19, 2015

MiguelCastillo commented Mar 20, 2015

User-configurable whitespace #31

User-configurable whitespace #31

Conversation

le717 commented Jan 14, 2015

Implementation preferences layout

Notes

Choose a reason for hiding this comment

le717 commented Jan 16, 2015

MiguelCastillo commented Jan 16, 2015

le717 commented Jan 16, 2015

le717 commented Jan 17, 2015

le717 commented Jan 17, 2015

le717 commented Feb 12, 2015

le717 commented Mar 5, 2015

MiguelCastillo commented Mar 5, 2015

le717 commented Mar 5, 2015

le717 commented Mar 18, 2015

larz0 commented Mar 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCastillo commented Mar 19, 2015

le717 commented Mar 19, 2015

le717 commented Mar 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCastillo commented Mar 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCastillo commented Mar 19, 2015

MiguelCastillo commented Mar 19, 2015

Choose a reason for hiding this comment

le717 commented Mar 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

le717 commented Mar 19, 2015

le717 commented Mar 19, 2015

MiguelCastillo commented Mar 19, 2015

MiguelCastillo commented Mar 20, 2015