bug#15900: 24.3.50; foreground-color-at-point returns wrong results

[Drew Adams <drew.adams@oracle.com>]

> CCing Drew cause the Change Log mentions him with regard to
> `foreground-color-at-point'.

Thanks, Michael.  I wasn't reading this thread.

1. Let me first butt in with this preliminary comment, having now
diff'd the current code (as of an MS Windows snapshot from 11/12)
against the Emacs 24.3 code (which is the same as what I submitted).

I think it is a mistake to fudge `face-at-point' so that it returns
a face named in the buffer.  That is NOT at all "the face of the
character after point", which is what the doc string first line
says.  And it is (was) not the intention of `face-at-point' at all.
See below for a suggestion, for those who want a face named at point.

In addition, that argument should not be called THING.  It is NOT
(as is usually the case for an argument so named) a symbol or a
string naming an Emacs THING, as in `thingatpt.el'.  It is just a
Boolean which if non-nil means to try first for a face name at point.  
IOW, it should be called something like CHECK-FOR-FACE-NAMED-IN-TEXT.

2. Now on to the addition of argument MULTIPLE, which is mainly what
this bug is about, IIUC.

In the bug report, Michael said this:

  it looks only at one face and disregards the others.  This is
  especially meaningless when this first face doesn't specify any
  foreground at all.

Yes, `face-at-point' returned only the first of a list of faces.

No, it is not meaningless for `face-at-point' to return a face that
does not specify a foreground.  Or does not specify a background.
Or an underline.  Or any other face attribute.  By default, it was
designed to return face `default' if no other face was found at
point but if another face was found it returned that face.

`face-at-point' should give you a face at point, regardless of the
attributes that face might specify explicitly.

Does it make sense for `foreground-color-at-point' to return nil
if the face returned by `face-at-point' specifies no foreground?

Yes.  It was designed to return the color specified by a given
face, and if no such color was specified, to return nil.  But it
uses `face-at-point', which looks at face `default' if no other
face is found at point.

If there is a non-`default' face at point, then its foreground
should be used, and if there is none, then nil should be the result.

3. But MULTIPLE is something else.  It aims to give you all of the
faces at point, whether used as a text property or an overlay
property.  Like THING, MULTIPLE was added after I contributed the
code.  But unlike THING, MULTIPLE is a good addition.

I can imagine that it might be useful to optionally get only faces
used as a text property or only those used as an overlay property.
Michael's use case is apparently one such.

Perhaps `*face-at-point' should provide an argument for this.
(That would certainly be more useful than the THING argument,
which has nothing to do with the properties at point.)

It looks like there is a bug wrt the use of `get-char-property'.
`get-char-property' itself always favors an overlay property over
the same property used as a text property at the same position.

Furthermore, this important part of the `get-char-property'
behavior is not even mentioned in the doc string (it is mentioned
in the Elisp manual).  This DOC BUG should be the first thing to
be fixed.

Given this limitation, it seems that the right thing for
`face-at-point' to do is to gather faces using both
`get-text-property' and `get-overlay-property', and not
`get-char-property'.  And it might be good to add an argument that
lets you specify one or the other of these or both.

Given that fix, MULTIPLE should DTRT.  And Michael should be
have what he needs.

4. So I would propose the following:

a. REMOVE the misnamed argument THING altogether.  It does not
belong in `face-at-point'.  Create a function
`face-named-at-point' that returns the face named at point.
Anyone wanting the behavior of first-look-for-face-named-...
would use (or (face-named-at-point...) (face-at-point...)).
The time to make this change is NOW, before 24.4 is released.

b. Add an optional argument TYPE, with a nil value meaning
both overlay and text property.  Possible values would be
`overlay', `text', and nil.

c. TYPE would be taken into account regardless of the value
of MULTIPLE.  For non-nil MULTIPLE the function would return a
list of all faces at point, whether in overlays or the `face'
text property.  `face-at-point' would use `get-text-property'
or `get-overlay-property', or both, and not `get-char-property'.

> > As you later discovered, even (face-at-point nil t) doesn't
> > do the job.  Which doesn't surprise me a bit: this kind of
> > things cannot be done reliably from Lisp

I don't see any basis for saying that.  See above.  Sounds
pretty straightforward in Lisp, to me.  What am I missing?

> > even at a price of the kind of obfuscated code that
> > face-at-point and foreground-color-at-point use. 

Care to back that up?  Just where do you see obfuscation?

> > It is much simpler to write a C primitive that simulates
> > the display, then returns the resulting face at a given
> > character position, a simple and straightforward task on
> > the C level, something the display engine does all the
> > time.

Overengineering, IMO.  Not needed for `face-at-point', that
I can see.