tinted-theming / base16-emacs

Base16 themes for Emacs
MIT License
382 stars 77 forks source link

The code for ansi colors is not working. #25

Closed glennehrlich closed 8 years ago

glennehrlich commented 8 years ago

Near the bottom of base16-theme.el there's code for setting ansi-color-names-vector and ansi-term-color-vector. This code does not work. The following fix makes it work. Note that this is what the older base16 code did as well (the difference is in ` vs ' for the quoted lists and using , to unquote the list items).

`(ansi-color-names-vector
       ;; black, base08, base0B, base0A, base0D, magenta, cyan, white
       [,base00 ,base08 ,base0B ,base0A ,base0D ,base0E ,base0D ,base05])
     `(ansi-term-color-vector
       ;; black, base08, base0B, base0A, base0D, magenta, cyan, white
       [unspecified ,base00 ,base08 ,base0B ,base0A ,base0D ,base0E ,base0D ,base05])
belak commented 8 years ago

The difference between the old code and the new code is that the whole thing isn't wrapped in a macro, it's in a function, so the quoting is a bit different.

Anyway, I'll definitely fix this, but is there an easy way to test that the fix worked?

I've tried checking with term and ansi-term and the current code seems to work... unless there's something I'm doing wrong there.

glennehrlich commented 8 years ago

Thanks for the quick response!

You can test it with M-x shell and then do an ‘ls' in a directory that you know has some directories and normal files. When one of the base16 themes is active (I use base16-chalk), dirs will be be colored vs files (in fact when nothing was colored was how I found this error and then did the fix in the issue and it fixed it).

Depending on what kind of system you test on, you’ll have to make sure that your ‘ls' command is producing colors. You’ll want to force it to use the actual executable instead of going through an alias.

On mac: $ /bin/ls -G

On linux: $ /bin/ls —color # that’s 2 dashes or hyphens

And I think the refactorization in the new base16 is good. Very clean to support new faces as they become popular.

On Sep 6, 2016, at 2:35 PM, Kaleb Elwert notifications@github.com wrote:

The difference between the old code and the new code is that the whole thing isn't wrapped in a macro, it's in a function, so the quoting is a bit different.

Anyway, I'll definitely fix this, but is there an easy way to test that the fix worked?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/belak/base16-emacs/issues/25#issuecomment-245103008, or mute the thread https://github.com/notifications/unsubscribe-auth/AMIZ7FODSGXR8nuaBvief5ngWdnOT-5qks5qndyNgaJpZM4J13PS.

belak commented 8 years ago

Maybe I'm doing something wrong... I've tried with eshell, shell, term and ansi-term and what's currently there appears to work...

screen shot 2016-09-06 at 4 05 33 pm
glennehrlich commented 8 years ago

First off, ansi-term works correctly for me with your code. But ansi-term is deferring to bash to colorize.

It does not work for me using normal M-x shell, which is my concern.

Here’s some screen shots with the difference.

With your code, notice no coloring and in the ielm buffer note that ansi-color-names vector only has the symbols base00, base08, etc. The example let is showing what’s going on.

Now with my edited version, it works correctly. Note that ansi-color-names-vector gets the values base00, base08 (and not the symbols themselves). The example let shows that using the backtick and comma makes the list get the values instead of the symbols as in the first example.

On Sep 6, 2016, at 4:06 PM, Kaleb Elwert notifications@github.com wrote:

Maybe I'm doing something wrong... I've tried with eshell, shell, term and ansi-term and what's currently there appears to work...

https://cloud.githubusercontent.com/assets/107097/18294100/c98fdd76-744b-11e6-91dc-f843a0fef741.png — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/belak/base16-emacs/issues/25#issuecomment-245123675, or mute the thread https://github.com/notifications/unsubscribe-auth/AMIZ7DHsV6CWJLB74Z0-_cMmQLwW0d29ks5qnfHtgaJpZM4J13PS.