GNU bug report logs - #61188
30.0.50; color-lighten-name seems not to work

Previous Next

To reply to this bug, email your comments to 61188 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Mark Bestley" <gnu <at> bestley.co.uk>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; color-lighten-name seems not to work
Date: Mon, 30 Jan 2023 21:48:20 +0000

[Message part 1 (text/plain, inline)]

Look at the results of

(require 'color)
(message "reduce by 100 = %s" (color-lighten-name "Black" 100))
(message "reduce by 0 = %s" (color-lighten-name "Black" 0))

In emacs 28.2 they give "#ffffffffffff" and 0 as expected.
In emacs 30.0.50 they give 0 and 0

In GNU Emacs 30.0.50 (build 1, aarch64-apple-darwin22.2.0, NS appkit-2299.30
Version 13.1 (Build 22C65)) of 2023-01-02 built on mini20.local
Windowing system distributor 'Apple', version 10.3.2299
System Description:  macOS 13.1

Configured using:
'configure --prefix=/opt/local --disable-silent-rules --without-dubs
--without-gconf --without-libotf --without-m17n-flt --with-libgmp
--with-gnutls --with-json --with-xml2 --with-modules --infodir
/opt/local/share/info/emacs --with-sqlite3 --with-webp --with-ns
--with-lcms2 --without-harfbuzz --without-imagemagick --without-xaw3d
--with-tree-sitter --with-rsvg --with-xwidgets
--with-native-compilation=aot 'CFLAGS=-pipe -Os -Wno-attributes
-isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk -arch arm64'
'CPPFLAGS=-I/opt/local/include
-isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk'
'LDFLAGS=-L/opt/local/lib -Wl,-headerpad_max_install_names -Wl,-rpath
/opt/local/lib/gcc12 -Wl,-no_pie
-Wl,-syslibroot,/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk -arch
arm64''

Configured features:
ACL GIF GLIB GMP GNUTLS JPEG JSON LCMS2 LIBXML2 MODULES NATIVE_COMP NOTIFY
KQUEUE NS PDUMPER PNG RSVG SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS
TREE_SITTER WEBP XIM XWIDGETS ZLIB

Important settings:
  value of $LANG: en_GB.UTF-8
  locale-coding-system: utf-8

Major mode: ELisp/d

[Message part 2 (text/html, inline)]

Message #8 received at 61188 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: "Mark Bestley" <gnu <at> bestley.co.uk>
Cc: 61188 <at> debbugs.gnu.org
Subject: Re: bug#61188: 30.0.50; color-lighten-name seems not to work
Date: Mon, 30 Jan 2023 23:58:26 +0100

[Message part 1 (text/plain, inline)]

On Mon, 30 Jan 2023 21:48:20 +0000 "Mark Bestley" <gnu <at> bestley.co.uk> wrote:

> Look at the results of
>
> (require 'color)
> (message "reduce by 100 = %s" (color-lighten-name "Black" 100))
> (message "reduce by 0 = %s" (color-lighten-name "Black" 0))
>
> In emacs 28.2 they give "#ffffffffffff" and 0 as expected.
> In emacs 30.0.50 they give 0 and 0

This difference is due to this commit:

commit 656c2dd66e77a5fbeb99d358017e8327401fae05
Author:     Lars Ingebrigtsen <larsi <at> gnus.org>
Commit:     Lars Ingebrigtsen <larsi <at> gnus.org>
CommitDate: Tue Mar 22 15:28:02 2022 +0100

    Fix color-lighten-hsl logic

    * lisp/color.el (color-lighten-hsl): Lighten by percentage,
    instead of just adding the specified number to the luminance
    element (bug#54514).

The patch below restores the Emacs 28 result for the above examples
while keeping the desired result for the example in bug#54514, but I
have no idea if it yields undesirable results in other cases.

Steve Berman

[Message part 2 (text/x-patch, inline)]

diff --git a/lisp/color.el b/lisp/color.el
index f68cf5e6b17..a251b1a24a0 100644
--- a/lisp/color.el
+++ b/lisp/color.el
@@ -407,7 +407,7 @@ color-lighten-hsl
 Given a color defined in terms of hue, saturation, and luminance
 \(arguments H, S, and L), return a color that is PERCENT lighter.
 Returns a list (HUE SATURATION LUMINANCE)."
-  (list H S (color-clamp (+ L (* L (/ percent 100.0))))))
+  (list H S (color-clamp (+ L (* (if (> L 0) L 1) (/ percent 100.0))))))

 (defun color-lighten-name (name percent)
   "Make a color with a specified NAME lighter by PERCENT.

Message #11 received at 61188 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 61188 <at> debbugs.gnu.org, gnu <at> bestley.co.uk
Subject: Re: bug#61188: 30.0.50; color-lighten-name seems not to work
Date: Tue, 31 Jan 2023 20:39:45 +0200

> Cc: 61188 <at> debbugs.gnu.org
> From: Stephen Berman <stephen.berman <at> gmx.net>
> Date: Mon, 30 Jan 2023 23:58:26 +0100
> 
> On Mon, 30 Jan 2023 21:48:20 +0000 "Mark Bestley" <gnu <at> bestley.co.uk> wrote:
> 
> > Look at the results of
> >
> > (require 'color)
> > (message "reduce by 100 = %s" (color-lighten-name "Black" 100))
> > (message "reduce by 0 = %s" (color-lighten-name "Black" 0))
> >
> > In emacs 28.2 they give "#ffffffffffff" and 0 as expected.
> > In emacs 30.0.50 they give 0 and 0
> 
> This difference is due to this commit:
> 
> commit 656c2dd66e77a5fbeb99d358017e8327401fae05
> Author:     Lars Ingebrigtsen <larsi <at> gnus.org>
> Commit:     Lars Ingebrigtsen <larsi <at> gnus.org>
> CommitDate: Tue Mar 22 15:28:02 2022 +0100
> 
>     Fix color-lighten-hsl logic
> 
>     * lisp/color.el (color-lighten-hsl): Lighten by percentage,
>     instead of just adding the specified number to the luminance
>     element (bug#54514).
> 
> The patch below restores the Emacs 28 result for the above examples
> while keeping the desired result for the example in bug#54514, but I
> have no idea if it yields undesirable results in other cases.

If all the tests in color-tests.el pass after the change, please
install on the release branch.

Thanks.

Message #14 received at 61188 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 61188 <at> debbugs.gnu.org, gnu <at> bestley.co.uk
Subject: Re: bug#61188: 30.0.50; color-lighten-name seems not to work
Date: Tue, 31 Jan 2023 21:34:24 +0100

On Tue, 31 Jan 2023 20:39:45 +0200 Eli Zaretskii <eliz <at> gnu.org> wrote:

>> Cc: 61188 <at> debbugs.gnu.org
>> From: Stephen Berman <stephen.berman <at> gmx.net>
>> Date: Mon, 30 Jan 2023 23:58:26 +0100
>>
>> On Mon, 30 Jan 2023 21:48:20 +0000 "Mark Bestley" <gnu <at> bestley.co.uk> wrote:
>>
>> > Look at the results of
>> >
>> > (require 'color)
>> > (message "reduce by 100 = %s" (color-lighten-name "Black" 100))
>> > (message "reduce by 0 = %s" (color-lighten-name "Black" 0))
>> >
>> > In emacs 28.2 they give "#ffffffffffff" and 0 as expected.
>> > In emacs 30.0.50 they give 0 and 0
>>
>> This difference is due to this commit:
>>
>> commit 656c2dd66e77a5fbeb99d358017e8327401fae05
>> Author:     Lars Ingebrigtsen <larsi <at> gnus.org>
>> Commit:     Lars Ingebrigtsen <larsi <at> gnus.org>
>> CommitDate: Tue Mar 22 15:28:02 2022 +0100
>>
>>     Fix color-lighten-hsl logic
>>
>>     * lisp/color.el (color-lighten-hsl): Lighten by percentage,
>>     instead of just adding the specified number to the luminance
>>     element (bug#54514).
>>
>> The patch below restores the Emacs 28 result for the above examples
>> while keeping the desired result for the example in bug#54514, but I
>> have no idea if it yields undesirable results in other cases.
>
> If all the tests in color-tests.el pass after the change, please
> install on the release branch.

With that patch one test now fails: the one testing (color-lighten-name
"Black" 100).  This is because the tests were changed to conform to the
code change made in response to bug#54514.  So it seems there is
disagreement about what the result of (color-lighten-name "Black" 100)
should be.

Steve Berman

Message #17 received at 61188 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 61188 <at> debbugs.gnu.org, gnu <at> bestley.co.uk
Subject: Re: bug#61188: 30.0.50; color-lighten-name seems not to work
Date: Wed, 01 Feb 2023 15:21:50 +0200

> From: Stephen Berman <stephen.berman <at> gmx.net>
> Cc: gnu <at> bestley.co.uk,  61188 <at> debbugs.gnu.org
> Date: Tue, 31 Jan 2023 21:34:24 +0100
> 
> On Tue, 31 Jan 2023 20:39:45 +0200 Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
> > If all the tests in color-tests.el pass after the change, please
> > install on the release branch.
> 
> With that patch one test now fails: the one testing (color-lighten-name
> "Black" 100).  This is because the tests were changed to conform to the
> code change made in response to bug#54514.  So it seems there is
> disagreement about what the result of (color-lighten-name "Black" 100)
> should be.

So I guess the current behavior is the intended one, and we should
close this bug as wontfix?

Message #20 received at 61188 <at> debbugs.gnu.org (full text, mbox):

From: "Mark Bestley" <gnu <at> bestley.co.uk>
To: "Eli Zaretskii" <eliz <at> gnu.org>, "Stephen Berman" <stephen.berman <at> gmx.net>
Cc: 61188 <at> debbugs.gnu.org
Subject: Re: bug#61188: 30.0.50; color-lighten-name seems not to work
Date: Wed, 01 Feb 2023 14:11:13 +0000

On Wed, 1 Feb 2023, at 13:21, Eli Zaretskii wrote:
>> From: Stephen Berman <stephen.berman <at> gmx.net>
>> Cc: gnu <at> bestley.co.uk,  61188 <at> debbugs.gnu.org
>> Date: Tue, 31 Jan 2023 21:34:24 +0100
>> 
>> On Tue, 31 Jan 2023 20:39:45 +0200 Eli Zaretskii <eliz <at> gnu.org> wrote:
>> 
>> > If all the tests in color-tests.el pass after the change, please
>> > install on the release branch.
>> 
>> With that patch one test now fails: the one testing (color-lighten-name
>> "Black" 100).  This is because the tests were changed to conform to the
>> code change made in response to bug#54514.  So it seems there is
>> disagreement about what the result of (color-lighten-name "Black" 100)
>> should be.
>
> So I guess the current behavior is the intended one, and we should
> close this bug as wontfix?


No - I think the way 28.2 worked was correct. (for impact see highlight-indent.el which now does not work with a black background )

What is the expected value of (color-lighten-name  "Black" 100) as I don't know where that test is. I would think the test is wrong. Did it run for 28.2?

Surely lightening Black fully should give white

Also look at color-lighten-name  "Black" 50) in 28.2 it is a gray.

In 30 olor-lighten-name  "Black" of any positive value gives Black - surely this cannot be correct.

-- 
Mark

Message #23 received at 61188 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: "Mark Bestley" <gnu <at> bestley.co.uk>
Cc: stephen.berman <at> gmx.net, 61188 <at> debbugs.gnu.org
Subject: Re: bug#61188: 30.0.50; color-lighten-name seems not to work
Date: Wed, 01 Feb 2023 19:18:50 +0200

> Date: Wed, 01 Feb 2023 14:11:13 +0000
> From: "Mark Bestley" <gnu <at> bestley.co.uk>
> Cc: 61188 <at> debbugs.gnu.org
> 
> > So I guess the current behavior is the intended one, and we should
> > close this bug as wontfix?
> 
> No - I think the way 28.2 worked was correct. (for impact see highlight-indent.el which now does not work with a black background )

But by changing the tests to match what Emacs does now we explicitly
said that we disagree, and that the current behavior is the correct
one.

> What is the expected value of (color-lighten-name  "Black" 100) as I don't know where that test is. I would think the test is wrong. Did it run for 28.2?

The test is in test/lisp/color-tests.el.  The expected value is
exactly what you said is wrong.

> Surely lightening Black fully should give white

How so?  The 100 is the percentage of the present luminance, and if
that is zero, why do you expect it to become lighter?

See also the discussion in bug#54514, which was exactly about that.

> In 30 olor-lighten-name  "Black" of any positive value gives Black - surely this cannot be correct.

A zero multiplied by any percentage stays zero, no?  If you want a
non-zero result, start with something close to black, but not actually
black.

Message #26 received at 61188 <at> debbugs.gnu.org (full text, mbox):

From: "Mark Bestley" <gnu <at> bestley.co.uk>
To: "Eli Zaretskii" <eliz <at> gnu.org>
Cc: stephen.berman <at> gmx.net, 61188 <at> debbugs.gnu.org
Subject: Re: bug#61188: 30.0.50; color-lighten-name seems not to work
Date: Thu, 02 Feb 2023 15:46:09 +0000

[Message part 1 (text/plain, inline)]

 * 

> On 1 Feb 2023, at 17:18, Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
>> Date: Wed, 01 Feb 2023 14:11:13 +0000
>> From: "Mark Bestley" <gnu <at> bestley.co.uk>
>> Cc: 61188 <at> debbugs.gnu.org
>> 
>>> So I guess the current behavior is the intended one, and we should
>>> close this bug as wontfix?
>> 
>> No - I think the way 28.2 worked was correct. (for impact see highlight-indent.el which now does not work with a black background )
> 
> But by changing the tests to match what Emacs does now we explicitly
> said that we disagree, and that the current behavior is the correct
> one.
> 
>> What is the expected value of (color-lighten-name  "Black" 100) as I don't know where that test is. I would think the test is wrong. Did it run for 28.2?
> 
> The test is in test/lisp/color-tests.el.  The expected value is
> exactly what you said is wrong.
> 
>> Surely lightening Black fully should give white
> 
> How so?  The 100 is the percentage of the present luminance, and if
> that is zero, why do you expect it to become lighter?
> 
> See also the discussion in bug#54514, which was exactly about that.

I don't see a discussion there.
But I do understand and accept the rationale for changing * color-lighten-hsl*



> 
>> In 30 olor-lighten-name  "Black" of any positive value gives Black - surely this cannot be correct.
> 
> A zero multiplied by any percentage stays zero, no?  If you want a
> non-zero result, start with something close to black, but not actually
> black.
The issue is more with color-lighten-name and the use it has in highlight-indent.el
Here the background colour is increased or darkened so that a new background is  distinguishable from the original and it does that via varying the hue. In those terms increasing the hue from black to shades of grey and 100% takes you to white makes sense.

So just multiplying hue may not give the expected result here, The old reasoning for color-by-name might make some sense but I don't think we have the rationale why that was chosen.

So I understand why then change was made but I think that there will be broken code around which used color-lighten-name when emacs 29 is released.

Basically manipulation of coper values is more complex than at first thought.

I'll report this issue to highlight-indent.el and I think this bug can be closed,

[Message part 2 (text/html, inline)]

Message #29 received at 61188 <at> debbugs.gnu.org (full text, mbox):

From: "Mark Bestley" <gnu <at> bestley.co.uk>
To: "Eli Zaretskii" <eliz <at> gnu.org>
Cc: Stephen Berman <stephen.berman <at> gmx.net>, 61188 <at> debbugs.gnu.org
Subject: Re: bug#61188: 30.0.50; color-lighten-name seems not to work
Date: Thu, 02 Feb 2023 18:32:50 +0000

[Message part 1 (text/plain, inline)]

On Thu, 2 Feb 2023, at 15:46, Mark Bestley wrote:
>  * 
> 
>> On 1 Feb 2023, at 17:18, Eli Zaretskii <eliz <at> gnu.org> wrote:
>> 
>>> Date: Wed, 01 Feb 2023 14:11:13 +0000
>>> From: "Mark Bestley" <gnu <at> bestley.co.uk>
>>> Cc: 61188 <at> debbugs.gnu.org
>>> 
>>>> So I guess the current behavior is the intended one, and we should
>>>> close this bug as wontfix?
>>> 
>>> No - I think the way 28.2 worked was correct. (for impact see highlight-indent.el which now does not work with a black background )
>> 
>> But by changing the tests to match what Emacs does now we explicitly
>> said that we disagree, and that the current behavior is the correct
>> one.
>> 
>>> What is the expected value of (color-lighten-name  "Black" 100) as I don't know where that test is. I would think the test is wrong. Did it run for 28.2?
>> 
>> The test is in test/lisp/color-tests.el.  The expected value is
>> exactly what you said is wrong.
>> 
>>> Surely lightening Black fully should give white
>> 
>> How so?  The 100 is the percentage of the present luminance, and if
>> that is zero, why do you expect it to become lighter?
>> 
>> See also the discussion in bug#54514, which was exactly about that.
> 
> I don't see a discussion there.
> But I do understand and accept the rationale for changing * color-lighten-hsl*
> 
> 
> 
>> 
>>> In 30 olor-lighten-name  "Black" of any positive value gives Black - surely this cannot be correct.
>> 
>> A zero multiplied by any percentage stays zero, no?  If you want a
>> non-zero result, start with something close to black, but not actually
>> black.
> The issue is more with color-lighten-name and the use it has in highlight-indent.el
> Here the background colour is increased or darkened so that a new background is  distinguishable from the original and it does that via varying the hue. In those terms increasing the hue from black to shades of grey and 100% takes you to white makes sense.
> 
> So just multiplying hue may not give the expected result here, The old reasoning for color-by-name might make some sense but I don't think we have the rationale why that was chosen.
> 
> So I understand why then change was made but I think that there will be broken code around which used color-lighten-name when emacs 29 is released.
> 
> Basically manipulation of coper values is more complex than at first thought.
> 
> I'll report this issue to highlight-indent.el and I think this bug can be closed,
Sorry for the further post 

I looked at the old tests for color-lighten-hsl and see their rationale.

The old code added the percentage change to the hue the new code multiplies by the change.
So their is rationale to that but the comment for the function is not exact enough.
--
Mark

[Message part 2 (text/html, inline)]

Message #32 received at 61188 <at> debbugs.gnu.org (full text, mbox):

From: Bernd Rellermeyer <bernd.rellermeyer <at> t-online.de>
To: 61188 <at> debbugs.gnu.org
Subject: bug#61188: 30.0.50; color-lighten-name seems not to work
Date: Tue, 21 Feb 2023 14:08:29 +0100

I am not happy with the new implementation of color-lighten-hsl. I think the old implementation was correc. In my opinion there is a misunderstanding about the argument percent. The value L itself is a percentage, ranging from 0.0 to 1.0, and the argument percent is a percentage point to add to the percentage L (divided by 100). E.g. if L is 0.5 and percent is 10, the result should be 0.6 and not 0.5 * 1.1 = 0.55. In the case of the black color, the result should be 0.0 + 0.1 = 0.1. That is because percent is a percentage point and not a percentage.

I would suggest to switch back to the old implementation.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.