From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#69733: [PATCH] Flyspell (flyspell-word): do not force 'save-excursion' on timers Date: Thu, 14 Mar 2024 12:36:02 +0200 Message-ID: <86ttl99k31.fsf@gnu.org> References: <86ttlbtwmt.fsf@gnu.org> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="1283"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 69733@debbugs.gnu.org To: Daniel Pettersson Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Mar 14 11:37:05 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rkiSO-000Ae0-Fb for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 14 Mar 2024 11:37:04 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rkiS3-00048a-0b; Thu, 14 Mar 2024 06:36:43 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rkiRm-00047s-VY for bug-gnu-emacs@gnu.org; Thu, 14 Mar 2024 06:36:27 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rkiRm-0004Ei-GW for bug-gnu-emacs@gnu.org; Thu, 14 Mar 2024 06:36:26 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rkiSL-0003hc-PL for bug-gnu-emacs@gnu.org; Thu, 14 Mar 2024 06:37:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 14 Mar 2024 10:37:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 69733 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 69733-submit@debbugs.gnu.org id=B69733.171041261414211 (code B ref 69733); Thu, 14 Mar 2024 10:37:01 +0000 Original-Received: (at 69733) by debbugs.gnu.org; 14 Mar 2024 10:36:54 +0000 Original-Received: from localhost ([127.0.0.1]:48411 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rkiSE-0003h6-6H for submit@debbugs.gnu.org; Thu, 14 Mar 2024 06:36:54 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:46326) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rkiSB-0003gr-F8 for 69733@debbugs.gnu.org; Thu, 14 Mar 2024 06:36:52 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rkiRW-0004CB-5N; Thu, 14 Mar 2024 06:36:10 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=ZzGHzXLKjgb+dBGUWqHxoJJxVggkS9Bb3EBlRwnMoeU=; b=AHv9xreyl1kM c2eMycvizZpXdu7dGZZQ4HdJ9BV6jTmnQmLk9h0GFcwHzbOrPzS9DO4oSAxovvweeIBJP7It91WCs urhH+LM3okpQLBjMPEPuS6ZNn+uRU+33spWjpLoV3VYhG+UmQ3lY0zTsoQhEVlPLAwm2DrVzc+ChG fEIUaZRrM4LLPP0S+kp9YJvHfZO4vmHzwD778WQ18N5Q5Mtas0cw9ATzJWoICfS9/qslJPMCCxAxB yIIt3q+wj2DHnRXHAIjJL6NgOwq03lgKBvBslwn1AR+PQZDvNqd7Jj9whAHry889pQWALiMIc4HD6 nnIUdbo6T6tvCYfDJ+NIXw==; In-Reply-To: (message from Daniel Pettersson on Tue, 12 Mar 2024 15:28:22 +0100) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:281606 Archived-At: > From: Daniel Pettersson > Cc: 69733@debbugs.gnu.org > Date: Tue, 12 Mar 2024 15:28:22 +0100 > > Eli Zaretskii writes: > > > Thanks, but I don't think I understand the relation between what you > > say above (and in the comments in your patch below) and the code > > change you propose: > > flyspell-word wraps it's body inside an `save-excursion'. > > (defun flyspell-word (&optional following known-misspelling) > ... > (save-excursion > ... > >> (while (progn > >> - (accept-process-output ispell-process) > >> + ;; don't force save-excursion to timers. > >> + ;; only accept output from ispell-process. > >> + (accept-process-output ispell-process nil nil t) > > > Where's save-excursion and point moving to which you allude here? > > See above and for point moving I think that the stack trace at the > bottom of the email highlights my point. > > > More importantly, why is it a good idea to stop running timers during > > this accept-process-output call and ignore output from other > > subprocesses? > > I don't think that is the optimal solution. But this does not ignore > output right, it just delays timers and filter functions. If flyspell > is active in an comit buffer it might also screw up the point if process > outputs stuff during `accept-process-output'. > > The best solution would be to run `accept-process-output' outside of the > body of save-excursion, but that would require a bit more mangling, > which might be worth the effort. > > Let's create an simple example to illustrate the issue with current > implementation. This is a highly construed example but there is always > the chance that flyspell has unexpected interactions with all buffers > that filter functions and timer functions modify the point of. > > (defun timer-goto-point-min () > (goto-char (point-min))) > > (defun some-command () > (interactive) > ;; flyspell needs point to have been and at word. > (goto-char (point-min)) > (re-search-forward "word.") > ;; Goto top of buffer with timer > (run-with-timer 0 0 'timer-goto-point-min)) > > (trace-function #'timer-goto-point-min) > (trace-function 'flyspell-word) > (trace-function 'accept-process-output) > > ;; Prepare buffer > (with-current-buffer (get-buffer-create "buffer") > (save-excursion > (insert "Some sentence with words.")) > (flyspell-mode)) > > ;; Open buffer M-x some-command expects point to be at the start of > ;; buffer > > ;; 1 -> (flyspell-word) -- start of save-excursion > ;; | 2 -> (accept-process-output #) > ;; | | 3 -> (timer-goto-point-min) -- moves point > ;; | | 3 <- timer-goto-point-min: 1 > ;; | 2 <- accept-process-output: t > ;; 1 <- flyspell-word: nil -- end of save-excursion > ;; resets the point Thanks, but I'm still confused regarding what you are trying to fix and why you are trying to fix it with the patch you proposed. First, AFAIU, save-excursion is there because flyspell-get-word might move point. So this is justified. Next, you seem to be saying that it is for some reason bad to run timers under save-excursion, but you haven't explained why. IMO, if a timer that runs during post-command-hook moves point, that can have negative effect on UX. Also, code that runs from a timer cannot possibly rely on Emacs leaving point where the timer moves it. Are there any examples of timers that run like this which _must_ be able to move point and make sure point stays where the timer moved it?