Hello Prot,
In the manual one of the example looks like,
```
(setq modus-themes-common-palette-overrides
`((bg-paren-match bg-magenta-intense)
,@modus-themes-preset-overrides-intense))
```
And this works. But if I change `bg-magenta-intense` to say `bg-hl-line`
then load-theme call fails with an error. But that's surprising because
if I am not splicing the preset override there,
```
(setq modus-themes-common-palette-overrides
`((bg-paren-match bg-hl-line)))
```
The above just works, load-theme now doesn't fail.
I feel like this is an issue in reconciliation with the preset, though I
can't tell how `bg-hl-line` is any different to `bg-magenta-intense`.
The `modus-themes-list-colors` command lists `bg-hl-line` too so it is a
named colour right? It shouldn't be susceptible to recursion
limitation.
(also apologies if you are aware of this, or if I am missing something in
the documentation)
> From: Imran Khan <contact@imrankhan.live>> Date: Tue, 3 Jan 2023 23:48:14 +0600>> Hello Prot,
Good day Imran,
> In the manual one of the example looks like,>> ```> (setq modus-themes-common-palette-overrides> `((bg-paren-match bg-magenta-intense)> ,@modus-themes-preset-overrides-intense))> ```>> And this works. But if I change `bg-magenta-intense` to say `bg-hl-line`> then load-theme call fails with an error. But that's surprising because> if I am not splicing the preset override there,>> ```> (setq modus-themes-common-palette-overrides> `((bg-paren-match bg-hl-line)))> ```>> The above just works, load-theme now doesn't fail.>> I feel like this is an issue in reconciliation with the preset, though I> can't tell how `bg-hl-line` is any different to `bg-magenta-intense`.> The `modus-themes-list-colors` command lists `bg-hl-line` too so it is a> named colour right? It shouldn't be susceptible to recursion> limitation.
The reason is that the preset remaps 'bg-hl-line' to 'bg-cyan-subtle',
thus undoing the original named colour and introducing recursion.
Maybe there is a smart way to guard against this but I need to think
more about it. Any ideas are most welcome!
> (also apologies if you are aware of this, or if I am missing something in> the documentation)
The documentation can probably be improved though I think here we need
something on the coding front that "just works".
All the best,
Prot
--
Protesilaos Stavrou
https://protesilaos.com
> The reason is that the preset remaps 'bg-hl-line' to 'bg-cyan-subtle',> thus undoing the original named colour and introducing recursion.>> Maybe there is a smart way to guard against this but I need to think> more about it. Any ideas are most welcome!
Oh, that makes sense!
Is there a way for you to detect this and emit a warning? Could be a compilation warning, or also a simple message during theme loading. That'd help debug issues like that because then users at least know why it's not working, and can fall back to copy-pasting the hex codes and leave a comment.
Cheers,
Christian
> From: "Christian Tietze" <me@christiantietze.de>
> Date: Wed, 4 Jan 2023 09:24:36 +0100
Hello Christian,
(CC Imran)
>> The reason is that the preset remaps 'bg-hl-line' to 'bg-cyan-subtle',
>> thus undoing the original named colour and introducing recursion.
>>
>> Maybe there is a smart way to guard against this but I need to think
>> more about it. Any ideas are most welcome!
>
> Oh, that makes sense!
>
> Is there a way for you to detect this and emit a warning? Could be a
> compilation warning, or also a simple message during theme
> loading. That'd help debug issues like that because then users at
> least know why it's not working, and can fall back to copy-pasting the
> hex codes and leave a comment.
I was hesitant to introduce recursion for 4.0.0 as the overrides were
not widely tested. But now I think we should reconsider that point.
Here is the new function I propose:
(defun modus-themes--retrieve-palette-value (color palette)
"Return COLOR from PALETTE.
Use recursion until COLOR is retrieved as a string. Refrain from
doing so if the value of COLOR is not a key in the PALETTE.
Return `unspecified' if the value of COLOR cannot be determined.
This symbol is accepted by faces and is thus harmless.
This function is used in the macros `modus-themes-theme',
`modus-themes-with-colors'."
(let ((value (car (alist-get color palette))))
(cond
((or (stringp value)
(eq value 'unspecified))
value)
((and (symbolp value)
(memq value (mapcar #'car palette)))
(modus-themes--retrieve-palette-value value palette))
(t
'unspecified))))
I tested it with 3 levels of recursion, like:
(setq modus-themes-common-palette-overrides
'((rainbow-0 fg-main)
(rainbow-1 rainbow-0)
(rainbow-2 rainbow-1)
(rainbow-3 rainbow-2)))
(modus-themes--retrieve-palette-value
'rainbow-3
(append modus-themes-common-palette-overrides modus-operandi-palette))
I also check for bogus values with this:
(setq modus-themes-common-palette-overrides
'((rainbow-0 fg-main)
(rainbow-1 rainbow-0)
(rainbow-2 rainbow-1)
(rainbow-3 incoooorect)))
(modus-themes--retrieve-palette-value
'rainbow-3
(append modus-themes-common-palette-overrides modus-operandi-palette))
And then the standard case of what we already had where a named colour
is the value:
(setq modus-themes-common-palette-overrides
'((rainbow-0 fg-main)
(rainbow-1 rainbow-0)
(rainbow-2 rainbow-1)
(rainbow-3 magenta)))
(modus-themes--retrieve-palette-value
'rainbow-3
(append modus-themes-common-palette-overrides modus-operandi-palette))
The following diff implements this function in the macros mentioned in
its doc string. Is there any chance that you can test this before I
merge it into 'main'? It seems to work on my end, but we need to be
sure. I can put it in its own branch, if it helps.
All the best,
Prot
modus-themes.el | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/modus-themes.el b/modus-themes.el
index f2c4eac..e9f6508 100644
--- a/modus-themes.el+++ b/modus-themes.el
@@ -3778,6 +3778,24 @@ ;;; Theme macros
;;;; Instantiate a Modus theme
+(defun modus-themes--retrieve-palette-value (color palette)+ "Return COLOR from PALETTE.+Use recursion until COLOR is retrieved as a string. Refrain from+doing so if the value of COLOR is not a key in the PALETTE.++This function is used in the macros `modus-themes-theme',+`modus-themes-with-colors'."+ (let ((value (car (alist-get color palette))))+ (cond+ ((or (stringp value)+ (eq value 'unspecified))+ value)+ ((and (symbolp value)+ (memq value (mapcar #'car palette)))+ (modus-themes--retrieve-palette-value value palette))+ (t+ 'unspecified))))+;;;###autoload
(defmacro modus-themes-theme (name palette &optional overrides)
"Bind NAME's color PALETTE around face specs and variables.
@@ -3795,11 +3813,7 @@ (defmacro modus-themes-theme (name palette &optional overrides)
(,sym (append ,overrides modus-themes-common-palette-overrides ,palette))
,@(mapcar (lambda (color)
(list color
- `(let* ((value (car (alist-get ',color ,sym))))- (if (or (stringp value)- (eq value 'unspecified))- value- (car (alist-get value ,sym))))))+ `(modus-themes--retrieve-palette-value ',color ,sym))) colors))
(ignore c ,@colors) ; Silence unused variable warnings
(custom-theme-set-faces ',name ,@modus-themes-faces)
@@ -3821,11 +3835,7 @@ (defmacro modus-themes-with-colors (&rest body)
(,sym (modus-themes--current-theme-palette :overrides))
,@(mapcar (lambda (color)
(list color
- `(let* ((value (car (alist-get ',color ,sym))))- (if (or (stringp value)- (eq value 'unspecified))- value- (car (alist-get value ,sym))))))+ `(modus-themes--retrieve-palette-value ',color ,sym))) colors))
(ignore c ,@colors) ; Silence unused variable warnings
,@body)))
--
Protesilaos Stavrou
https://protesilaos.com
Protesilaos Stavrou <info@protesilaos.com> writes:
>> I feel like this is an issue in reconciliation with the preset, though I>> can't tell how `bg-hl-line` is any different to `bg-magenta-intense`.>> The `modus-themes-list-colors` command lists `bg-hl-line` too so it is a>> named colour right? It shouldn't be susceptible to recursion>> limitation.>> The reason is that the preset remaps 'bg-hl-line' to 'bg-cyan-subtle',> thus undoing the original named colour and introducing recursion.>> Maybe there is a smart way to guard against this but I need to think> more about it. Any ideas are most welcome!
Oh, I should have thought of this, makes total sense.
>> (also apologies if you are aware of this, or if I am missing something in>> the documentation)>> The documentation can probably be improved though I think here we need> something on the coding front that "just works".
Beyond a guard, I wonder if the assumption that user didn't mean to
introduce recursion could simplify things for everyone's benefit. For
example if there is an (a -> b) mapping as well as (b -> c) or (b -> d),
I wonder if the latter could be safely ignored due to the existence of
the former.
For example, this will not throw an error because it discards 'bg-hl-line'
remap to 'bg-magenta-intense'.
```
(setq modus-themes-common-palette-overrides
(seq-uniq
`((bg-paren-match bg-hl-line)
,@modus-themes-preset-overrides-intense)
(lambda (e1 e2) (equal (cadr e1) (car e2)))))
```
I am personally okay with that, but this might be too presumptuous to do
by default. An option might be to provide a wrapper function that uses
seq-uniq, but not have it applied by default.
A full blown error message pointing to precise source of recursion might
be nice, but I think it will require an intermediary hash table to do so,
which might complicate codebase in many places.
> From: Imran Khan <contact@imrankhan.live>> Date: Wed, 4 Jan 2023 15:26:59 +0600> [... 17 lines elided]>>> (also apologies if you are aware of this, or if I am missing something in>>> the documentation)>>>> The documentation can probably be improved though I think here we need>> something on the coding front that "just works".>> Beyond a guard, I wonder if the assumption that user didn't mean to> introduce recursion could simplify things for everyone's benefit. For> example if there is an (a -> b) mapping as well as (b -> c) or (b -> d),> I wonder if the latter could be safely ignored due to the existence of> the former. > [... 19 lines elided]
Please check the other message I sent a few minutes ago. It includes
Christian Tietze in the disucssion. I am now making it possible to use
recursion. This way the code "just works" and we do not need to worry
about documenting any kind of idiosyncratic limitations.
--
Protesilaos Stavrou
https://protesilaos.com
Protesilaos Stavrou <info@protesilaos.com> writes:
>> From: Imran Khan <contact@imrankhan.live>>> Date: Wed, 4 Jan 2023 15:26:59 +0600>>> [... 17 lines elided]>>>>> (also apologies if you are aware of this, or if I am missing something in>>>> the documentation)>>>>>> The documentation can probably be improved though I think here we need>>> something on the coding front that "just works".>>>> Beyond a guard, I wonder if the assumption that user didn't mean to>> introduce recursion could simplify things for everyone's benefit. For>> example if there is an (a -> b) mapping as well as (b -> c) or (b -> d),>> I wonder if the latter could be safely ignored due to the existence of>> the former. >>> [... 19 lines elided]>> Please check the other message I sent a few minutes ago. It includes> Christian Tietze in the disucssion. I am now making it possible to use> recursion. This way the code "just works" and we do not need to worry> about documenting any kind of idiosyncratic limitations.>> -- > Protesilaos Stavrou> https://protesilaos.com
I like that solution! It has solved my original complaint, and has the
nice property of being order insensitive. Plus in case user unwittingly
introduces mutual recursion, there is a swift stack overflow rather than
a hang up ;)
> From: Imran Khan <contact@imrankhan.live>> Date: Wed, 4 Jan 2023 16:15:30 +0600>> [... 30 lines elided]> I like that solution! It has solved my original complaint, and has the> nice property of being order insensitive. Plus in case user unwittingly> introduces mutual recursion, there is a swift stack overflow rather than> a hang up ;)
Very well! I have been testing it since I posted it earlier. I think
it is ready to be used.
You are right about the case of mutual recursion. I feel we don't need
to guard against that scenario, though we can think about it further if
need be.
Will push the changes now.
--
Protesilaos Stavrou
https://protesilaos.com