~protesilaos/modus-themes

7 3

[Bug?] Unable to override *some* named colours, when combining with a preset

Details
Message ID
<b7ca4702162fd575593f8ded28d9a888.contact@imrankhan.live>
DKIM signature
missing
Download raw message
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)
Details
Message ID
<87bknf6nfe.fsf@protesilaos.com>
In-Reply-To
<b7ca4702162fd575593f8ded28d9a888.contact@imrankhan.live> (view parent)
DKIM signature
missing
Download raw message
> 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
Details
Message ID
<5afcb3b5-fd86-482a-ad4f-ccee06eea603@app.fastmail.com>
In-Reply-To
<87bknf6nfe.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
> 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
Details
Message ID
<877cy27isj.fsf@protesilaos.com>
In-Reply-To
<5afcb3b5-fd86-482a-ad4f-ccee06eea603@app.fastmail.com> (view parent)
DKIM signature
missing
Download raw message
Patch: +20 -10
> 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
Details
Message ID
<468eb18ada44c0ffff667289f2bd974c.contact@imrankhan.live>
In-Reply-To
<87bknf6nfe.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
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.
Details
Message ID
<87tu16iprb.fsf@protesilaos.com>
In-Reply-To
<468eb18ada44c0ffff667289f2bd974c.contact@imrankhan.live> (view parent)
DKIM signature
missing
Download raw message
> 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
Details
Message ID
<6fa78b2cedd6a60c5a542ab5f65bb915.contact@imrankhan.live>
In-Reply-To
<87tu16iprb.fsf@protesilaos.com> (view parent)
DKIM signature
missing
Download raw message
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 ;)
Details
Message ID
<87r0waioo1.fsf@protesilaos.com>
In-Reply-To
<6fa78b2cedd6a60c5a542ab5f65bb915.contact@imrankhan.live> (view parent)
DKIM signature
missing
Download raw message
> 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
Reply to thread Export thread (mbox)