jdtsmith / indent-bars

Fast, configurable indentation guide-bars for Emacs
GNU General Public License v3.0
272 stars 7 forks source link

Automatically `indent-bars-reset` when switching between themes #31

Closed aciceri closed 3 months ago

aciceri commented 6 months ago

I just added the following to my config

(advice-add 'consult-theme :after #'(lambda (&rest r) (with-eval-after-load 'indent-bars (indent-bars-reset))))

(I'm not sure if with-eval-after-load is really needed)

Indeed I switch between themes often (especially between a light and a dark theme) and I indent bars' colors were updating correctly. I don't know indent-bars internals but I wonder why this is not the default (advising an emacs builtin like load-theme instead). Has it any caveat I'm not considering?

jdtsmith commented 6 months ago

There is strangely no published hook for theme changes. I think there should be; perhaps submit it as a bug report? In the meantime, the right way to do it is to add advice to enable-theme on indent-bars load (e.g. in use-package :config). Prot shows how.

aciceri commented 6 months ago

Yeah I agree that an hook should exist, Prot's solution is more versatile than mine. So I guess that you are waiting for this to be in Emacs itself before implementing something that automatically calls indent-bars-reset when a new theme is loaded?

Feel free to close in that case.

jdtsmith commented 6 months ago

Yes implementing it as advice is a maintenance issue. A hook should exist for many other modes, so please do M-x report-emacs-bug. Do you want to propose some documentation?

aciceri commented 4 months ago

@jdtsmith Sorry for never answering this. I'm using the following right now:

  (defvar after-enable-theme-hook nil
    "Normal hook run after enabling a theme.")

  (defun run-after-enable-theme-hook (&rest _args)
    "Run `after-enable-theme-hook'."
    (run-hooks 'after-enable-theme-hook))

  (advice-add 'enable-theme :after #'run-after-enable-theme-hook)

  (add-hook 'after-enable-theme-hook #'indent-bars-reset)

For some reason if I put the hook inside the use-package's :hook "stanza" it doesn't work anymore.

please do M-x report-emacs-bug

Will do it as soon as I understand how the GNU bugs tracker works (first time using it)

Do you want to propose some documentation?

Sure! Do you mean in the wiki or in the README? The snippet above with a bit of context on what it does and why it's needed is enough?

aciceri commented 4 months ago

Just sent a report with report-emacs-bug, added you in CC.

aciceri commented 4 months ago

I reported that and they told me that actually such an hook exists since 29.1, indeed I'm successfully using this now.

(add-hook 'enable-theme-functions #'(lambda (&rest _) (indent-bars-reset)))

Would you want me to send a PR doing this? In that case can I simply assume that everybody is at least on 29.1 or do you prefer a check?

PS: do you want to re-open this issue maybe?

jdtsmith commented 4 months ago

Thanks. In fact I've added a similar thing to another mode of mine so I'll do so here.

jdtsmith commented 4 months ago

Closed in 4583e3e; please remove your config and give it a test.

aciceri commented 4 months ago

Just tried, it works! (I'm on 30.0.50)

Gleek commented 3 months ago

Hey @jdtsmith! This enables indent-bars even if it was loaded and not enabled when switching themes.

We should only setup the hook when the mode is enabled and remove the hook once the we disable the mode.

jdtsmith commented 3 months ago

Aha, right. But this solution won't work, since the mode is enabled per-buffer. The theme hook must be enabled globally, since there is no such thing as a per-buffer theme change. It makes sense to defer loading the mode until you actually want to use it, which the use-package based config should do.

That said, I think the behavior will depend on what buffer you are in when you change themes (right?), which isn't right. In a version to be released soon I separate out the global indent-bars face (re-)creation from the local buffer updates, so I'll switch to doing that on theme load instead.

jdtsmith commented 3 months ago

For now I've disable enable-theme-functions support since it was causing more trouble than it solves. v0.5 will correctly handle this.