LionyxML / auto-dark-emacs

Auto-Dark-Emacs is an auto changer between 2 themes, dark/light, following MacOS, Linux or Windows Dark Mode settings
GNU General Public License v2.0
149 stars 35 forks source link

“Loading a theme can run Lisp code. Really load? (y or n)” breaks Doom Emacs initialization #64

Open ehamberg opened 1 week ago

ehamberg commented 1 week ago

After #57 was merged, I get Loading a theme can run Lisp code. Really load? (y or n) when I start Emacs which seems to break something with the Doom Emacs initialization and leads to the window not being redrawn.

(Is this because it now calls (load-theme theme) instead of (load-theme theme t)?)

LionyxML commented 6 days ago

Oh dang! Sorry we broke it for you @ehamberg.

I managed to set doom here and loading with this extra argument did not fixed the issue. What I ended up doing is forcing Emacs to recognize custom themes as safe.

As a temporary fix while I find some time to proper debug this issue, could you try:

(after! doom-ui
  (setq custom-safe-themes t) ;; <<<---- ADD THIS
  ;; set your favorite themes
  (setq! auto-dark-themes '((doom-one) '(doom-one-light)))
  (auto-dark-mode))

Tell me if it 'solves' it for you 😄

LionyxML commented 6 days ago

Added this note to README via #65.

ehamberg commented 5 days ago

Tell me if it 'solves' it for you 😄

It did![^1] Thanks!

I couldn't get auto-dark-themes to work though, but my old, trusty (but now deprecated 😬) settings still works:

  (setq auto-dark-light-theme 'doom-bluloco-light)
  (setq auto-dark-dark-theme 'doom-bluloco-dark)

[^1]: But I had to hunt down to places where the value of custom-safe-themes was persisted, first, though. It was saved at the bottom of my config.el and in a custom.el file in ~/.config/doom.

sellout commented 4 days ago

@ehamberg Sorry to have caused you trouble. Can you show me how you were setting auto-dark-themes?

The removal of the optional argument to load-theme should have been called out more clearly, or perhaps made in a second PR – I apologize. But I think it is the correct behavior, and using custom-safe-themes is the right way to silence the confirmation (but it should be called out in the README & docstrings – thanks @LionyxML for addressing my oversight there so quickly).

I made an effort to keep the old approach working alongside the new. After figuring out which var to pull the list of themes from, the logic is shared – so I’m surprised the auto-dark-themes isn’t working for you. I did notice I have a typo in the README:

(setq! auto-dark-themes '((doom-one) '(doom-one-light)))

should be

(setq! auto-dark-themes '((doom-one) (doom-one-light)))

Did you maybe copy that bug? If not, can you show me how you tried setting auto-dark-themes?

@LionyxML I can address updating the docstrings and fixing my README typo.

ehamberg commented 3 days ago

I made an effort to keep the old approach working alongside the new. After figuring out which var to pull the list of themes from, the logic is shared – so I’m surprised the auto-dark-themes isn’t working for you. I did notice I have a typo in the README:

(setq! auto-dark-themes '((doom-one) '(doom-one-light)))

should be

(setq! auto-dark-themes '((doom-one) (doom-one-light)))

Did you maybe copy that bug? If not, can you show me how you tried setting auto-dark-themes?

I probably did copy that bug, because (setq! auto-dark-themes '((doom-one) (doom-one-light))) worked. Thanks!

LionyxML commented 2 days ago

Thanks for offering to change the README @sellout.

Yeah, the typo seemed to work for me because it defaulted to Emacs default theme (light) and gave me the false impression it was alright, lol.

I was messing again with doomemacs today and I got something strange happening.

At least for my version:

GNU Emacs     v30.0.91         nil
Doom core     v3.0.0-pre       grafted, HEAD -> master, origin/master, origin/HEAD 2e5307e 2024-09-14 13:08:00 -0700
Doom modules  v24.10.0-pre     grafted, HEAD -> master, origin/master, origin/HEAD 2e5307e 2024-09-14 13:08:00 -0700

The after! doom-ui is actually not working, it must be set to after! doom-themes.

The working example would be something like:

;; packages.el
(package! auto-dark)

;; config.el
(after! doom-themes
  (setq custom-safe-themes t)  ;; as discussed on https://github.com/LionyxML/auto-dark-emacs/issues/64

  (setq auto-dark-themes '((doom-one) (doom-gruvbox)))
  (auto-dark-mode))

Could you please validate if this snippet also works for you @ehamberg?

mepcotterell commented 1 day ago

Just adding this here in case anyone finds it useful. I don't use Doom, but when I encountered this issue in vanilla after upgrading and saw the fix in the documentation, I decided to adapt it slightly using advice so that custom-safe-themes is only set to t temporarily when auto-dark--enable-themes is called. Here the use-package declaration that I use:

(use-package auto-dark
  :if (display-graphic-p)
  :after solarized-theme
  :diminish
  :custom
  (auto-dark-themes '((solarized-dark wombat) (solarized-light leuven)))
  (auto-dark-polling-interval-seconds 5)
  (auto-dark-allow-osascript (eq system-type 'darwin))
  :init
  (defun advice/auto-dark--enable-themes (auto-dark--enable-themes &rest args)
    "Set `custom-safe-themes' to t when calling AUTO-DARK--ENABLE-THEMES with ARGS.
See URL `https://github.com/LionyxML/auto-dark-emacs/issues/64' for more information."
    (let ((custom-safe-themes t)) (apply auto-dark--enable-themes args)))
  (advice-add 'auto-dark--enable-themes :around 'advice/auto-dark--enable-themes)
  (auto-dark-mode))

This was just a little more readable for me and has two added benefits: i) it shows the advice docstring when looking up auto-dark--enable-themes using describe-function; and ii) it restores custom-safe-themes automatically, assuming lexical binding is enabled.

ehamberg commented 1 day ago

Could you please validate if this snippet also works for you @ehamberg?

Not really. It seems to only work after one dark mode toggle. So with the following config…

(after! doom-themes
  (setq custom-safe-themes t)  ;; as discussed on https://github.com/LionyxML/auto-dark-emacs/issues/64

  (setq auto-dark-themes '((leuven-dark) (leuven)))
  (auto-dark-mode))

… I first get the doom-one (default, I guess) theme, then if I enable the system's dark mode, I get the leuven-dark theme, then if I disable the system's dark mode I get leuven.

This is the actual, working config I use in my config.el file:

(use-package! auto-dark
  :config
  (setq custom-safe-themes t)
  (setq! auto-dark-themes '((doom-bluloco-dark) (doom-bluloco-light)))
  (auto-dark-mode))
LionyxML commented 1 day ago

Thanks for testing @ehamberg!

Well, this also works for me (+ the entry on packages.el).

Maybe this is going to be our new DoomEmacs snippet?

LionyxML commented 1 day ago

Just adding this here in case anyone finds it useful. I don't use Doom, but when I encountered this issue in vanilla after upgrading and saw the fix in the documentation, I decided to adapt it slightly using advice so that custom-safe-themes is only set to t temporarily when auto-dark--enable-themes is called. Here the use-package declaration that I use:

(use-package auto-dark
  :if (display-graphic-p)
  :after solarized-theme
  :diminish
  :custom
  (auto-dark-themes '((solarized-dark wombat) (solarized-light leuven)))
  (auto-dark-polling-interval-seconds 5)
  (auto-dark-allow-osascript (eq system-type 'darwin))
  :init
  (defun advice/auto-dark--enable-themes (auto-dark--enable-themes &rest args)
    "Set `custom-safe-themes' to t when calling AUTO-DARK--ENABLE-THEMES with ARGS.
See URL `https://github.com/LionyxML/auto-dark-emacs/issues/64' for more information."
    (let ((custom-safe-themes t)) (apply auto-dark--enable-themes args)))
  (advice-add 'auto-dark--enable-themes :around 'advice/auto-dark--enable-themes)
  (auto-dark-mode))

This was just a little more readable for me and has two added benefits: i) it shows the advice docstring when looking up auto-dark--enable-themes using describe-function; and ii) it restores custom-safe-themes automatically, assuming lexical binding is enabled.

Ohhh dang, I missed this message! Thanks for reaching @mepcotterell.

Humm, so this is also happening on Vanilla Emacs? Maybe we should add this 'fix' on the Readme for all cases. What do you think @sellout ?

sellout commented 22 hours ago

Humm, so this is also happening on Vanilla Emacs? Maybe we should add this 'fix' on the Readme for all cases. What do you think @sellout ?

Yeah, custom-safe-themes is going to be needed on all versions of Emacs.

But I think if you don’t just want to set it to t, the way to go about it is to limit it to the themes you intend to use, rather than toggling it on with advice for auto-dark. E.g.,

(use-package auto-dark
  :custom
  (auto-dark-themes '((solarized-dark wombat) (solarized-light leuven)))
  (custom-safe-themes
   '("c8cfb034378af37e278fbf1d7cc6584131b686650fb0503d78c59817310aee54" ; leuven
     "52c3d95f52f9e2889576bd7500fbbc7d3d56f1f718d734d87f02e9cc309eaa77" ; solarized-dark
     "ba5fedf9df7a51454f21f100479c3cf562b66c919be41cf2bfdc088330e77ed4" ; solarized-light
     "ff6aecadd496de713fce479ee3aca55359f2af8b1955d61c9e05363e4d97d8f4" ; wombat
     )))

Where each string is the SHA-256 hash of the theme file in question (see The Emacs Manual, §50.1.7 Custom Themes).

NB: I just made up those SHAs, they’re not the right ones for those themes.

One other possibility is adding something like defcustom auto-dark-assume-themes-are-safe, which would default to nil, but when set to t would have the same behavior as before #57. The effect would be like @mepcotterell’s advice, but simpler for users. Not as safe as listing explicit theme SHAs, but safer than setting custom-safe-themes to t, and better than the old behavior because the user still has to opt-in.