search

protesilaos / modus-themes

Highly accessible themes for GNU Emacs, conforming with the highest standard for colour contrast between background and foreground values (WCAG AAA).
https://protesilaos.com/emacs/modus-themes
GNU General Public License v3.0
573 stars 31 forks source link

M-x `toggle-theme' née `theme-choose-variant' ignores modus hooks. #112

Open 11fdriver opened 4 months ago

11fdriver commented 4 months ago

The built-in function toggle-theme does not run hooks in modus-themes-after-load-theme-hook. It's probably not a big enough issue to be put into the modus package, but it did cause me a touch of confusion earlier today.

Perhaps a quick note could be put in the documentation? I advised theme-choose-variant to get what I wanted:

(defun maybe-run-modus-themes-after-load-theme-hooks ()
  (when (modus-themes--current-theme))
    (run-hooks 'modus-themes-after-load-theme-hook)))

(advice-add 'theme-choose-variant :after #'maybe-run-modus-themes-after-load-theme-hooks)

Cheers for all your work on the modus themes; I'm always deeply impressed when using them!

protesilaos commented 3 months ago

From: "11F. Driver" @.***> Date: Sat, 13 Jul 2024 16:17:11 -0700

The built-in function toggle-theme does not run hooks in modus-themes-after-load-theme-hook. It's probably not a big enough issue to be put into the modus package, but it did cause me a touch of confusion earlier today.

Maybe you can try the 'enable-theme-functions' hook instead? It will run in that situation. Though it can cause issues if you are switching between a Modus and non-Modus theme. I have not tested that scenario.

-- Protesilaos Stavrou https://protesilaos.com

11fdriver commented 3 months ago

Ah, an abnormal hook! (C-h v's words, not mine). It passes the current theme, so I've modified my stuff. Seeing as this is just a hook, would it make sense to add this into modus? It synchronises the behaviour of modus' own toggling commands and the built-in commands.

(defun maybe-run-modus-theme-hooks (theme)
  (when (memq theme '(modus-vivendi modus-operandi))
    (run-hooks 'modus-themes-after-load-theme-hook)))
(add-hook 'enable-theme-functions #'maybe-run-modus-theme-hooks)
protesilaos commented 3 months ago

From: "11F. Driver" @.***> Date: Wed, 17 Jul 2024 16:15:59 -0700

Ah, an abnormal hook! (C-h v's words, not mine). It passes the current theme, so I've modified my stuff. Seeing as this is just a hook, would it make sense to add this into modus? It synchronises the behaviour of modus' own toggling commands and the built-in commands.

(defun maybe-run-modus-theme-hooks (theme)
  (when (memq theme '(modus-vivendi modus-operandi))
    (run-hooks 'modus-themes-after-load-theme-hook)))
(add-hook 'enable-theme-functions #'maybe-run-modus-theme-hooks)

This approach is good, though I am not sure we should have it in the themes. At which point would it be activated? And, more importantly, when do we remove this hook? There is 'disable-theme-functions', of course, but how do we handle it?

-- Protesilaos Stavrou https://protesilaos.com