bug#20543: 24.5; <SPC> in ispell-buffer accepts spelling for the whole line

[Eli Zaretskii <eliz@gnu.org>]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 28 May 2021 02:10:50 +0200
> Cc: Jürgen Hartmann <juergen_hartmann_@hotmail.com>,
>  20543@debbugs.gnu.org
> 
> Marcin Borkowski <mbork@mbork.pl> writes:
> 
> >> line 3651 of emacs-24.5/lisp/textmodes/ispell.el:
> >>
> >>    ;; Do not recheck accepted word on this line.
> 
> I think there's two meanings of "accepted" in that function -- one is
> hitting SPC, and the other is when the word is already in the dictionary.

No, the two meanings of "accepted" here are:

  . the user hits SPC to "leave the word unchanged"
  . the user hits 'a' to "accept the word for this session"

See below.

> >> This suggests that there might be a reason for that behavior. If this is
> >> true, what is it?
> >
> > I've just seen this report.  I'm also very curious about that reason.
> 
> It seems nonsensical to me -- just because you accept the word once on a
> line, it might not be acceptable in the next instance.  So I've changed
> this in Emacs 28.

This is wrong, because now 'a' doesn't work as expected.

Recipe:

  . emacs -Q
  . type into *scratch*:
    foobarical something foobarical something else
  . M-x ispell-buffer
  . a (to "accept" the first "foobarical")
  . observe Emacs stopping on the next "foobarical", instead of
    skipping it, as expected

The problem is that replace == nil means two things: either the user
pressed SPC or the user pressed 'a' (but NOT 'A').  However, the
"don't check the same line" logic should only be applied for 'a', not
for SPC.  So what we need to fix this is IMO adding a way to
distinguish between 'a' and SPC (perhaps by looking at
ispell-buffer-session-localwords?).

Note that the comments in ispell-process-line wrt the meaning of the
value of 'replace' are AFAICT inaccurate:

            ;; Insert correction if needed.
            (cond
             ((equal 0 replace)         ; INSERT
              (if (equal 0 replace)     ; BUFFER-LOCAL DICT ADD
                  (ispell-add-per-file-word-list (car poss)))
              ;; Do not recheck accepted word on this line.
              (setq accept-list (cons (car poss) accept-list)))
             (t
              ;; The user hit SPC, so accept this word, but keep
              ;; checking the rest of the line.
              (unless replace
                (setq accepted t)
                (setq replace (list (buffer-substring-no-properties
                                     (point) (+ word-len (point))))))

(This is also inelegant, as it tests 'replace' for being zero twice.)

Contrary to the comment, as can be seen from ispell-command-loop, the
value of 'replace' can be:

  nil if user pressed 'i' or 'u'
  nil if user pressed 'a'
  nil if user typed SPC
  0 if user pressed 'A'
  replacement word if user typed 'r' or 'R'
  t if the spelling session should end

So the above 'cond' is incorrect and should be fixed.

In any case, the change, as it is, is for the worse, because 'a' is by
far more important than SPC during spell-checking of technical text,
where there are many acronyms and jargon words unknown to the
dictionary.  If we don't have good ideas how to fix the SPC case, we
should revert the change and add a FIXME.