Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

[Po Lu]

Mattias Engdegård <mattiase@acm.org> writes:

> branch: master
> commit 6eeab90632506348a58d264eb3304625756c8659
> Author: Mattias Engdegård <mattiase@acm.org>
> Commit: Mattias Engdegård <mattiase@acm.org>
>
>     Don't accept whitespace or hex floats in rgbi: colour specs
>     
>     `color-values-from-color-spec` (new in Emacs 28) erroneously accepted
>     leading whitespace and hex floats in rgbi: components.
>     
>     Reported by Philip Kaludercic.
>     
>     * src/xfaces.c (parse_float_color_comp): Disallow leading whitespace
>     and hex floats.
>     * test/src/xfaces-tests.el
>     (xfaces-internal-color-values-from-color-spec): Add test cases.

I don't see a corresponding bug report, so please forgive me if I missed
something here.

If the leading whitespace and hex floats don't cause problems, why not
keep the ability to understand them?  It could come in handy.

Thanks.

Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

[Mattias Engdegård]

6 mars 2022 kl. 14.46 skrev Po Lu <luangruo@yahoo.com>:

> I don't see a corresponding bug report, so please forgive me if I missed
> something here.

It's bug#54263 -- sorry about the omission from the log message.

> If the leading whitespace and hex floats don't cause problems, why not
> keep the ability to understand them?  It could come in handy.

So it might seem, but over-tolerant implementation is generally a mistake. It restricts compatibility in the other direction (what worked with Emacs won't work elsewhere), restricts future changes and implementations, detects fewer user mistakes, and makes documentation and specifications less useful. It's not doing the user a service.

RE: [External] : Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

[Drew Adams]

> > If the leading whitespace and hex floats don't cause
> > problems, why not keep the ability to understand them?
> > It could come in handy.
> 
> So it might seem, but over-tolerant implementation is generally a
> mistake. It restricts compatibility in the other direction (what worked
> with Emacs won't work elsewhere), restricts future changes and
> implementations, detects fewer user mistakes, and makes documentation
> and specifications less useful. It's not doing the user a service.

+1.

Dunno about this particular case, but there are
often (at least) two different use cases for such
things.  They could both be accommodated easily
by adding an optional arg, LAX, or its opposite,
STRICT, to choose the other behavior.

The two behaviors are:

1. LAX: Allow nonexact, nonkosher, or in some
   way looser matches.

2. STRICT: Allow only exact matches.

Examples:

1. JSON - in practice - in the wild, lots of
nonstandard syntax is used.  Applications often
want to support such syntax because, well, it's
used - common.

But if you want/need to know _whether_ some
data is well-formed JSON data according to the
standard, then you don't want to allow data
that's not well-formed.

https://docs.oracle.com/en/database/oracle/oracle-database/21/adjsn/conditions-is-json-and-is-not-json.html#GUID-1B6CFFBE-85FE-41DD-BA14-DD1DE73EAB20

(There's also the notion of "validity",
relative to a JSON schema.  That's different;
there, the schema decides what's allowed.)


2. Thing-at-point - Emacs (unfortunately) doesn't
test for a THING being _at_ point.  It instead
tests for a THING being either at point or at
(1- point).

This could be considered a good thing in contexts
where all you want is to maximize the chances of
obtaining a thing from the buffer for use as, say,
a default value for input.  But it's definitely
a bad thing if what you want is to actually find
out if there is a THING at point - that is, if
you really want a THING only if it's AT point.

My own approach is to have thing-at-point and its
bounds actually check for a thing _AT_ point, and
to provide other functions for obtaining a THING
that's _near_ point, where you can control what
"near" means.  See bug #9300:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=9300

https://www.emacswiki.org/emacs/ThingAtPointPlus


3. Completion - If your aim is to maximize the
number of possible completion candidates, then a
very loose kind of matching can be good.  But if
you want to obtain a set of candidates that are
particularly appropriate, then such looseness
can just introduce noise.

This is a problem with the `completion-styles'
design of Emacs.  The fallback process, from a
failing style to the next and so on, is really
aimed at maximizing the chances of finding a
candidate of some kind (the styles define the
possible kinds - the matching is a union/OR of
all of those kinds).  Your code can't know
which kind of matching was used successfully.

My own approach lets users change completion
methods on the fly, that is, control their use
as _alternatives_.  Then you know what kind of
matching is used and is successful or not.

(I also support using `completion-styles', in
combination with other methods.  But you're
not a captive of the fallback behavior that
`completion-styles' design locks you into.)

https://www.emacswiki.org/emacs/Icicles_-_Completion_Methods_and_Styles#VanillaEmacsStylesAndOption_completing-styles

______________________

You get the point.  There are advantages to both
LAX and STRICT.  (And there can be more complex
division/choices than just a binary LAX/STRICT.)

A LAX choice can help if the priority is to get
(or support) _something_, rather than raising
an error or returning nil or NULL.  A STRICT
choice is useful if you want to know _whether_
there's a match, and not just obtain _something_.
___

Without bothering to understand more about this
RGBI thing, a naive suggestion would be to add
an optional STRICT arg.  That would leave the
behavior as it is now, by default (so backward
compatible, if that's important).  But it would
also let you test for an exact match.

(If this is a new feature, so there's no need to
keep the existing behavior by default, then you
could change it to be STRICT by default and
accept an optional LAX arg.)
___

We should keep this kind of thing in mind.

Pretty much any time there's some matching or
built-in predicate-satisfying involved, it's
worth thinking about whether it makes sense to
either add an optional argument to provide
either (1) a common "opposite" behavior (LAX or
STRICT) or (2) a predicate, or other function,
to specify the behavior (overriding the default
behavior).

This hopefully all seems obvious.  But I think
it's not always kept in mind.

Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

[Po Lu]

Mattias Engdegård <mattiase@acm.org> writes:

> It's bug#54263 -- sorry about the omission from the log message.

Thanks.

> So it might seem, but over-tolerant implementation is generally a
> mistake. It restricts compatibility in the other direction (what
> worked with Emacs won't work elsewhere)

AFAICT, there are no other implementations of such an rgbi color
specification (please correct me if I'm wrong.)

> restricts future changes and implementations, detects fewer user
> mistakes, and makes documentation and specifications less useful. It's
> not doing the user a service.

I don't think there's a chance that the user will type any of those by
mistake.  Besides, the documentation can always be amended, and that
function has no other specification.  So I disagree that it doesn't do
the user a service.

Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

[Philip Kaludercic]

Po Lu <luangruo@yahoo.com> writes:

> Mattias Engdegård <mattiase@acm.org> writes:
>
>> It's bug#54263 -- sorry about the omission from the log message.
>
> Thanks.
>
>> So it might seem, but over-tolerant implementation is generally a
>> mistake. It restricts compatibility in the other direction (what
>> worked with Emacs won't work elsewhere)
>
> AFAICT, there are no other implementations of such an rgbi color
> specification (please correct me if I'm wrong.)

It seems the "rgbi" (that stands for RGB Intensity) syntax comes from
X11[0], specifically "XCMS" (X Color Management System) Interestingly
enough, if you look around you can find that the "rgbi" prefix seems
to be case-insensitive[1], while `color-values-from-color-spec' is not:

    (color-values-from-color-spec "rgbi:0/0/0") ;; => (0 0 0)
    (color-values-from-color-spec "rgbI:0/0/0") ;; => nil
    (color-values-from-color-spec "RGBi:0/0/0") ;; => nil
    (color-values-from-color-spec "RGBI:0/0/0") ;; => nil

The closes thing to a specification I could find was [2], that says:

        1.3.2.2.  RGB Intensity	String Specification

        An RGB intensity specification in the form of a	string is
        identified by the prefix "rgbi:" and whose string conforms
        to the following syntax:

            rgbi:<red>/<green>/<blue>

        Where red, green, and blue are floating	point values between
        0.0 and	1.0, inclusive.	 The input format for these values
        is an optional sign, a string of numbers possibly containing
        a decimal point, and an	optional exponent field	containing
        an E or	e followed by a	possibly signed	integer	string.

[0] https://man.archlinux.org/man/extra/libx11/XLookupColor.3.en.
[1] https://www.niksula.hut.fi/~jkirma/books/xlib.pdf, section 7.2.2,
    https://www.klauser.ch/lxug/ch14.pdf, page 18.
[2] http://www.nic.funet.fi/pub/X11/X11R4/DOCS/color/Xcms.text

-- 
	Philip Kaludercic

Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

[Mattias Engdegård]

7 mars 2022 kl. 01.34 skrev Po Lu <luangruo@yahoo.com>:

> AFAICT, there are no other implementations of such an rgbi color
> specification (please correct me if I'm wrong.)

The syntax is from X11, or at least that is why Emacs supports it.

> I don't think there's a chance that the user will type any of those by
> mistake.  Besides, the documentation can always be amended, and that
> function has no other specification.  So I disagree that it doesn't do
> the user a service.

These unintended extensions do not provide the user with any additional expressive power so there seems to be little reason for them. For that matter, I believe that extensions should be motivated by needs, not by implementation happenstance. Don't you agree?

In any case we are debating angels dancing on a pinhead. I just fixed it because I made the mistake to begin with, and I don't think you should elevate my errors to holy scripture quite yet (please wait until I'm dead).

Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

[Po Lu]

Mattias Engdegård <mattiase@acm.org> writes:

> The syntax is from X11, or at least that is why Emacs supports it.

But the X11 intensity string specification doesn't accept whitespace or
hexidecimal values, so it's different from the rgbi specification that
was implemented in Emacs.  Though in practice, tolerance for whitespace
is X server-specific.  For the same reason, we don't use XParseColor to
parse color specifications.

> These unintended extensions do not provide the user with any
> additional expressive power so there seems to be little reason for
> them. For that matter, I believe that extensions should be motivated
> by needs, not by implementation happenstance. Don't you agree?

If the happenstance doesn't cause trouble, there is no need to get rid
of it.

Thanks.

Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

[Po Lu]

Philip Kaludercic <philipk@posteo.net> writes:

> It seems the "rgbi" (that stands for RGB Intensity) syntax comes from
> X11[0], specifically "XCMS" (X Color Management System) Interestingly
> enough, if you look around you can find that the "rgbi" prefix seems
> to be case-insensitive[1], while `color-values-from-color-spec' is not:
>
>     (color-values-from-color-spec "rgbi:0/0/0") ;; => (0 0 0)
>     (color-values-from-color-spec "rgbI:0/0/0") ;; => nil
>     (color-values-from-color-spec "RGBi:0/0/0") ;; => nil
>     (color-values-from-color-spec "RGBI:0/0/0") ;; => nil
>
> The closes thing to a specification I could find was [2], that says:
>
>         1.3.2.2.  RGB Intensity	String Specification
>
>         An RGB intensity specification in the form of a	string is
>         identified by the prefix "rgbi:" and whose string conforms
>         to the following syntax:
>
>             rgbi:<red>/<green>/<blue>
>
>         Where red, green, and blue are floating	point values between
>         0.0 and	1.0, inclusive.	 The input format for these values
>         is an optional sign, a string of numbers possibly containing
>         a decimal point, and an	optional exponent field	containing
>         an E or	e followed by a	possibly signed	integer	string.

See my reply to Mattias, that's not the RGBi specification I'm talking
about.

Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

[Mattias Engdegård]

7 mars 2022 kl. 11.16 skrev Po Lu <luangruo@yahoo.com>:

> See my reply to Mattias, that's not the RGBi specification I'm talking
> about.

Actually it is the spec that we are implementing. Let's recap the pertinent history, very roughly:

1. Emacs gets X11 support, and uses XParseColor.
2. XCMS is added to X11, so Emacs now understands rgbi:0/0.5/1 in addition to #fcb8a9 and SlimeGreen, but hardly anyone uses it.
3. Emacs is ported to Windows. For compatibility, the backend parses the rgbi: syntax just in case.
4. Emacs is ported to NextStep (etc), but the backend omits the rgbi: syntax. Nobody complains.
5. The colour spec parsing from all backends is consolidated to a single implementation and naturally it includes rgbi:.

Not only is the rgbi: syntax very little used, until recently it wasn't even available on all platforms.

Thus this doesn't matter much at all, and it's been fixed now because it was reported as a bug, and it was clearly my mistake to mend. I wouldn't mind dropping the support for rgbi: completely if that is all right with the maintainers.

Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

[Po Lu]

Mattias Engdegård <mattiase@acm.org> writes:

> 2. XCMS is added to X11, so Emacs now understands rgbi:0/0.5/1 in
> addition to #fcb8a9 and SlimeGreen, but hardly anyone uses it.

It happened as part of the changes in X11R5 to support
device-independent color, but it was never part of the Xcms.  This is
all unrelated though.

> 5. The colour spec parsing from all backends is consolidated to a
> single implementation and naturally it includes rgbi:.

And at this point, the `rgbi:' syntax in Emacs stops implementing the
syntax understood by the X server.  So there is no point in limiting it
to what is understood by the X server again, since the new syntax
doesn't break anything.

Thanks.

Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

[Philip Kaludercic]

Po Lu <luangruo@yahoo.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> It seems the "rgbi" (that stands for RGB Intensity) syntax comes from
>> X11[0], specifically "XCMS" (X Color Management System) Interestingly
>> enough, if you look around you can find that the "rgbi" prefix seems
>> to be case-insensitive[1], while `color-values-from-color-spec' is not:
>>
>>     (color-values-from-color-spec "rgbi:0/0/0") ;; => (0 0 0)
>>     (color-values-from-color-spec "rgbI:0/0/0") ;; => nil
>>     (color-values-from-color-spec "RGBi:0/0/0") ;; => nil
>>     (color-values-from-color-spec "RGBI:0/0/0") ;; => nil
>>
>> The closes thing to a specification I could find was [2], that says:
>>
>>         1.3.2.2.  RGB Intensity	String Specification
>>
>>         An RGB intensity specification in the form of a	string is
>>         identified by the prefix "rgbi:" and whose string conforms
>>         to the following syntax:
>>
>>             rgbi:<red>/<green>/<blue>
>>
>>         Where red, green, and blue are floating	point values between
>>         0.0 and	1.0, inclusive.	 The input format for these values
>>         is an optional sign, a string of numbers possibly containing
>>         a decimal point, and an	optional exponent field	containing
>>         an E or	e followed by a	possibly signed	integer	string.
>
> See my reply to Mattias, that's not the RGBi specification I'm talking
> about.

The point remains that if rgbi: is supported, it should be
case-insensitive.

-- 
	Philip Kaludercic

RE: [External] : Re: master 6eeab90632: Don't accept whitespace or hex floats in rgbi: colour specs

[Drew Adams]

 
> If the happenstance doesn't cause trouble, there is no need to get rid
> of it.

As I suggested, mentioning other, similar contexts:

 But if you want/need to know _whether_ some
 data is well-formed ______ according to the
 standard, then you don't want to allow data
 that's not well-formed.

Whether or not you want to support such a use case,
i.e., _checking_ whether something is ONLY an rgbi
code and not something that also includes whitespace,
is up to you.  But it's easy to provide users with
both behaviors, by adding an optional arg (LAX or
STRICT), depending on what you want the default
behavior to be.