bug#61188: 30.0.50; color-lighten-name seems not to work

bug#61188: 30.0.50; color-lighten-name seems not to work

[Mark Bestley]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 2483 bytes --]

bug#61188: 30.0.50; color-lighten-name seems not to work

[Stephen Berman]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

On Mon, 30 Jan 2023 21:48:20 +0000 "Mark Bestley" <gnu@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@gnus.org>
Commit:     Lars Ingebrigtsen <larsi@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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: color-lighten-hsl patch --]
[-- Type: text/x-patch, Size: 585 bytes --]

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.

bug#61188: 30.0.50; color-lighten-name seems not to work

[Eli Zaretskii]

> Cc: 61188@debbugs.gnu.org
> From: Stephen Berman <stephen.berman@gmx.net>
> Date: Mon, 30 Jan 2023 23:58:26 +0100
> 
> On Mon, 30 Jan 2023 21:48:20 +0000 "Mark Bestley" <gnu@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@gnus.org>
> Commit:     Lars Ingebrigtsen <larsi@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.

bug#61188: 30.0.50; color-lighten-name seems not to work

[Stephen Berman]

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

>> Cc: 61188@debbugs.gnu.org
>> From: Stephen Berman <stephen.berman@gmx.net>
>> Date: Mon, 30 Jan 2023 23:58:26 +0100
>>
>> On Mon, 30 Jan 2023 21:48:20 +0000 "Mark Bestley" <gnu@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@gnus.org>
>> Commit:     Lars Ingebrigtsen <larsi@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

bug#61188: 30.0.50; color-lighten-name seems not to work

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: gnu@bestley.co.uk,  61188@debbugs.gnu.org
> Date: Tue, 31 Jan 2023 21:34:24 +0100
> 
> On Tue, 31 Jan 2023 20:39:45 +0200 Eli Zaretskii <eliz@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?

bug#61188: 30.0.50; color-lighten-name seems not to work

[Mark Bestley]

On Wed, 1 Feb 2023, at 13:21, Eli Zaretskii wrote:
>> From: Stephen Berman <stephen.berman@gmx.net>
>> Cc: gnu@bestley.co.uk,  61188@debbugs.gnu.org
>> Date: Tue, 31 Jan 2023 21:34:24 +0100
>> 
>> On Tue, 31 Jan 2023 20:39:45 +0200 Eli Zaretskii <eliz@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

bug#61188: 30.0.50; color-lighten-name seems not to work

[Eli Zaretskii]

> Date: Wed, 01 Feb 2023 14:11:13 +0000
> From: "Mark Bestley" <gnu@bestley.co.uk>
> Cc: 61188@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.

bug#61188: 30.0.50; color-lighten-name seems not to work

[Mark Bestley]

[-- Attachment #1: Type: text/plain, Size: 2309 bytes --]

 * 

> On 1 Feb 2023, at 17:18, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> Date: Wed, 01 Feb 2023 14:11:13 +0000
>> From: "Mark Bestley" <gnu@bestley.co.uk>
>> Cc: 61188@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,

[-- Attachment #2: Type: text/html, Size: 4629 bytes --]

bug#61188: 30.0.50; color-lighten-name seems not to work

[Mark Bestley]

[-- Attachment #1: Type: text/plain, Size: 2718 bytes --]



On Thu, 2 Feb 2023, at 15:46, Mark Bestley wrote:
>  * 
> 
>> On 1 Feb 2023, at 17:18, Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>>> Date: Wed, 01 Feb 2023 14:11:13 +0000
>>> From: "Mark Bestley" <gnu@bestley.co.uk>
>>> Cc: 61188@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



[-- Attachment #2: Type: text/html, Size: 7363 bytes --]

bug#61188: 30.0.50; color-lighten-name seems not to work

[Bernd Rellermeyer]

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.