vic / color-theme-buffer-local

Set emacs color themes by buffer.
http://github.com/vic/color-theme-buffer-local
GNU General Public License v3.0
81 stars 14 forks source link

Broken load-theme-buffer-local #5

Closed sbp closed 8 years ago

sbp commented 10 years ago

Using load-theme-buffer-local 20120702.1336 from MELPA in emacs 24.3.1:

(load-theme-buffer-local 'theme-name)

Breaks with:

Symbol's function definition is void: symbol
sbp commented 10 years ago

Using b4444be311 seems to work, so I think the issue is that you need to update the MELPA package.

vt5491 commented 10 years ago

I had this problem too. It only happened on my windows emacs 24.3.1 and not on my ubuntu 1404 emacs 24.3.1. The problem is emacs 24.3 seems to require all common lisp (cl) functions be prefixed with "cl". This is true for "loop" (now "cl-loop") and "flet" (now "cl-flet"). You used to be able to get away with not prefixing them.

If you look in file 'load-theme-buffer-local.el' under the ELPA path (~/.emacs.d/elpa/load-theme-buffer-local-20120702.1336) you'll see it's using "flet" to dynamically define the function "symbol". I just updated this to have 'cl-flet'.

Here is what the updated code looks like:

;;;###autoload (defun load-theme-buffer-local (theme &optional buffer no-confirm no-enable) "Load an Emacs24 THEME only in BUFFER." (interactive (list (intern (completing-read "Install theme: " (mapcar 'symbol-name (custom-available-themes)))) (read-buffer "on Buffer: " (current-buffer) t))) (or buffer (setq buffer (current-buffer))) ;;vt sub cl-flet for flet (cl-flet ((custom-theme-recalc-face (symbol) (custom-theme-buffer-local-recalc-face symbol buffer)) (custom-theme-recalc-variable (symbol) (custom-theme-buffer-local-recalc-variable symbol buffer))) (load-theme theme no-confirm no-enable)))

Don't forget to to c-x c-e against the updated defun so it takes effect, and the to do a 'byte-compile-file' to create the '.elc' file so it take effect when you re-start emacs as well.

Now 'load-theme-buffer-local' runs, but unfortunately it is not buffer level. It updates all buffers just like 'load-theme'. Trying to figure that one out now.

vic commented 10 years ago

@vt5491 could you provide a pull-request ?

vt5491 commented 10 years ago

Hi Vic, It's a pleasure to correspond with you. I'm not quite sure what a pull request is, but in googling on it, I understand it as a request to examine changes I've committed to the project (I just signed up to github yesterday). Unfortunately, since I'm still working out a fix for my own personal use, I haven't yet committed anything official. Ergo, I haven't committed anything yet. But I will gladly try to make my fixes available once I have something definitive. For now, I will try to document in more detail what I uncovered, if only for my future reference, since I may not have time to work on it for a while.

Here is where I am currently at with the modified version on my laptop.

1) Unfortunately, since cl-flet uses lexical binding instead of dynamic binding (http://stackoverflow.com/questions/18895605/should-flet-be-replaced-with-cl-flet-or-cl-letf) cl-flet cannot be used to replace 'flet' in this case (Why I was not getting buffer level overrides when using it). Basically, what this means is the overridden functions are no longer overridden once outside of the context where the override was established. I put in prints and manually verified that when using cl-flet it indeed is calling the base methods and not the overridden versions. 2) I looked at the code a little more and now understand that the basic mechanism is to override the functions 'custom-theme-buffer-local-recalc-face' and 'custom-theme-buffer-local-recalc-variable' (with 'custom-theme-buffer-local-recalc-face' being the one that is mostly used) defined in 'custom.el', the main load-theme package. The base functions will change the face for all frames, whereas the overridden ones will just do it for the specified buffer only. In other words, 'load-theme-buffer-local' package is basically hooking two functions in the standard 'load-theme' package.

However, as previously mentioned, since cl-flet uses lexical binding it means that the override is lost once it leaves the confine of the setting up function ('load-theme-buffer-local'), whereas with flet, which uses dynamic binding, the override was in effect 'all the way down'.

So the problem becomes how to override these functions without using flet.

3) One way, and I think the best way, is to use 'around advice' and use an aspect to completely replace the base function (but temporarily -- just for the life of the call). Some people think aspects are horrible though, but I've used them before in perl and I love them. See the following for some discussion on "advice": http://stackoverflow.com/questions/2406814/overriding-a-function-in-emacs-lisp http://stackoverflow.com/questions/15717103/preferred-method-of-overriding-an-emacs-lisp-function

Unfortunately, I could not get around advice working, which perhaps isn't surprising since I'm just learning elisp and aspect oriented programming is generally considered to be a pretty advanced technique in most languages.

4) There are other versions of flet out there that you can use to replace the old 'flet'. I read about noflet https://github.com/nicferrier/emacs-noflet. I don't think this is a good idea to use this however, since it will require a dependency on a package that is not standard to emacs. However, after being frustrated with around advice, I downloaded this, and used it just to see if it would work. Indeed it did work, and now I can set themes at the buffer level again. However, I'm having trouble getting the compiled version of 'load-theme-buffer-local' to properly use noflet. For some reason, I have to manually "c-x c-e" the (modified) source code for "defun load-theme-buffer-local" when I first bring up emacs in order to get it work -- I really need to familiarize myself more with requires, and autoloading etc. But it's proof of concept that I understood the code, what the problem was, and that there are solutions available.

5) After invoking the theme at the buffer level, I did observe a lot of message on the Messages buffer that look like the following. You basically get these every time you move the cursor in the buffer: Invalid face reference: mode-line-buffer-id Invalid face attribute :background nil Invalid face reference: mode-line-buffer-id

I don't know if this is existing behavior, or it's somehow an effect of how I did it. I suppose I could use my Ubuntu as a control, since it doesn't seem to have a problem with 'flet'. In other words, I need to do more testing.

6) It was fun to do all this because I'm trying to learn elisp, and this was a great exercise. Also, since I have to have colored backgrounds and I use colors to distinguish buffers and frames, I will need to get this working before I can use emacs (I have traditionally used the ‘color-theme-select’ themes, but I’m trying to force myself to use the new theming). In other words, I’m pretty motivated to get ‘load-theme-buffer-local’ working, assuming I have the time.

7) However, since I’m new to elisp, I might be thinking about this too hard. Maybe there’s just a really simple way to ‘hook’ functions. It also occurred to me that maybe the best ‘solution’ is simply to add buffer level and frame-level capability into ‘load-theme’ (custom.el) directly, instead of hacking it with another module. OTOH, the base module is pretty complicated, so this might be hard to do.

Anyway, if I can work out a way to get it working without requiring another module, I will try to do a ‘pull-request’ at that time. Well, maybe I can ask you. Do think it’s a show-stopper if it requires ‘noflet’. Do you think using “around advice” to hook the functions is a good idea? Regards, vt

vt5491 commented 10 years ago

I started working on this again. I got defadvice (around advice) now working. This is a lot better than noflet in that it doesn't introduce new dependencies on the module.

Here is what my current development version of 'load-theme-buffer-local' looks like. It needs clean up, I still have debug messages in, and I need to remove my initials from the variables etc (convenient during testing so I know what is "mine"), but it does work:

;; this works.
;; from cli:
;;(vt-dev-load-theme-buffer-local 'wombat vt-buf-messages)
;;(vt-dev-load-theme-buffer-local 'wheatgrass vt-buf-advice)
;;(vt-dev-load-theme-buffer-local 'tango vt-buf-advice)
;;
;; and from mini-buffer as well
(defun vt-dev-load-theme-buffer-local (theme &optional buffer no-confirm no-enable)
  "Load an Emacs24 THEME only in BUFFER."
  (interactive
   (list (intern (completing-read
                  "Install theme: "
                  (mapcar 'symbol-name (custom-available-themes))))
         (read-buffer "on Buffer: " (current-buffer) t)))
  (or buffer (setq buffer (current-buffer)))
  ;;vt-new
      (defadvice custom-theme-recalc-face (around vt-recalc-face-around)
      (progn
        (message "vt: entered vt-custom-theme-recalc-face-around advice, symbol=%s, buffer=%s" symbol buffer)
        ;; Note: 'buffer is bound at definition time e.g when this devadvice is created
        ;; and 'symbol is bound at runtime, when 'load-theme -> 'enable-theme is called
        ;; We obviously have to bind the buffer at definition time, since 'buffer will
        ;; not be available at 'enable-theme run time, since it is *not* a buffer level
        ;; function
        (custom-theme-buffer-local-recalc-face symbol buffer)
        ;; Note: we do not call the advised function at all.  All the logic 
        ;; is in the monkey patch itelf (this devadvice call).  We leave
        ;; the 'ad-do-it' line commented out to make this clear.  Normally, ad-do-it
        ;; will transfer control to the overridden function.
        ;;ad-do-it
        (message "vt vt-custom-theme-recalc-face-around-advice: exited")))
      (ad-enable-advice 'custom-theme-recalc-face 'around 'vt-recalc-face-around)
      (ad-activate 'custom-theme-recalc-face)
      (load-theme theme))
  ;;vt-end
  ;; (flet ((custom-theme-recalc-face
  ;;         (symbol) (custom-theme-buffer-local-recalc-face symbol buffer))
  ;;        (custom-theme-recalc-variable
  ;;         (symbol) (custom-theme-buffer-local-recalc-variable symbol buffer)))
  ;;   (load-theme theme no-confirm no-enable)))

;;vt end

I figure I'll put in an (fboundp 'flet) call and use flet by default if it's available, and only use advice if flet is not available. I also just need to add:

      (ad-disable-advice 'custom-theme-recalc-face 'around 'vt-recalc-face-around)
      (ad-activate 'custom-theme-recalc-face)

at the end to remove the monkey-patch so 'load-theme' would work in it's standard fashion.

I forked a new branch and will commit my changes and do a pull-request at that time. I still need more time to test it etc. I have also previously hooked the 'custom-theme-recalc-face' function as well, although it doesn't seemed to be used, at least when I change themes. I also do no see error messages on the Messages log like before. Wonder if hooking 'custom-theme-recalc-face' is what was causing this?

vt5491 commented 10 years ago

I'm now leaning towards using 'noflet' instead of using advice. I read the following from elisp entry on wikipedia (http://en.wikipedia.org/wiki/Emacs_Lisp):

Discouraged features[edit] After decades of evolution, some features have become deprecated or replaced, and other features are now supported for Emacs users but their use is not allowed in the Emacs source code itself. defadvice and letf are two examples of the latter, of which Richard Stallman says "it makes for confusion in debugging. (...) Users can use these features -- the only people they might confuse are themselves, (....) However, in our code, we should handle these situations in other ways."[3] Adding new hooks can sometimes provide what's necessary to replace advice.[9]
vt5491 commented 9 years ago

This was originally only only a problem on windows emacs. Recently it just started happening on ubuntu 14.10. It does not happen on Ubuntu 1404, or linux mint 17.0 or 17.1.

Anyways, I just wanted it to work on ubuntu, so I just made the easiest change which is to use noflet. Note: I did not update the package manager ('load-theme-buffer-local-autoloads.el') to require 'noflet', as I'm not familiar with that mechanism.

Instead of researching it to death, and trying to find the 'best' solution like last time, I just decided to make a quick and dirty fix, as now I'm time constrained and if I don't do it now I'll never do it.

The point it is, if you decide to accept the fix, you're going to have to update the package to require noflet.

vt5491 commented 8 years ago

This can be closed now. It's simply not happening anymore. It originally only happened only on windows. Anyway, I'm running emacs 25.0 on windows and linux and this is not happening. Not that going to emacs 25 made a difference -- it was working with emacs 24.3 and 24.5.

I think it's safe to say this was an environmental issue and not a problem with 'load-theme-buffer-local'. Whatever the environmental issue was it's now gone away.

vic commented 8 years ago

ok closing now.

radrow commented 1 year ago

The issue seems to be back on emacs 28.2. I am getting exactly the same error message. I am on Fedora.

leonchas17 commented 1 year ago

I was also seeing this issue on Emacs for Mac OSX 28.2, and on Aquamacs. I was able to correct it by following the advice by @vt5491 on https://github.com/vic/color-theme-buffer-local/pull/6 (under the "files changed" tab) : after installing noflet, I changed the two lines mentioned on pull request #6 on my load-theme-buffer-local.el and byte-compiled it, then restarted Emacs again. That got me the correct behavior. I'm still working on getting the correct functionality on Aquamacs, though.

leonchas17 commented 1 year ago

I was also having this issue on Emacs 27.2 on Windows 11. Applying the changes suggested by @vt5491 on pull request #6 solved it.