bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Tomasz Hołubowicz via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hello,

This is my configuration for completion-preview:

(use-package completion-preview
  :hook (after-init . global-completion-preview-mode)
  :bind (:map completion-preview-active-mode-map
              ("M-n" . completion-preview-next-candidate)
              ("M-p" . completion-preview-prev-candidate))
  :custom (completion-preview-idle-delay 0.2)
  :config
  ;; Org mode has a custom `self-insert-command'
  (push 'org-self-insert-command completion-preview-commands)

  ;; Hide `completion-preview' overlay when invoking `completion-in-region'
  (with-eval-after-load 'consult
    (advice-add 'consult-completion-in-region :before (lambda (&rest _) (completion-preview-hide)))))

When I enter org-mode file and flyspell-mode is toggled on then the
completion-preview-idle-delay is much longer in reality that 0.2s. After
I disable flyspell-mode, then the delay is truly 0.2s. It
seems that completion-preview is being delayed by flyspell checking
currently written word. It makes sense if we consider 'idle' literally,
but I'm not sure if this is desired.

In GNU Emacs 30.0.92 (build 1, x86_64-redhat-linux-gnu, GTK+ Version
 3.24.43, cairo version 1.18.0) of 2024-11-19 built on
 f37c2cd00c6a4c8fbc0fa73e20903c45
System Description: Fedora Linux 41 (Toolbx Container Image)

Configured using:
 'configure --build=x86_64-redhat-linux --host=x86_64-redhat-linux
 --program-prefix= --disable-dependency-tracking --prefix=/usr
 --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin
 --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include
 --libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var
 --runstatedir=/run --sharedstatedir=/var/lib --mandir=/usr/share/man
 --infodir=/usr/share/info --with-cairo --with-dbus --with-gif
 --with-gpm=no --with-harfbuzz --with-jpeg --with-modules
 --with-native-compilation=aot --with-pgtk --with-png --with-rsvg
 --with-sqlite3 --with-tiff --with-tree-sitter --with-webp --with-xpm
 build_alias=x86_64-redhat-linux host_alias=x86_64-redhat-linux CC=gcc
 'CFLAGS=-DMAIL_USE_LOCKF -O2 -flto=auto -ffat-lto-objects -fexceptions
 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security
 -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS
 -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong
 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=x86-64
 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection
 -fcf-protection -mtls-dialect=gnu2 -fno-omit-frame-pointer
 -mno-omit-leaf-frame-pointer ' 'LDFLAGS=-Wl,-z,relro -Wl,--as-needed
 -Wl,-z,pack-relative-relocs -Wl,-z,now
 -specs=/usr/lib/rpm/redhat/redhat-hardened-ld
 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -Wl,--build-id=sha1
 -specs=/usr/lib/rpm/redhat/redhat-package-notes ' CXX=g++ 'CXXFLAGS=-O2
 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches
 -pipe -Wall -Werror=format-security
 -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS
 -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong
 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=x86-64
 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection
 -fcf-protection -mtls-dialect=gnu2 -fno-omit-frame-pointer
 -mno-omit-leaf-frame-pointer '
 PKG_CONFIG_PATH=:/usr/lib64/pkgconfig:/usr/share/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG
LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY
PDUMPER PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Org

Minor modes in effect:
  denote-menu-bar-mode: t
  flyspell-mode: t
  rcirc-track-minor-mode: t
  server-mode: t
  recentf-mode: t
  savehist-mode: t
  save-place-mode: t
  repeat-mode: t
  global-subword-mode: t
  subword-mode: t
  delete-selection-mode: t
  electric-pair-mode: t
  pixel-scroll-precision-mode: t
  global-auto-revert-mode: t
  vertico-mode: t
  marginalia-mode: t
  global-completion-preview-mode: t
  completion-preview-mode: t
  diff-hl-margin-local-mode: t
  diff-hl-margin-mode: t
  global-diff-hl-mode: t
  diff-hl-mode: t
  global-treesit-auto-mode: t
  minibuffer-electric-default-mode: t
  minibuffer-depth-indicate-mode: t
  popper-echo-mode: t
  popper-mode: t
  windmove-mode: t
  override-global-mode: t
  apheleia-global-mode: t
  apheleia-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tab-bar-history-mode: t
  tab-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  minibuffer-regexp-mode: t
  line-number-mode: t
  visual-line-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/var/home/alternateved/.config/emacs/elpa/transient-20241115.2034/transient hides /usr/share/emacs/30.0.92/lisp/transient

Features:
(shadow sort mail-extr misearch multi-isearch dired-aux hl-line denote
xref emacsbug cl-print vertico-directory completion help-fns radix-tree
puni pulse color mule-util orderless dabbrev tempel apheleia
apheleia-rcs apheleia-dp apheleia-formatters apheleia-utils apheleia-log
apheleia-formatter-context embark-org embark-consult embark ffap vc-git
cape oc-basic org-element org-persist org-id org-refile org-element-ast
inline avl-tree generator ol-eww eww xdg url-queue mm-url ol-rmail
ol-mhe ol-irc ol-info ol-gnus nnselect gnus-art mm-uu mml2015 mm-view
mml-smime smime dig gnus-sum shr pixel-fill kinsoku url-file svg dom
gnus-group gnus-undo gnus-start gnus-dbus dbus xml gnus-cloud nnimap
nnmail mail-source utf7 nnoo gnus-spec gnus-int gnus-range message
sendmail yank-media rfc822 mml mml-sec mm-decode mm-bodies mm-encode
mail-parse rfc2231 rfc2047 rfc2045 ietf-drums mailabbrev gmm-utils
mailheader gnus-win gnus nnheader gnus-util mail-utils range mm-util
mail-prsvr ol-docview doc-view jka-compr image-mode exif ol-bibtex
bibtex ol-bbdb ol-w3m ol-doi org-link-doi org ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-macro org-src sh-script smie executable ob-comint
org-pcomplete pcomplete org-list org-footnote org-faces org-entities
org-version ob-emacs-lisp ob-core ob-eval org-cycle org-table ol
org-fold org-fold-core org-keys oc org-loaddefs find-func cal-menu
calendar cal-loaddefs org-compat org-macs format-spec comp comp-cstr
comp-run comp-common rx cursor-sensor vertico-repeat consult bookmark
flyspell ispell gnutls network-stream puny nsm rcirc epa-file epa
derived epg rfc6068 epg-config parse-time iso8601 cus-start server
noutline outline time-date checkdoc lisp-mnt flymake project compile
text-property-search comint ansi-osc ansi-color warnings thingatpt
recentf tree-widget savehist saveplace repeat cap-words superword
subword delsel elec-pair pixel-scroll cua-base ring autorevert
filenotify vertico marginalia compat completion-preview
exec-path-from-shell diff-hl-margin diff-hl-dired dired dired-loaddefs
diff-hl log-view pcvs-util vc-dir ewoc vc vc-dispatcher diff-mode
track-changes flymake-collection-hook treesit-auto treesit minibuf-eldef
mb-depth popper-echo popper windmove modus-operandi-theme modus-themes
disp-table advice edmacro kmacro use-package use-package-ensure
use-package-delight use-package-diminish use-package-bind-key bind-key
cl-extra help-mode use-package-core finder-inf ajrepl-autoloads
apheleia-autoloads avy-autoloads cape-autoloads consult-dir-autoloads
corfu-terminal-autoloads corfu-autoloads denote-autoloads
diff-hl-autoloads dts-mode-autoloads eat-autoloads
edit-indirect-autoloads elfeed-autoloads embark-consult-autoloads
consult-autoloads embark-autoloads exec-path-from-shell-autoloads
flymake-collection-autoloads geiser-guile-autoloads geiser-autoloads
gptel-autoloads haskell-mode-autoloads janet-ts-mode-autoloads
ledger-mode-autoloads lingva-autoloads magit-autoloads pcase
magit-section-autoloads marginalia-autoloads markdown-mode-autoloads
olivetti-autoloads orderless-autoloads pcmpl-args-autoloads
popon-autoloads popper-autoloads puni-autoloads easy-mmode
purescript-mode-autoloads rainbow-mode-autoloads sly-autoloads
smartparens-autoloads dash-autoloads tempel-autoloads
transient-autoloads treesit-auto-autoloads vertico-autoloads
vundo-autoloads web-mode-autoloads wgrep-autoloads info
with-editor-autoloads xclip-autoloads package browse-url url url-proxy
url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util mailcap url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs password-cache json subr-x
map byte-opt bytecomp byte-compile url-vars gv cus-edit pp cus-load
icons wid-edit cl-loaddefs cl-lib rmc iso-transl tooltip cconv eldoc
paren electric uniquify ediff-hook vc-hooks lisp-float-type elisp-mode
mwheel term/pgtk-win pgtk-win term/common-win touch-screen pgtk-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer nadvice seq
simple cl-generic indonesian philippine cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button
loaddefs theme-loaddefs faces cus-face macroexp files window
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget keymap hashtable-print-readable backquote threads dbusbind
inotify dynamic-setting system-font-setting font-render-setting cairo
gtk pgtk multi-tty move-toolbar make-network-process native-compile
emacs)

Memory information:
((conses 16 736317 667463) (symbols 48 41370 4) (strings 32 204409 35035)
 (string-bytes 1 6531566) (vectors 16 68015) (vector-slots 8 1503632 408280)
 (floats 8 602 7069) (intervals 56 3248 2681) (buffers 992 28))

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi there,

Tomasz Hołubowicz writes:

> Hello,
>
> This is my configuration for completion-preview:
>
> (use-package completion-preview
>   :hook (after-init . global-completion-preview-mode)
>   :bind (:map completion-preview-active-mode-map
>               ("M-n" . completion-preview-next-candidate)
>               ("M-p" . completion-preview-prev-candidate))
>   :custom (completion-preview-idle-delay 0.2)
>   :config
>   ;; Org mode has a custom `self-insert-command'
>   (push 'org-self-insert-command completion-preview-commands)
>
>   ;; Hide `completion-preview' overlay when invoking `completion-in-region'
>   (with-eval-after-load 'consult
>     (advice-add 'consult-completion-in-region :before (lambda (&rest _) (completion-preview-hide)))))

Side note: it is better to use (completion-preview-active-mode -1)
instead of (completion-preview-hide) to dismiss the preview, since
disabling the minor mode performs additional cleanup.  Perhaps I should
mention that in the documentation somewhere... :)

> When I enter org-mode file and flyspell-mode is toggled on then the
> completion-preview-idle-delay is much longer in reality that 0.2s. After
> I disable flyspell-mode, then the delay is truly 0.2s. It
> seems that completion-preview is being delayed by flyspell checking
> currently written word. It makes sense if we consider 'idle' literally,
> but I'm not sure if this is desired.

Thank you for this report.  I can reproduce the extended delay here.
(In the future, please provide a minimal recipe to reproduce the issue
starting from emacs -Q, if possible.)

The root cause is a peculiarity of flyspell-mode: it calls sit-for and
blocks Emacs for 3 whole seconds (by default, see flyspell-delay) after
certain commands, including after (org-)self-insert-command.  This also
blocks the idle timer that Completion Preview mode uses, unfortunately.

A quick search shows that this behavior of flyspell affects other
features as well.  For example, IIUC, auto completion in Corfu switched
to using run-at-time instead of run-with-idle-timer due to this issue.

I think flyspell should be modified to use a timer instead of sit-for,
so as to avoid blocking idle timers.  I can come up with such a patch,
but it's not quite trivial, so I wonder what others think about this
issue and how it should be addressed.

As a stopgap, you can try setting flyspell-delay to 0.


Best,

Eshel

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Tomasz Hołubowicz via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Hello,

On Tue, Nov 19 2024, Eshel Yaron wrote:

> Side note: it is better to use (completion-preview-active-mode -1)
> instead of (completion-preview-hide) to dismiss the preview, since
> disabling the minor mode performs additional cleanup.  Perhaps I should
> mention that in the documentation somewhere... :)
>
Thank you for that tip!

> Thank you for this report.  I can reproduce the extended delay here.
> (In the future, please provide a minimal recipe to reproduce the issue
> starting from emacs -Q, if possible.)

Apologies for a bit noisy report, I wrote it in a bit of a rush.

If anyone would like to reproduce this issue, I believe below recipe
should be helpful:

(require 'flyspell)
(add-hook 'org-mode-hook #'flyspell-mode)

(require 'completion-preview)
(setopt completion-preview-idle-delay 0.2)
(push 'org-self-insert-command completion-preview-commands)

(global-completion-preview-mode 1)

After that open an Org file and try to trigger completion-preview by
writing a part of a word. Completion results most likely depend on the
contents of file pointed by ispell-alternate-dictionary value. If that
file is empty there might be no completions available.

> The root cause is a peculiarity of flyspell-mode: it calls sit-for and
> blocks Emacs for 3 whole seconds (by default, see flyspell-delay) after
> certain commands, including after (org-)self-insert-command.  This also
> blocks the idle timer that Completion Preview mode uses, unfortunately.
>
> A quick search shows that this behavior of flyspell affects other
> features as well.  For example, IIUC, auto completion in Corfu switched
> to using run-at-time instead of run-with-idle-timer due to this issue.
>
> I think flyspell should be modified to use a timer instead of sit-for,
> so as to avoid blocking idle timers.  I can come up with such a patch,
> but it's not quite trivial, so I wonder what others think about this
> issue and how it should be addressed.
>
> As a stopgap, you can try setting flyspell-delay to 0.

Thank you for help and explanation. I will try flyspell-delay 0 for a
time (or stick with completion-preview-mode only for prog-mode buffers
at it seems more appropriate).

Kind wishes,
Tomasz Hołubowicz


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



[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Eli Zaretskii]

> Cc: mail@alternateved.com
> Date: Tue, 19 Nov 2024 17:13:16 +0100
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> The root cause is a peculiarity of flyspell-mode: it calls sit-for and
> blocks Emacs for 3 whole seconds (by default, see flyspell-delay) after
> certain commands, including after (org-)self-insert-command.  This also
> blocks the idle timer that Completion Preview mode uses, unfortunately.
> 
> A quick search shows that this behavior of flyspell affects other
> features as well.  For example, IIUC, auto completion in Corfu switched
> to using run-at-time instead of run-with-idle-timer due to this issue.
> 
> I think flyspell should be modified to use a timer instead of sit-for,
> so as to avoid blocking idle timers.  I can come up with such a patch,
> but it's not quite trivial, so I wonder what others think about this
> issue and how it should be addressed.

This is likely to cause differences in behavior that some users will
be unhappy about (how do you support delaying only after some
commands? and how do you interact with async subprocess from an idle
timer?).  So if you want to install such a patch, it must be an opt-in
feature, so we could let users try it for some time before we decide
whether to retire the old behavior based on sit-for.

> As a stopgap, you can try setting flyspell-delay to 0.

That will make flyspell-mode get in the way of fast typing, AFAIU, but
maybe the OP doesn't care about that.

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: mail@alternateved.com
>> Date: Tue, 19 Nov 2024 17:13:16 +0100
>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> The root cause is a peculiarity of flyspell-mode: it calls sit-for and
>> blocks Emacs for 3 whole seconds (by default, see flyspell-delay) after
>> certain commands, including after (org-)self-insert-command.  This also
>> blocks the idle timer that Completion Preview mode uses, unfortunately.
>> 
>> A quick search shows that this behavior of flyspell affects other
>> features as well.  For example, IIUC, auto completion in Corfu switched
>> to using run-at-time instead of run-with-idle-timer due to this issue.
>> 
>> I think flyspell should be modified to use a timer instead of sit-for,
>> so as to avoid blocking idle timers.  I can come up with such a patch,
>> but it's not quite trivial, so I wonder what others think about this
>> issue and how it should be addressed.
>
> This is likely to cause differences in behavior that some users will
> be unhappy about (how do you support delaying only after some
> commands? and how do you interact with async subprocess from an idle
> timer?).  So if you want to install such a patch, it must be an opt-in
> feature, so we could let users try it for some time before we decide
> whether to retire the old behavior based on sit-for.

All right, something like this?

diff --git a/lisp/textmodes/flyspell.el b/lisp/textmodes/flyspell.el
index 7bf9cb1ae9d..7ba5bb871ea 100644
--- a/lisp/textmodes/flyspell.el
+++ b/lisp/textmodes/flyspell.el
@@ -810,6 +810,13 @@ flyspell-check-changed-word-p
              (let ((pos (point)))
                (or (>= pos start) (<= pos stop) (= pos (1+ stop))))))))
 
+(defcustom flyspell-blocking-delay t
+  "Whether to block Emacs while Flyspell waits after a delayed command."
+  :type 'boolean
+  :version "30.1")
+
+(defvar flyspell--timer nil)
+
 ;;*---------------------------------------------------------------------*/
 ;;*    flyspell-check-word-p ...                                        */
 ;;*---------------------------------------------------------------------*/
@@ -844,7 +851,15 @@ flyspell-check-word-p
 	;; The current command is not delayed, that
 	;; is that we must check the word now.
 	(and (not unread-command-events)
-	     (sit-for flyspell-delay)))
+             (if flyspell-blocking-delay
+                 (sit-for flyspell-delay)
+               (setq flyspell--timer
+                     (run-with-idle-timer
+                      flyspell-delay nil
+                      (lambda (buffer)
+                        (when (eq (current-buffer) buffer) (flyspell-word)))
+                      (current-buffer)))
+               nil)))
        (t t)))
      (t t))))
 
@@ -955,6 +970,7 @@ flyspell-debug-signal-changed-checked
 (defun flyspell-post-command-hook ()
   "The `post-command-hook' used by flyspell to check a word on-the-fly."
   (interactive)
+  (when (timerp flyspell--timer) (cl-callf cancel-timer flyspell--timer))
   (when flyspell-mode
     (with-local-quit
       (let ((command this-command)
@@ -1179,7 +1195,7 @@ flyspell-word
 		  (set-process-query-on-exit-flag ispell-process nil)
                   ;; Wait until ispell has processed word.
                   (while (progn
-                           (accept-process-output ispell-process)
+                           (accept-process-output ispell-process 1)
                            (not (string= "" (car ispell-filter)))))
                   ;; (ispell-send-string "!\n")
                   ;; back to terse mode.

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 74437@debbugs.gnu.org,  mail@alternateved.com
> Date: Tue, 19 Nov 2024 19:40:07 +0100
> 
> +(defcustom flyspell-blocking-delay t

I'd name this flyspell-use-timer or somesuch.

> +  "Whether to block Emacs while Flyspell waits after a delayed command."
> +  :type 'boolean
> +  :version "30.1")
     ^^^^^^^^^^^^^^
You are an optimist ;-)


> -                           (accept-process-output ispell-process)
> +                           (accept-process-output ispell-process 1)

Does this really work reliably from a timer?  You _are_ aware that
this call to accept-process-output will run timers while we wait?
Also, what happens if there are other async subprocesses running in
parallel, like maybe Grep or compilation or url-retrieve?

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: 74437@debbugs.gnu.org,  mail@alternateved.com
>> Date: Tue, 19 Nov 2024 19:40:07 +0100
>> 
>> +(defcustom flyspell-blocking-delay t
>
> I'd name this flyspell-use-timer or somesuch.

OK, will do.  Actually, how about flyspell-delay-use-timer?  Just
flyspell-use-timer feels a bit too general: Flyspell might use timers
for other purposes regardless of this option.

>> +  "Whether to block Emacs while Flyspell waits after a delayed command."
>> +  :type 'boolean
>> +  :version "30.1")
>      ^^^^^^^^^^^^^^
> You are an optimist ;-)

Oh well, 31.1 it is then :)

>> -                           (accept-process-output ispell-process)
>> +                           (accept-process-output ispell-process 1)
>
> Does this really work reliably from a timer?

I don't immediately see why it shouldn't, and the tests I tried so far
seem to work, but your skepticism makes me wonder if there's anything
I'm missing.  Do you have any particular potential issue in mind?

> You _are_ aware that this call to accept-process-output will run
> timers while we wait?

Yes.  If that's a cause for concern, we can inhibit running timers here
when we're calling flyspell-word from a timer, like in the patch below.

> Also, what happens if there are other async
> subprocesses running in parallel, like maybe Grep or compilation or
> url-retrieve?

They make progress, which seems to work as expected, at least with Grep.
That is if we use the previous patch, with the one below we pass non-nil
JUST-THIS-ONE argument to accept-process-output when called from a timer
so other processes shouldn't see new output during this call.  Either
way works, AFAICT.


diff --git a/lisp/textmodes/flyspell.el b/lisp/textmodes/flyspell.el
index 7bf9cb1ae9d..5442b46cd1d 100644
--- a/lisp/textmodes/flyspell.el
+++ b/lisp/textmodes/flyspell.el
@@ -810,6 +810,13 @@ flyspell-check-changed-word-p
              (let ((pos (point)))
                (or (>= pos start) (<= pos stop) (= pos (1+ stop))))))))
 
+(defcustom flyspell-delay-use-timer nil
+  "Whether to use a timer for waiting after a delayed command."
+  :type 'boolean
+  :version "31.1")
+
+(defvar flyspell--timer nil)
+
 ;;*---------------------------------------------------------------------*/
 ;;*    flyspell-check-word-p ...                                        */
 ;;*---------------------------------------------------------------------*/
@@ -844,7 +851,16 @@ flyspell-check-word-p
 	;; The current command is not delayed, that
 	;; is that we must check the word now.
 	(and (not unread-command-events)
-	     (sit-for flyspell-delay)))
+             (if (not flyspell-delay-use-timer)
+                 (sit-for flyspell-delay)
+               (setq flyspell--timer
+                     (run-with-idle-timer
+                      flyspell-delay nil
+                      (lambda (buffer)
+                        (when (eq (current-buffer) buffer)
+                          (flyspell-word nil nil t)))
+                      (current-buffer)))
+               nil)))
        (t t)))
      (t t))))
 
@@ -955,6 +971,7 @@ flyspell-debug-signal-changed-checked
 (defun flyspell-post-command-hook ()
   "The `post-command-hook' used by flyspell to check a word on-the-fly."
   (interactive)
+  (when (timerp flyspell--timer) (cl-callf cancel-timer flyspell--timer))
   (when flyspell-mode
     (with-local-quit
       (let ((command this-command)
@@ -1095,13 +1112,14 @@ flyspell-word
 ;;*---------------------------------------------------------------------*/
 ;;*    flyspell-word ...                                                */
 ;;*---------------------------------------------------------------------*/
-(defun flyspell-word (&optional following known-misspelling)
+(defun flyspell-word (&optional following known-misspelling block-timers)
   "Spell check a word.
 If the optional argument FOLLOWING, or, when called interactively
 `ispell-following-word', is non-nil, checks the following (rather
 than preceding) word when the cursor is not over a word.  If
 optional argument KNOWN-MISSPELLING is non-nil considers word a
-misspelling and skips redundant spell-checking step.
+misspelling and skips redundant spell-checking step.  Non-nil optional
+argument BLOCK-TIMERS says not to run timers while spell-checking.
 
 See `flyspell-get-word' for details of how this finds the word to
 spell-check."
@@ -1179,7 +1197,8 @@ flyspell-word
 		  (set-process-query-on-exit-flag ispell-process nil)
                   ;; Wait until ispell has processed word.
                   (while (progn
-                           (accept-process-output ispell-process)
+                           (accept-process-output ispell-process 1 nil
+                                                  (when block-timers 0))
                            (not (string= "" (car ispell-filter)))))
                   ;; (ispell-send-string "!\n")
                   ;; back to terse mode.

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 74437@debbugs.gnu.org,  mail@alternateved.com
> Date: Tue, 19 Nov 2024 21:16:56 +0100
> 
> >> -                           (accept-process-output ispell-process)
> >> +                           (accept-process-output ispell-process 1)
> >
> > Does this really work reliably from a timer?
> 
> I don't immediately see why it shouldn't, and the tests I tried so far
> seem to work, but your skepticism makes me wonder if there's anything
> I'm missing.  Do you have any particular potential issue in mind?

Not concretely, no.  But I see potential issues, since
accept-process-output enters a recursive wait_reading_process_output,
which could read output from other subprocesses, and timers affect how
frequently wait_reading_process_output loops.  So this should be
thoroughly tested in various scenarios.

> > You _are_ aware that this call to accept-process-output will run
> > timers while we wait?
> 
> Yes.  If that's a cause for concern, we can inhibit running timers here
> when we're calling flyspell-word from a timer, like in the patch below.

That's one measure, yes.  But it, too, should be thoroughly tested.

> > Also, what happens if there are other async
> > subprocesses running in parallel, like maybe Grep or compilation or
> > url-retrieve?
> 
> They make progress, which seems to work as expected, at least with Grep.
> That is if we use the previous patch, with the one below we pass non-nil
> JUST-THIS-ONE argument to accept-process-output when called from a timer
> so other processes shouldn't see new output during this call.  Either
> way works, AFAICT.

The question is: what do users expect to happen in those cases?

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: 74437@debbugs.gnu.org,  mail@alternateved.com
>> Date: Tue, 19 Nov 2024 21:16:56 +0100
>
>> > Also, what happens if there are other async
>> > subprocesses running in parallel, like maybe Grep or compilation or
>> > url-retrieve?
>> 
>> They make progress, which seems to work as expected, at least with Grep.
>> That is if we use the previous patch, with the one below we pass non-nil
>> JUST-THIS-ONE argument to accept-process-output when called from a timer
>> so other processes shouldn't see new output during this call.  Either
>> way works, AFAICT.
>
> The question is: what do users expect to happen in those cases?

I don't know, IIUC it is just a short period (spell-checking one word)
in which other processes either do or don't see output, so I don't think
it matters that much to users either way.  It seems more natural to let
other processes make progress, because why not?  But again, either way
works for me, since both let us reconcile Flyspell with idle timers.


Best,

Eshel

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 74437@debbugs.gnu.org,  mail@alternateved.com
> Date: Wed, 20 Nov 2024 16:52:49 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Eshel Yaron <me@eshelyaron.com>
> >> Cc: 74437@debbugs.gnu.org,  mail@alternateved.com
> >> Date: Tue, 19 Nov 2024 21:16:56 +0100
> >
> >> > Also, what happens if there are other async
> >> > subprocesses running in parallel, like maybe Grep or compilation or
> >> > url-retrieve?
> >> 
> >> They make progress, which seems to work as expected, at least with Grep.
> >> That is if we use the previous patch, with the one below we pass non-nil
> >> JUST-THIS-ONE argument to accept-process-output when called from a timer
> >> so other processes shouldn't see new output during this call.  Either
> >> way works, AFAICT.
> >
> > The question is: what do users expect to happen in those cases?
> 
> I don't know

Neither do I.  I didn't say something was wrong with either of these
implementations, I'm just saying they should be well tested by users
before we have enough basis to make the decisions whether the idea is
generally good and whether it should probably become the default in
some future version.

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: 74437@debbugs.gnu.org,  mail@alternateved.com
>> Date: Wed, 20 Nov 2024 16:52:49 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Eshel Yaron <me@eshelyaron.com>
>> >> Cc: 74437@debbugs.gnu.org,  mail@alternateved.com
>> >> Date: Tue, 19 Nov 2024 21:16:56 +0100
>> >
>> >> > Also, what happens if there are other async
>> >> > subprocesses running in parallel, like maybe Grep or compilation or
>> >> > url-retrieve?
>> >> 
>> >> They make progress, which seems to work as expected, at least with Grep.
>> >> That is if we use the previous patch, with the one below we pass non-nil
>> >> JUST-THIS-ONE argument to accept-process-output when called from a timer
>> >> so other processes shouldn't see new output during this call.  Either
>> >> way works, AFAICT.
>> >
>> > The question is: what do users expect to happen in those cases?
>> 
>> I don't know
>
> Neither do I.  I didn't say something was wrong with either of these
> implementations, I'm just saying they should be well tested by users
> before we have enough basis to make the decisions whether the idea is
> generally good and whether it should probably become the default in
> some future version.

Sounds good.  So here's a full patch that keeps the current
implementation as the default:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-New-option-flyspell-delay-use-timer.patch --]
[-- Type: text/x-patch, Size: 3105 bytes --]

From 0dfa6f8b4aaa825a2587fa36f30bf9f55918ff3d Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Fri, 22 Nov 2024 08:17:25 +0100
Subject: [PATCH] New option 'flyspell-delay-use-timer'

* lisp/textmodes/flyspell.el (flyspell-delay-use-timer):
New user option.
(flyspell--timer): New variable.
(flyspell-check-word-p): Use them.
(flyspell-post-command-hook): Disable timer.
(flyspell-word): Pass non-nil SECONDS argument to
'accept-process-output' to permit quitting.  (bug#74437)
---
 lisp/textmodes/flyspell.el | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/lisp/textmodes/flyspell.el b/lisp/textmodes/flyspell.el
index 09d4e8a8d1a..5fcb83efb57 100644
--- a/lisp/textmodes/flyspell.el
+++ b/lisp/textmodes/flyspell.el
@@ -810,6 +810,18 @@ flyspell-check-changed-word-p
              (let ((pos (point)))
                (or (>= pos start) (<= pos stop) (= pos (1+ stop))))))))
 
+(defcustom flyspell-delay-use-timer nil
+  "Whether Flyspell should use a timer for waiting after a delayed command.
+
+If this is non-nil, Flyspell sets up a timer for checking the word at
+point `flyspell-delay' seconds after you invoke a delayed command.
+Otherwise, if this option is nil, Flyspell uses `sit-for' to wait for
+that duration instead."
+  :type 'boolean
+  :version "31.1")
+
+(defvar flyspell--timer nil)
+
 ;;*---------------------------------------------------------------------*/
 ;;*    flyspell-check-word-p ...                                        */
 ;;*---------------------------------------------------------------------*/
@@ -844,7 +856,16 @@ flyspell-check-word-p
 	;; The current command is not delayed, that
 	;; is that we must check the word now.
 	(and (not unread-command-events)
-	     (sit-for flyspell-delay)))
+             (if (not flyspell-delay-use-timer)
+                 (sit-for flyspell-delay)
+               (setq flyspell--timer
+                     (run-with-idle-timer
+                      flyspell-delay nil
+                      (lambda (buffer)
+                        (when (eq (current-buffer) buffer)
+                          (flyspell-word nil nil t)))
+                      (current-buffer)))
+               nil)))
        (t t)))
      (t t))))
 
@@ -955,6 +976,7 @@ flyspell-debug-signal-changed-checked
 (defun flyspell-post-command-hook ()
   "The `post-command-hook' used by flyspell to check a word on-the-fly."
   (interactive)
+  (when (timerp flyspell--timer) (cl-callf cancel-timer flyspell--timer))
   (when flyspell-mode
     (with-local-quit
       (let ((command this-command)
@@ -1179,7 +1201,7 @@ flyspell-word
 		  (set-process-query-on-exit-flag ispell-process nil)
                   ;; Wait until ispell has processed word.
                   (while (progn
-                           (accept-process-output ispell-process)
+                           (accept-process-output ispell-process 1)
                            (not (string= "" (car ispell-filter)))))
                   ;; (ispell-send-string "!\n")
                   ;; back to terse mode.
-- 
2.46.2

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 74437@debbugs.gnu.org,  mail@alternateved.com
> Date: Fri, 22 Nov 2024 08:26:42 +0100
> 
> >> > The question is: what do users expect to happen in those cases?
> >> 
> >> I don't know
> >
> > Neither do I.  I didn't say something was wrong with either of these
> > implementations, I'm just saying they should be well tested by users
> > before we have enough basis to make the decisions whether the idea is
> > generally good and whether it should probably become the default in
> > some future version.
> 
> Sounds good.  So here's a full patch that keeps the current
> implementation as the default:

Thanks.  Please install on master, but please add a NEWS entry which
calls out this change and invites feedback on using this new behavior.

bug#74437: 30.0.92; completion-preview-idle-delay is delayed by flyspell

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: 74437@debbugs.gnu.org,  mail@alternateved.com
>> Date: Fri, 22 Nov 2024 08:26:42 +0100
>> 
>> >> > The question is: what do users expect to happen in those cases?
>> >> 
>> >> I don't know
>> >
>> > Neither do I.  I didn't say something was wrong with either of these
>> > implementations, I'm just saying they should be well tested by users
>> > before we have enough basis to make the decisions whether the idea is
>> > generally good and whether it should probably become the default in
>> > some future version.
>> 
>> Sounds good.  So here's a full patch that keeps the current
>> implementation as the default:
>
> Thanks.  Please install on master, but please add a NEWS entry which
> calls out this change and invites feedback on using this new behavior.

Done, in 097b685aa1c.

Tomasz, setting this new option flyspell-delay-use-timer to non-nil
(after building Emacs from the master branch, or applying these changes
to your copy of flyspell.el) should hopefully resolve the extended delay
you've been seeing.


Eshel