dracula / emacs

🧛🏻‍♂️ Dark theme for Emacs
https://draculatheme.com/emacs
MIT License
327 stars 87 forks source link

Fix ansi-color definitions #101

Closed aciceri closed 11 months ago

aciceri commented 11 months ago

Perhaps it's just me that I can't see how to do that but I would be cool if it would be nice having the possibility to use the colors defined here in other modes.

Or simply moving the colors definition from the let expression to a proper variable or creating some faces dracula-color-<color>.

My current workaround is using the faces term-color-<color> which anyway aren't 1:1 with colors names in colors (e.g. pink <-> magenta) and aren't all the colors.

aciceri commented 11 months ago

To better frame why I would like it here's my workaround to set eat's faces:

(require 'term)
  (let ((colors '("black" "red" "green" "yellow" "blue" "magenta" "cyan" "white"))
    (dolist (color colors)
      (set-face-attribute
        (intern (format "eat-term-color-%s" color))
        nil
        :inherit (intern (format "term-color-%s" color)))))

Notice that I needed to load term only because I needed its faces (I still haven't fully understood how emacs set faces when loads a theme).

milouse commented 11 months ago

Actually your are just looking for this theme to support eat :)

After a quick look, eat itself defines its colors as alias for other thing, which is great as it normally allows seamless integration without cluttering the theme. However, for the color you are looking for, it aliases ansi-color-* and ansi-bright-color-* which are not customized in the dracula-theme. We used to set the variable ansi-color-names-vector but I can see it is now deprecated.

Thus, need to fix all that.

aciceri commented 11 months ago

Yes that would be even better! I believe I can send a PR.

But at the same time I would like having access to colors directly. It would improve extendibility and hackability, would you also accept a PR adding new faces dracula-<color>?

milouse commented 11 months ago

I just pushed a fix regarding ansi-color definition. Can you try with the new version (as I pushed directly to main branch, it should be available very soon through melpa repository. You can just pick the dracula-theme.el file directly from the repository temporarily. Let me know if it’s better now or not (do not hesitate to send a screenshot).

Regarding opening dracula-<color> variable, I’m not very convinced. This is not how Emacs theme work. If you encounter other issue with other mode/package, the best way to fix that is to support them inside the theme (to let other user benefit from the fix).

Nobody can know all possible Emacs packages out there, thus it’s better for everybody to report here missing packages, instead of having individual fixing missing part on their side just for them :)

And in any case, you’ll have better integration result by re-using some highlevel faces like font-lock-* faces.

aciceri commented 11 months ago

Regarding opening dracula- variable, I’m not very convinced. This is not how Emacs theme work. If you encounter other issue with other mode/package, the best way to fix that is to support them inside the theme (to let other user benefit from the fix).

Nobody can know all possible Emacs packages out there, thus it’s better for everybody to report here missing packages, instead of having individual fixing missing part on their side just for them :)

I agree 100% with both these things obviously but I still think that there should be a way to access the color palette somehow. While a PR upstream is always the best thing to do, it would make easier experimenting and playing with Emacs. I'm a novice so perhaps what I said doesn't make sense on the implementation side I expect a convenient way to get colors, not something like what I did.

And in any case, you’ll have better integration result by re-using some highlevel faces like font-lock-* faces.

Why? As far as I can understand they are not named after the color they represent. I would need to hardcode a map in my code. At this point I could simply copy/paste the colors variable inside my configuration.

Anyway I've tried it, eat uses different colors now but they are still different from what I expected. Indeed my goal was having same colors both on terminal emulator (wezterm with the official dracula theme) and emacs' eat terminal emulator.

This is how I see it letting eat use the ansi colors faces defined by dracula-theme in your latest commit: image

This is how I see colors on wezterm: image

And this is what I see using my manually defined eat-term-color-<color> with my previous message's hack:

image

(Ignore the background color which I manually changed for some buffers using solaire-mode)

milouse commented 11 months ago

Thank you for the screenshot, it is exactly what I was willing to see (I’m not using eat at all).

It was just a quick test to see if I can improve the ansi palette or not. Quick explanation here: normally, on terminal ansi colors are defined as a serie of 8 colors, with to variation for each: "normal" color and "bright" color. However the dracula theme does not define any "bright" color and usually themers just copy paste the normal color on the bright one.

During my work on the Emacs theme, I found some situation in which this copy/pasting was no more sufficient (for exemple when dealing with diff output, which involve graduation of the same color). That’s why I previously added some "dark" colors. Those are the one you see in your screenshot.

I’ll change that and move to another idea: same colors for "normal" and "bright", but I’ll enforce a bold face for "bright" colors. It should be sufficient for texts.

I already pushed a new test. Can you check if it’s better now?

aciceri commented 11 months ago

Much better, thanks! This is inside eat now using your latest commit: image

But there is still something wrong, this is wezterm: image

And this is eat: image

In particular the blue should be more pinky.

milouse commented 11 months ago

Yep, they are using the dracula purple color for blue, when I’m using the "comment" color for blue. That’s a mistake of mine, the term-color-* also use purple for blue. Last commit should definitely be ok.

aciceri commented 11 months ago

Just tried, blue is shown as purple, I believe this can be closed now. Thanks!