* bug#64075: 28.2; ispell broken on uncolored terminals @ 2023-06-14 23:13 Al Petrofsky 2023-06-14 23:25 ` Gregory Heytings 0 siblings, 1 reply; 9+ messages in thread From: Al Petrofsky @ 2023-06-14 23:13 UTC (permalink / raw) To: 64075 [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#64075: 28.2; ispell broken on uncolored terminals 2023-06-14 23:13 bug#64075: 28.2; ispell broken on uncolored terminals Al Petrofsky @ 2023-06-14 23:25 ` Gregory Heytings 2023-06-14 23:48 ` Al Petrofsky 2023-06-15 5:23 ` Eli Zaretskii 0 siblings, 2 replies; 9+ messages in thread From: Gregory Heytings @ 2023-06-14 23:25 UTC (permalink / raw) To: Al Petrofsky; +Cc: 64075 [-- 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". ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#64075: 28.2; ispell broken on uncolored terminals 2023-06-14 23:25 ` Gregory Heytings @ 2023-06-14 23:48 ` Al Petrofsky 2023-06-15 5:36 ` Eli Zaretskii 2023-06-15 5:23 ` Eli Zaretskii 1 sibling, 1 reply; 9+ messages in thread From: Al Petrofsky @ 2023-06-14 23:48 UTC (permalink / raw) To: Gregory Heytings; +Cc: 64075 [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#64075: 28.2; ispell broken on uncolored terminals 2023-06-14 23:48 ` Al Petrofsky @ 2023-06-15 5:36 ` Eli Zaretskii 2023-06-15 6:34 ` Al Petrofsky 0 siblings, 1 reply; 9+ messages in thread From: Eli Zaretskii @ 2023-06-15 5:36 UTC (permalink / raw) To: Al Petrofsky; +Cc: 64075, gregory > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#64075: 28.2; ispell broken on uncolored terminals 2023-06-15 5:36 ` Eli Zaretskii @ 2023-06-15 6:34 ` Al Petrofsky 2023-06-15 7:48 ` Eli Zaretskii 0 siblings, 1 reply; 9+ messages in thread From: Al Petrofsky @ 2023-06-15 6:34 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 64075, gregory [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#64075: 28.2; ispell broken on uncolored terminals 2023-06-15 6:34 ` Al Petrofsky @ 2023-06-15 7:48 ` Eli Zaretskii 2023-06-15 23:19 ` Al Petrofsky 0 siblings, 1 reply; 9+ messages in thread From: Eli Zaretskii @ 2023-06-15 7:48 UTC (permalink / raw) To: Al Petrofsky; +Cc: 64075, gregory > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#64075: 28.2; ispell broken on uncolored terminals 2023-06-15 7:48 ` Eli Zaretskii @ 2023-06-15 23:19 ` Al Petrofsky 2023-06-16 6:27 ` Eli Zaretskii 0 siblings, 1 reply; 9+ messages in thread From: Al Petrofsky @ 2023-06-15 23:19 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 64075, gregory [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#64075: 28.2; ispell broken on uncolored terminals 2023-06-15 23:19 ` Al Petrofsky @ 2023-06-16 6:27 ` Eli Zaretskii 0 siblings, 0 replies; 9+ messages in thread From: Eli Zaretskii @ 2023-06-16 6:27 UTC (permalink / raw) To: Al Petrofsky; +Cc: 64075, gregory > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#64075: 28.2; ispell broken on uncolored terminals 2023-06-14 23:25 ` Gregory Heytings 2023-06-14 23:48 ` Al Petrofsky @ 2023-06-15 5:23 ` Eli Zaretskii 1 sibling, 0 replies; 9+ messages in thread From: Eli Zaretskii @ 2023-06-15 5:23 UTC (permalink / raw) To: Gregory Heytings; +Cc: al, 64075 > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-06-16 6:27 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-14 23:13 bug#64075: 28.2; ispell broken on uncolored terminals Al Petrofsky 2023-06-14 23:25 ` Gregory Heytings 2023-06-14 23:48 ` Al Petrofsky 2023-06-15 5:36 ` Eli Zaretskii 2023-06-15 6:34 ` Al Petrofsky 2023-06-15 7:48 ` Eli Zaretskii 2023-06-15 23:19 ` Al Petrofsky 2023-06-16 6:27 ` Eli Zaretskii 2023-06-15 5:23 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).