alphapapa / prism.el

Disperse Lisp forms (and other languages) into a spectrum of colors by depth
GNU General Public License v3.0
291 stars 4 forks source link

prism-after-theme advice broken on terminal #32

Open bram85 opened 3 months ago

bram85 commented 3 months ago

The prism-after-theme advice is broken when it is called after disable-theme on a terminal. The advice calls prism-set-colors which in turn calls prism-blend.

disable-theme calls:

    (set-frame-parameter frame 'background-color
                         (custom--frame-color-default
                          frame :background "background" "Background"
                          "unspecified-bg" "white"))

On a terminal, this will set the frame background color to unspecified-bg.prism-set-colors would retrieve the background color for the parens-fn parameter:

(parens-fn (lambda (color)
                       (prism-blend color (face-attribute 'default :background nil 'default) 0.5)))

This results in the following backtrace:

Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
  (#f(compiled-function (a b alpha) #<bytecode 0x177de5e630db1909>) 0.6784313725490196 nil 0.5)
  (prism-blend "#ade5ad" "unspecified-bg" 0.5)
  (#f(compiled-function (color) #<bytecode 0x5a758d4471ffd80>) "#ade5ad")
  (#f(compiled-function (arg1 &rest rest) #<bytecode 0x15266b1d7a20cb50>) ("#ade5ad" "#a0c8e1" "#32cbcc" "#e4ab94" "#c1e6c1" "#b3cfe0" "#49c2c2" "#e2b9a9") "parens" #f(compiled-function (color) #<bytecode 0x5a758d4471ffd80>))
  (prism-set-colors)
  (prism-after-theme modus-operandi-tinted)
  (apply prism-after-theme modus-operandi-tinted)
  (disable-theme modus-operandi-tinted)
  (#f(compiled-function (theme &optional no-confirm no-enable) "Load Custom theme named THEME from its file and possibly enable it.\nThe theme file is named THEME-theme.el, in one of the directories\nspecified by `custom-theme-load-path'.\n\nIf the theme is not considered safe by `custom-safe-themes',\nprompt the user for confirmation before loading it.  But if\noptional arg NO-CONFIRM is non-nil, load the theme without\nprompting.\n\nNormally, this function also enables THEME.  If optional arg\nNO-ENABLE is non-nil, load the theme but don't enable it, unless\nthe theme was already enabled.\n\nNote that enabling THEME does not disable any other\nalready-enabled themes.  If THEME is enabled, it has the highest\nprecedence (after `user') among enabled themes.  To disable other\nthemes, use `disable-theme'.\n\nThis function is normally called through Customize when setting\n`custom-enabled-themes'.  If used directly in your init file, it\nshould be called with a non-nil NO-CONFIRM argument, or after\n`custom-safe-themes' has been loaded.\n\nReturn t if THEME was successfully loaded, nil otherwise." (interactive #f(compiled-function () #<bytecode -0x1403a5c4a355a2c9>)) #<bytecode 0x1402561343130b0a>) modus-operandi-tinted :no-confirm)
  (apply #f(compiled-function (theme &optional no-confirm no-enable) "Load Custom theme named THEME from its file and possibly enable it.\nThe theme file is named THEME-theme.el, in one of the directories\nspecified by `custom-theme-load-path'.\n\nIf the theme is not considered safe by `custom-safe-themes',\nprompt the user for confirmation before loading it.  But if\noptional arg NO-CONFIRM is non-nil, load the theme without\nprompting.\n\nNormally, this function also enables THEME.  If optional arg\nNO-ENABLE is non-nil, load the theme but don't enable it, unless\nthe theme was already enabled.\n\nNote that enabling THEME does not disable any other\nalready-enabled themes.  If THEME is enabled, it has the highest\nprecedence (after `user') among enabled themes.  To disable other\nthemes, use `disable-theme'.\n\nThis function is normally called through Customize when setting\n`custom-enabled-themes'.  If used directly in your init file, it\nshould be called with a non-nil NO-CONFIRM argument, or after\n`custom-safe-themes' has been loaded.\n\nReturn t if THEME was successfully loaded, nil otherwise." (interactive #f(compiled-function () #<bytecode -0x1403a5c4a355a2c9>)) #<bytecode 0x1402561343130b0a>) (modus-operandi-tinted :no-confirm))
  (load-theme modus-operandi-tinted :no-confirm)
  (modus-themes-load-theme modus-operandi-tinted)
  (modus-themes-toggle)
  (funcall-interactively modus-themes-toggle)
  (call-interactively modus-themes-toggle record nil)
  (command-execute modus-themes-toggle record)
  (execute-extended-command nil "modus-themes-toggle" nil)
  (funcall-interactively execute-extended-command nil "modus-themes-toggle" nil)
  (call-interactively execute-extended-command nil nil)
  (command-execute execute-extended-command)

I created a workaround to add an advice to prism-blend and intercept any unspecified colors:

    (defun bram85-prism-blend-advice (f a b alpha)
      (let ((a_ (if (string-prefix-p "unspecified-" a) "white" a))
            (b_ (if (string-prefix-p "unspecified-" b) "white" b)))
        (funcall f a_ b_ alpha)))

    (advice-add #'prism-blend :around #'bram85-prism-blend-advice)

Personally I only encounter this issue when calling modus-themes-toggle, which is roughly a sequence of disable-theme and load-theme. Therefore the workaround is sufficient for me, as I wouldn't observe the blending result with the white dummy color. But it wouldn't generalize well for those who run disable-theme in isolation.

alphapapa commented 3 months ago

According to the face-attribute docstring:

To ensure that the return value is always specified and absolute, use a
value of ‘default’ for INHERIT; this will resolve any unspecified or
relative values by merging with the ‘default’ face (which is always
completely specified).

If your default face has unspecified attributes, that might indicate a bug elsewhere, unless I'm misunderstanding something.

alphapapa commented 3 months ago

According to the Elisp manual, as I quoted, the default face is supposed to be always completely specified. Yet after running emacs -q -nw, (face-attribute 'default :background nil 'default) returns "unspecified-bg". That would seem like a bug, in the documentation if nothing else.

I'd suggest that someone search the Emacs bug tracker for reports related to this; I'd be surprised if it's never come up before.

I don't know yet what the best solution is, but a potential workaround is to wrap the body of prism-after-theme in ignore-errors, like:

(defun prism-after-theme (&rest args)
  "For `load-theme' advice.
ARGS may be what `load-theme' and `disable-theme' expect.  Unless
NO-ENABLE (optional third argument, like `load-theme') is
non-nil, call `prism-set-colors' to update `prism' faces."
  (ignore-errors
    (unless (cl-third args)
      (prism-set-colors))))

That prevents the error from disable-theme, and then when the next theme is loaded, it works as expected.

shipmints commented 3 days ago

Having the same issue with prism on tty when changing themes via modus-themes-toggle which first disables all themes and then enables the specified theme. Digging into it, Emacs behavior GUI to tty is different when disabling themes and it resets bg and fg colors to unspecified. If prism advises after disable-theme, prism color resetting will always fail on a tty if it does not guard against unspecified. Below is the same on 29.4 and current master.

(defun disable-theme (theme)
...
      (set-frame-parameter frame 'background-color
                           (custom--frame-color-default
                            frame :background "background" "Background"
                            "unspecified-bg" "white")) ; <--- tty-default is unspecified
...
(defun custom--frame-color-default (frame attribute resource-attr resource-class
                      tty-default x-default)
...

Some archaeology:

https://lists.gnu.org/archive/html/emacs-diffs/2022-05/msg00863.html