bug#64075: 28.2; ispell broken on uncolored terminals

bug#64075: 28.2; ispell broken on uncolored terminals

[Al Petrofsky]

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

   emacs-28.2 -Q -nw --color=no
   s t o n k s M-$ 1

At this point, I expect to see the word "stonks" replaced with
"stocks", but it is instead replaced with "stockstonks".

The problem seems to be in ispell-highlight-spelling-error-generic, a
20th-century kludge from before face support for ttys was added in
emacs-21 in 2001.  It's been unnecessary since then, and it looks like
it's been breaking ispell on colorless ttys since at least emacs-23.

Changing "(display-color-p)" to "t" in ispell-highlight-spelling-error
suffices to fix the bug.

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

bug#64075: 28.2; ispell broken on uncolored terminals

[Gregory Heytings]

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


> 
> emacs-28.2 -Q -nw --color=no
> s t o n k s M-$ 1
>
> At this point, I expect to see the word "stonks" replaced with "stocks", 
> but it is instead replaced with "stockstonks".
> 
> The problem seems to be in ispell-highlight-spelling-error-generic, a 
> 20th-century kludge from before face support for ttys was added in 
> emacs-21 in 2001.  It's been unnecessary since then, and it looks like 
> it's been breaking ispell on colorless ttys since at least emacs-23.
>

This recipe can be reproduced with Emacs 25, 26, 27 and 28... but not with 
Emacs 29, in which "stonks" is indeed replaced with "stocks".

bug#64075: 28.2; ispell broken on uncolored terminals

[Al Petrofsky]

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

Ah, I see Eli fixed ispell-highlight-spelling-error-generic last year.
http://git.savannah.gnu.org/cgit/emacs.git/commit/lisp/textmodes/ispell.el?id=f3b876fa75042a1c00886e07d8491ac11824a892

Still, this anachronistic kludge should really be nuked entirely.
Even with the current fix, it fails to use ispell-highlight-face,
instead always using inverse-video.

By "nuked entirely" I mean:
   (1) delete ispell-highlight-spelling-error
   (2) delete ispell-highlight-spelling-error-generic
   (3) rename ispell-highlight-spelling-error-overlay
           to ispell-highlight-spelling-error

On Wed, Jun 14, 2023 at 7:25 PM Gregory Heytings <gregory@heytings.org>
wrote:

>
> >
> > emacs-28.2 -Q -nw --color=no
> > s t o n k s M-$ 1
> >
> > At this point, I expect to see the word "stonks" replaced with "stocks",
> > but it is instead replaced with "stockstonks".
> >
> > The problem seems to be in ispell-highlight-spelling-error-generic, a
> > 20th-century kludge from before face support for ttys was added in
> > emacs-21 in 2001.  It's been unnecessary since then, and it looks like
> > it's been breaking ispell on colorless ttys since at least emacs-23.
> >
>
> This recipe can be reproduced with Emacs 25, 26, 27 and 28... but not with
> Emacs 29, in which "stonks" is indeed replaced with "stocks".
>

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

bug#64075: 28.2; ispell broken on uncolored terminals

[Eli Zaretskii]

> Cc: 64075@debbugs.gnu.org
> Date: Wed, 14 Jun 2023 23:25:10 +0000
> From: Gregory Heytings <gregory@heytings.org>
> 
> > emacs-28.2 -Q -nw --color=no
> > s t o n k s M-$ 1
> >
> > At this point, I expect to see the word "stonks" replaced with "stocks", 
> > but it is instead replaced with "stockstonks".
> > 
> > The problem seems to be in ispell-highlight-spelling-error-generic, a 
> > 20th-century kludge from before face support for ttys was added in 
> > emacs-21 in 2001.  It's been unnecessary since then, and it looks like 
> > it's been breaking ispell on colorless ttys since at least emacs-23.
> >
> 
> This recipe can be reproduced with Emacs 25, 26, 27 and 28... but not with 
> Emacs 29, in which "stonks" is indeed replaced with "stocks".

Yes, see bug#56219.

bug#64075: 28.2; ispell broken on uncolored terminals

[Eli Zaretskii]

> Cc: 64075@debbugs.gnu.org
> From: Al Petrofsky <al@petrofsky.org>
> Date: Wed, 14 Jun 2023 19:48:37 -0400
> 
> Still, this anachronistic kludge should really be nuked entirely.

Why?  And what's "anachronistic" about that code?

> Even with the current fix, it fails to use ispell-highlight-face,
> instead always using inverse-video.

Why is that a problem?  This function exists so that even the dumbest
terminals could be used for spell-checking with reasonable
convenience.

> By "nuked entirely" I mean:
>    (1) delete ispell-highlight-spelling-error
>    (2) delete ispell-highlight-spelling-error-generic
>    (3) rename ispell-highlight-spelling-error-overlay 
>            to ispell-highlight-spelling-error

Sorry, not going to happen.  You are suggesting to delete a useful
capability for no good reason, just because you happen to think it's
"anachronistic".

Unlike what you seem to think, display-color-p is not "a 20th-century
kludge from before face support for ttys was added in emacs-21 in
2001".  Quite to the contrary, display-color-p was introduced with
Emacs 21, to replace references to window-system (which _was_ "the
20-century kludge" for requesting faces) in a way that would support
text-mode terminals.

Now, I'm okay will adding more tests to display-color-p, for
monochrome terminals that can support other face attributes which will
make the misspelled word stand out, but then the ispell-highlight-face
should probably be modified accordingly, keeping in mind that (a) the
user could customize the 'isearch' and/or 'highlight' faces from which
it inherits, and (b) face customization is global, not specific to
frame, let alone Emacs command, and users rarely customize faces in
complex ways that can account for frame capabilities.

And in any case, we should keep that code for terminals which can only
support inverse video.  There's no reason to delete it, none
whatsoever.

bug#64075: 28.2; ispell broken on uncolored terminals

[Al Petrofsky]

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

I apologize if I'm misunderstanding things.  I'm certainly not
suggesting removing any functionality that is useful on old monochrome
ttys.  The reason I became aware of this bug is that I am using a
monochrome tty.

> > Still, this anachronistic kludge should really be nuked entirely.

> Why?  And what's "anachronistic" about that code?

What's anachronistic about that code (meaning the
ispell-highlight-spelling-error-generic function) is that it uses a
kludge that made it possible in pre-version-21 emacs to get some text
displayed in inverse video on a tty even though emacs could not
display faces on ttys.  But since Emacs 21, on any tty that has a
termcap "so" capability, you can get inverse-video simply by using an
inverse-video face.

So I don't think ispell-highlight-spelling-error-generic currently
provides any functionality that isn't more conveniently and
maintainably provided by ispell-highlight-spelling-error-overlay, with
the additional benefit that the user can, if he wants, customize
ispell-highlight-face to something other than inverse-video.

(Making the default isearch face (and therefore the default
ispell-highlight-face) be blue-on-magenta on color terminals but
inverse-video on monochrome terminals is already handled
automatically.)

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

bug#64075: 28.2; ispell broken on uncolored terminals

[Eli Zaretskii]

> From: Al Petrofsky <al@petrofsky.org>
> Date: Thu, 15 Jun 2023 02:34:48 -0400
> Cc: gregory@heytings.org, 64075@debbugs.gnu.org
> 
> What's anachronistic about that code (meaning the
> ispell-highlight-spelling-error-generic function) is that it uses a
> kludge that made it possible in pre-version-21 emacs to get some text
> displayed in inverse video on a tty even though emacs could not
> display faces on ttys.  But since Emacs 21, on any tty that has a
> termcap "so" capability, you can get inverse-video simply by using an
> inverse-video face.

That this was written before Emacs 21 doesn't yet make it not useful
now.

> So I don't think ispell-highlight-spelling-error-generic currently
> provides any functionality that isn't more conveniently and
> maintainably provided by ispell-highlight-spelling-error-overlay, with
> the additional benefit that the user can, if he wants, customize
> ispell-highlight-face to something other than inverse-video.

It's a working code whose replacement (basically, a cleanup) will mean
extra work for us, and all that for quite rare situations.  Based on
my long experience with Emacs, it also means some subtle bugs in some
even rarer use cases, which will take years to find and fix.  No,
thanks.

> (Making the default isearch face (and therefore the default
> ispell-highlight-face) be blue-on-magenta on color terminals but
> inverse-video on monochrome terminals is already handled
> automatically.)

You've ignored what I wrote about that possibility: when faces are
customized by users, they are usually customized in simplistic ways,
and are thus unlikely to work for all the cases.  IOW, once you allow
for face customizations, it is very hard to make sure this face will
still be distinct on a colorless terminal.

If you or someone wants to present a patch that will make more
terminals use ispell-highlight-spelling-error-overlay, and includes in
that patch a suitable change to the ispell-highlight-face, then I'll
gladly review it.  Otherwise, I see this issue as closed by that
last-year bugfix.

bug#64075: 28.2; ispell broken on uncolored terminals

[Al Petrofsky]

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

[Resending because I mistakenly used reply rather than reply-all. Sorry]

This ispell code is still broken in emacs-29.0.91.  New recipe:

   emacs-29.0.91 -Q -nw --color=no
   C-x b x RET
   M-x enriched-mode RET
   M-o b stonks M-$ SPC

At this point, "stonks" should have been unchanged by the M-$ SPC, and
therefore should still be bold, but instead only the "s" is bold and
the "tonks" has the default face.  Less importantly, while the ispell
choices were displayed, "stonks" should have been in inverse-video,
but it was not.

The same results are obtained in an xterm version 372 or in a M-x term
inside emacs-29.0.91.

As was the case for the original recipe, changing "(display-color-p)"
to "t" in ispell-highlight-spelling-error fixes the bugs.

On Thu, Jun 15, 2023 at 3:48 AM Eli Zaretskii <eliz@gnu.org> wrote:

> It's a working code whose replacement (basically, a cleanup) will mean
> extra work for us, and all that for quite rare situations.  Based on
> my long experience with Emacs, it also means some subtle bugs in some
> even rarer use cases, which will take years to find and fix.

As you said in bug #56219, it's code that hasn't worked in any release
in the last 15 years (now 16 years).  What I'm saying is it's also
code for a special case that hasn't existed for 22 years (that the
terminal has an "so" capability but the display code can't render
inverse-video faces on it), and that's why it makes more sense to me
to simply eliminate this code and let monochrome terminals be handled
by ispell-highlight-spelling-error-overlay (which is working code),
rather than go in and try to fix the logic in a function that is a
self-labeled "kludge" that depends on details of the low-level
redisplay engine, and hope no new subtle bug has been introduced.

> You've ignored what I wrote about that possibility: when faces are
> customized by users, they are usually customized in simplistic ways,
> and are thus unlikely to work for all the cases.  IOW, once you allow
> for face customizations, it is very hard to make sure this face will
> still be distinct on a colorless terminal.

Fortunately, the face doesn't really need to be distinct, because the
location is also indicated by the cursor (which we all found to be a
sufficient interface for isearch and ispell in Emacs's first decade or
two).  So this is a much lesser concern than the bugs at issue.

But I think I do see what you mean.  Starting with an empty HOME
directory, I started emacs on a color terminal, used M-x customize to
change the colors of the isearch face and save the changes, then
restarted emacs on a monochrome terminal, and now the isearch face is
rendered the same as the default face.

I can easily use the defaults, or easily change the face to suit my
preferences on whatever type of terminal I use most, but to get
support for different types of terminals in a custom manner takes more
work.

Is this situation so bad that it would be preferable for isearch to be
hard-coded to always use inverse video on monochrome displays?  Is
that your position?  I think I prefer it as is, and would prefer that
ispell worked the same way.

I do wonder if it would make sense to make :inverse-video t in all the
defaults for the isearch face, and swap all the default :background
and :foreground properties.  That would result in no change to the
default appearances, with the benefit that after a simple
customization of the colors during an emacs session on a color
terminal, emacs sessions on monochrome terminals would still use
inverse-video for isearch (and ispell).

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

bug#64075: 28.2; ispell broken on uncolored terminals

[Eli Zaretskii]

> From: Al Petrofsky <al@petrofsky.org>
> Date: Thu, 15 Jun 2023 19:19:23 -0400
> Cc: gregory@heytings.org, 64075@debbugs.gnu.org
> 
>    emacs-29.0.91 -Q -nw --color=no
>    C-x b x RET
>    M-x enriched-mode RET
>    M-o b stonks M-$ SPC
> 
> At this point, "stonks" should have been unchanged by the M-$ SPC, and
> therefore should still be bold, but instead only the "s" is bold and
> the "tonks" has the default face.  Less importantly, while the ispell
> choices were displayed, "stonks" should have been in inverse-video,
> but it was not.

Your expectations are wrong: no one said pasting arbitrary text in
Enriched mode over a word should necessarily retain the face
information from the original miss-spelled word.

> As was the case for the original recipe, changing "(display-color-p)"
> to "t" in ispell-highlight-spelling-error fixes the bugs.

Because that uses overlays, so it doesn't interfere with faces.  But
you haven't tested it with any other feature that uses overlays with
face information.  So your testing is not balanced: it is designed to
emphasize the disadvantages of only one method, and disregards the
disadvantages of the other.

> > It's a working code whose replacement (basically, a cleanup) will mean
> > extra work for us, and all that for quite rare situations.  Based on
> > my long experience with Emacs, it also means some subtle bugs in some
> > even rarer use cases, which will take years to find and fix.
> 
> As you said in bug #56219, it's code that hasn't worked in any release
> in the last 15 years (now 16 years).

That's not what I said in that bug.

> What I'm saying is it's also
> code for a special case that hasn't existed for 22 years (that the
> terminal has an "so" capability but the display code can't render
> inverse-video faces on it), and that's why it makes more sense to me
> to simply eliminate this code and let monochrome terminals be handled
> by ispell-highlight-spelling-error-overlay (which is working code),
> rather than go in and try to fix the logic in a function that is a
> self-labeled "kludge" that depends on details of the low-level
> redisplay engine, and hope no new subtle bug has been introduced.

I've considered your proposal and decided against removing this code.
Please don't keep pushing for it, as I won't change my mind just
because you are reiterating the same arguments.

> I can easily use the defaults, or easily change the face to suit my
> preferences on whatever type of terminal I use most, but to get
> support for different types of terminals in a custom manner takes more
> work.

That was exactly my point.  So if we want to make ispell.el work
better in these cases, we need more complex setting of the faces used
in this case, and in particular to depend less on (possibly
customized) other faces.

> Is this situation so bad that it would be preferable for isearch to be
> hard-coded to always use inverse video on monochrome displays?  Is
> that your position?  I think I prefer it as is, and would prefer that
> ispell worked the same way.

My position is as I already explained it.  let me reiterate it:

  . I'm okay with adding more conditions to use the overlay code, by
    testing terminal capabilities other that display-color-p
  . Each such capability should have a distinct face to go with it,
    and that face should use the corresponding display capabilities
    without inheriting from other, more general faces

It is possible that, if we support enough display capabilities in the
above way, the set of terminals that use
ispell-highlight-spelling-error-generic will become much smaller, or
even empty.  But the difference is that we will have a comprehensive
and sound solution based on specific terminal capabilities instead of
a hack that works only in some situations.

> I do wonder if it would make sense to make :inverse-video t in all the
> defaults for the isearch face, and swap all the default :background
> and :foreground properties.  That would result in no change to the
> default appearances, with the benefit that after a simple
> customization of the colors during an emacs session on a color
> terminal, emacs sessions on monochrome terminals would still use
> inverse-video for isearch (and ispell).

Sorry, we will not change the defaults for a face as general and
widely-used as 'isearch', because such a change will cause annoyance
from many users.  Definitely not due to marginal use cases such as
this one.  (You can, of course, make such face customizations
locally.)  This problem should be solved where it happens: in
ispell.el, not by changing a much wider used face.