bug#54450: 29.0.50; redisplay--pre-redisplay-functions issue

bug#54450: 29.0.50; redisplay--pre-redisplay-functions issue

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

Hi:

As discussed in emacs develop, it seems like the display engine is not
fully running redisplay--pre-redisplay-functions when moving point in
not active windows.

To reproduce:

emacs -Q
(setq highlight-nonselected-windows t)

C-x 2
C-x o
C-x C-f /tmp/test
(insert some text)
C-a C-SPC C-f C-f C-f
C-x o 

[at this point we should see a region highlighted over the 3 first
letters of the inserted text in the other winfow buffer (test)]

then eval this several times:

(with-selected-window (get-buffer-window "test" 0)
    (right-char))

This will move the point in the test window, but the region won't be
updated.




In GNU Emacs 29.0.50 (build 129, x86_64-pc-linux-gnu, GTK+ Version 3.24.33, cairo version 1.17.4)
 of 2022-03-18 built on Ergus
Repository revision: 04350dc2f56b405bc9d1c7046ad5523b3ff23591
Repository branch: feature/completions-customs
System Description: Arch Linux

Configured using:
 'configure --prefix=/home/ergo/.local/ --with-mailutils --with-json
 --with-x-toolkit=gtk3 --with-xft --with-wide-int --with-modules
 --with-cairo --with-harfbuzz --with-native-compilation --with-pgtk'

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

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

Major mode: Lisp Interaction

Minor modes in effect:
  windmove-mode: t
  global-auto-revert-mode: t
  xclip-mode: t
  electric-pair-mode: t
  flyspell-mode: t
  company-mode: t
  flycheck-mode: t
  diff-hl-margin-mode: t
  vertico-mouse-mode: t
  vertico-mode: t
  composable-mark-mode: t
  composable-mode: t
  repeat-mode: t
  xterm-mouse-mode: t
  my/consult-mode: t
  minibuffer-depth-indicate-mode: t
  winner-mode: t
  save-place-mode: t
  delete-selection-mode: t
  savehist-mode: t
  global-display-fill-column-indicator-mode: t
  display-fill-column-indicator-mode: t
  global-display-line-numbers-mode: t
  display-line-numbers-mode: t
  which-key-mode: t
  override-global-mode: t
  eldoc-mode: t
  show-paren-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  context-menu-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  size-indication-mode: t
  column-number-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
~/gits/emacs_clones/composable/composable-mark hides /home/ergo/.config/emacs/elpa/composable-20201024.1458/composable-mark
~/gits/emacs_clones/composable/composable hides /home/ergo/.config/emacs/elpa/composable-20201024.1458/composable
/home/ergo/.config/emacs/elpa/transient-20220314.1605/transient hides /home/ergo/.local/share/emacs/29.0.50/lisp/transient

Features:
(shadow sort mail-extr windmove misearch multi-isearch jka-compr
emacsbug message mailcap yank-media rmc puny rfc822 mml mml-sec epa
derived epg rfc6068 epg-config gnus-util time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
comp comp-cstr warnings rfc2047 rfc2045 ietf-drums mm-util mail-prsvr
mail-utils vertico-directory vertico-repeat autorevert filenotify ffap
thingatpt url-parse auth-source password-cache url-vars xclip yasnippet
elec-pair flyspell-correct flyspell ispell company-semantic
company-template company-capf company flycheck json map find-func dash
pcase diff-hl-margin diff-hl-dired advice dired-x dired dired-loaddefs
diff-hl log-view pcvs-util vc-dir vc vc-dispatcher diff-mode orderless
vertico-mouse vertico cape term/tmux term/xterm xterm init composable
composable-mark ggtags etags fileloop generator xref project compile
text-property-search comint ansi-color ewoc repeat xt-mouse
simple-16-theme edmacro kmacro mb-depth winner ring saveplace delsel
savehist display-fill-column-indicator display-line-numbers diminish
which-key cl-extra help-mode use-package use-package-ensure
use-package-delight use-package-diminish use-package-bind-key bind-key
easy-mmode use-package-core disp-table info ede/auto eieio-base cl-seq
eieio seq subr-x byte-opt bytecomp byte-compile cconv eieio-core cl-macs
gv eieio-loaddefs cl-loaddefs cl-lib tex-site rx slime-autoloads
early-init iso-transl tooltip eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/pgtk-win pgtk-win
term/common-win 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 cl-generic 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 simple abbrev obarray cl-preloaded nadvice button
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
lcms2 multi-tty make-network-process native-compile emacs)

Memory information:
((conses 16 249128 19383)
 (symbols 48 18537 7)
 (strings 32 64337 5041)
 (string-bytes 1 2193613)
 (vectors 16 33919)
 (vector-slots 8 981792 104476)
 (floats 8 111 1580)
 (intervals 56 2279 427)
 (buffers 992 13))

bug#54450: 29.0.50; redisplay--pre-redisplay-functions issue

[Eli Zaretskii]

> Date: Fri, 18 Mar 2022 16:38:45 +0100
> From:  Ergus via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> emacs -Q
> (setq highlight-nonselected-windows t)
> 
> C-x 2
> C-x o
> C-x C-f /tmp/test
> (insert some text)
> C-a C-SPC C-f C-f C-f
> C-x o 
> 
> [at this point we should see a region highlighted over the 3 first
> letters of the inserted text in the other winfow buffer (test)]
> 
> then eval this several times:
> 
> (with-selected-window (get-buffer-window "test" 0)
>     (right-char))
> 
> This will move the point in the test window, but the region won't be
> updated.

Thanks.  Does the patch below solve the problem?

diff --git a/src/intervals.c b/src/intervals.c
index 687b237..cbcc100 100644
--- a/src/intervals.c
+++ b/src/intervals.c
@@ -44,6 +44,7 @@
 #include "lisp.h"
 #include "intervals.h"
 #include "buffer.h"
+#include "window.h"
 #include "puresize.h"
 #include "keymap.h"
 
@@ -1885,6 +1886,11 @@ set_point_both (ptrdiff_t charpos, ptrdiff_t bytepos)
       return;
     }
 
+  if (highlight_nonselected_windows
+      && !NILP (Vtransient_mark_mode)
+      && !NILP (BVAR (current_buffer, mark_active)))
+    bset_redisplay (current_buffer);
+
   /* Set TO to the interval containing the char after CHARPOS,
      and TOPREV to the interval containing the char before CHARPOS.
      Either one may be null.  They may be equal.  */

bug#54450: 29.0.50; redisplay--pre-redisplay-functions issue

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

On Fri, Mar 18, 2022 at 07:01:06PM +0200, Eli Zaretskii wrote:
>> Date: Fri, 18 Mar 2022 16:38:45 +0100
>> From:  Ergus via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> emacs -Q
>> (setq highlight-nonselected-windows t)
>>
>> C-x 2
>> C-x o
>> C-x C-f /tmp/test
>> (insert some text)
>> C-a C-SPC C-f C-f C-f
>> C-x o
>>
>> [at this point we should see a region highlighted over the 3 first
>> letters of the inserted text in the other winfow buffer (test)]
>>
>> then eval this several times:
>>
>> (with-selected-window (get-buffer-window "test" 0)
>>     (right-char))
>>
>> This will move the point in the test window, but the region won't be
>> updated.
>
>Thanks.  Does the patch below solve the problem?
>
>diff --git a/src/intervals.c b/src/intervals.c
>index 687b237..cbcc100 100644
>--- a/src/intervals.c
>+++ b/src/intervals.c
>@@ -44,6 +44,7 @@
> #include "lisp.h"
> #include "intervals.h"
> #include "buffer.h"
>+#include "window.h"
> #include "puresize.h"
> #include "keymap.h"
>
>@@ -1885,6 +1886,11 @@ set_point_both (ptrdiff_t charpos, ptrdiff_t bytepos)
>       return;
>     }
>
>+  if (highlight_nonselected_windows
>+      && !NILP (Vtransient_mark_mode)
>+      && !NILP (BVAR (current_buffer, mark_active)))
>+    bset_redisplay (current_buffer);
>+
>   /* Set TO to the interval containing the char after CHARPOS,
>      and TOPREV to the interval containing the char before CHARPOS.
>      Either one may be null.  They may be equal.  */

Hi Eli:

Yes, this could solve the issue for the region, but not in general for
redisplay--update-cursor-face-highlight which was the initial problem
with Juri's code. Same for any other function that goes into the
pre-redisplay-functions hook.

The region example reported in this issue was only the simplest way I
found to give you a simple reproducible snippet, but the issue is a bit
more general.

With the cursor-face we may have a similar issue cause such hook works
in the same way, so this condition will need to be extended and extended
and extended on every case for every function in
pre-redisplay-functions hook.

else

For Juri's code maybe there is a way that he can explicitly do
bset_redisplay at the end of the with-selected-window?

Does it makes sense?

Best,
Ergus

bug#54450: 29.0.50; redisplay--pre-redisplay-functions issue

[Eli Zaretskii]

> Date: Fri, 18 Mar 2022 18:42:52 +0100
> From: Ergus <spacibba@aol.com>
> Cc: 54450@debbugs.gnu.org
> 
> >+  if (highlight_nonselected_windows
> >+      && !NILP (Vtransient_mark_mode)
> >+      && !NILP (BVAR (current_buffer, mark_active)))
> >+    bset_redisplay (current_buffer);
> >+
> >   /* Set TO to the interval containing the char after CHARPOS,
> >      and TOPREV to the interval containing the char before CHARPOS.
> >      Either one may be null.  They may be equal.  */
> 
> Hi Eli:
> 
> Yes, this could solve the issue for the region, but not in general for
> redisplay--update-cursor-face-highlight which was the initial problem
> with Juri's code. Same for any other function that goes into the
> pre-redisplay-functions hook.

I asked for a test case, and this is what you gave me.

If there's another test case, please show it.

> The region example reported in this issue was only the simplest way I
> found to give you a simple reproducible snippet, but the issue is a bit
> more general.
> 
> With the cursor-face we may have a similar issue cause such hook works
> in the same way, so this condition will need to be extended and extended
> and extended on every case for every function in
> pre-redisplay-functions hook.

I need to understand the more general case in practical terms.  Then I
can think about a solution.

bug#54450: 29.0.50; redisplay--pre-redisplay-functions issue

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

On Fri, Mar 18, 2022 at 08:38:18PM +0200, Eli Zaretskii wrote:
>> Date: Fri, 18 Mar 2022 18:42:52 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: 54450@debbugs.gnu.org
>>
>> >+  if (highlight_nonselected_windows
>> >+      && !NILP (Vtransient_mark_mode)
>> >+      && !NILP (BVAR (current_buffer, mark_active)))
>> >+    bset_redisplay (current_buffer);
>> >+
>> >   /* Set TO to the interval containing the char after CHARPOS,
>> >      and TOPREV to the interval containing the char before CHARPOS.
>> >      Either one may be null.  They may be equal.  */
>>
>> Hi Eli:
>>
>> Yes, this could solve the issue for the region, but not in general for
>> redisplay--update-cursor-face-highlight which was the initial problem
>> with Juri's code. Same for any other function that goes into the
>> pre-redisplay-functions hook.
>
>I asked for a test case, and this is what you gave me.
>
>If there's another test case, please show it.
>
Hi Eli

A similar issue happens if in the test buffer you do:

M-x cursor-face-highlight-mode
(setq cursor-face-highlight-nonselected-window t)
M-: (insert "AAA"(propertize "Hello World" 'cursor-face 'highlight)"BBB")

Then try to move the cursor over the inserted text with the arrows (you
should see the highlight when cursor is over the propertized text); The
try te same and from other buffer (using with-selected-window).

In the second case you will see the cursor moving but the highlighted
not updated.

>> The region example reported in this issue was only the simplest way I
>> found to give you a simple reproducible snippet, but the issue is a bit
>> more general.
>>
>> With the cursor-face we may have a similar issue cause such hook works
>> in the same way, so this condition will need to be extended and extended
>> and extended on every case for every function in
>> pre-redisplay-functions hook.
>
>I need to understand the more general case in practical terms.  Then I
>can think about a solution.

bug#54450: 29.0.50; redisplay--pre-redisplay-functions issue

[Eli Zaretskii]

> Date: Fri, 18 Mar 2022 19:52:14 +0100
> From: Ergus <spacibba@aol.com>
> Cc: 54450@debbugs.gnu.org
> 
> A similar issue happens if in the test buffer you do:
> 
> M-x cursor-face-highlight-mode
> (setq cursor-face-highlight-nonselected-window t)
> M-: (insert "AAA"(propertize "Hello World" 'cursor-face 'highlight)"BBB")

There's no such mode in master, right?

Anyway, please try the last patch I posted and see if it solves the
problem.

bug#54450: 29.0.50; redisplay--pre-redisplay-functions issue

[Eli Zaretskii]

> Date: Fri, 18 Mar 2022 20:38:18 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 54450@debbugs.gnu.org
> 
> > Yes, this could solve the issue for the region, but not in general for
> > redisplay--update-cursor-face-highlight which was the initial problem
> > with Juri's code. Same for any other function that goes into the
> > pre-redisplay-functions hook.
> 
> I asked for a test case, and this is what you gave me.
> 
> If there's another test case, please show it.

But before you do, please try the alternative patch below:

diff --git a/src/xdisp.c b/src/xdisp.c
index 5cb5839..6d8d4aa 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -13211,7 +13211,8 @@ prepare_menu_bars (void)
 	      struct window *w = XWINDOW (this);
 	      if (w->redisplay
 		  || XFRAME (w->frame)->redisplay
-		  || XBUFFER (w->contents)->text->redisplay)
+		  || XBUFFER (w->contents)->text->redisplay
+		  || w->last_point != marker_position (w->pointm))
 		{
 		  windows = Fcons (this, windows);
 		}

bug#54450: 29.0.50; redisplay--pre-redisplay-functions issue

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

On Fri, Mar 18, 2022 at 08:54:00PM +0200, Eli Zaretskii wrote:
>> Date: Fri, 18 Mar 2022 20:38:18 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: 54450@debbugs.gnu.org
>>
>> > Yes, this could solve the issue for the region, but not in general for
>> > redisplay--update-cursor-face-highlight which was the initial problem
>> > with Juri's code. Same for any other function that goes into the
>> > pre-redisplay-functions hook.
>>
>> I asked for a test case, and this is what you gave me.
>>
>> If there's another test case, please show it.
>
>But before you do, please try the alternative patch below:
>
>diff --git a/src/xdisp.c b/src/xdisp.c
>index 5cb5839..6d8d4aa 100644
>--- a/src/xdisp.c
>+++ b/src/xdisp.c
>@@ -13211,7 +13211,8 @@ prepare_menu_bars (void)
> 	      struct window *w = XWINDOW (this);
> 	      if (w->redisplay
> 		  || XFRAME (w->frame)->redisplay
>-		  || XBUFFER (w->contents)->text->redisplay)
>+		  || XBUFFER (w->contents)->text->redisplay
>+		  || w->last_point != marker_position (w->pointm))
> 		{
> 		  windows = Fcons (this, windows);
> 		}

Yes, now this seems to solve the issue for the two use cases described.

Thanks,
Ergus

bug#54450: 29.0.50; redisplay--pre-redisplay-functions issue

[Eli Zaretskii]

> Date: Fri, 18 Mar 2022 22:25:24 +0100
> From: Ergus <spacibba@aol.com>
> Cc: 54450@debbugs.gnu.org
> 
> On Fri, Mar 18, 2022 at 08:54:00PM +0200, Eli Zaretskii wrote:
> >> Date: Fri, 18 Mar 2022 20:38:18 +0200
> >> From: Eli Zaretskii <eliz@gnu.org>
> >> Cc: 54450@debbugs.gnu.org
> >>
> >> > Yes, this could solve the issue for the region, but not in general for
> >> > redisplay--update-cursor-face-highlight which was the initial problem
> >> > with Juri's code. Same for any other function that goes into the
> >> > pre-redisplay-functions hook.
> >>
> >> I asked for a test case, and this is what you gave me.
> >>
> >> If there's another test case, please show it.
> >
> >But before you do, please try the alternative patch below:
> >
> >diff --git a/src/xdisp.c b/src/xdisp.c
> >index 5cb5839..6d8d4aa 100644
> >--- a/src/xdisp.c
> >+++ b/src/xdisp.c
> >@@ -13211,7 +13211,8 @@ prepare_menu_bars (void)
> > 	      struct window *w = XWINDOW (this);
> > 	      if (w->redisplay
> > 		  || XFRAME (w->frame)->redisplay
> >-		  || XBUFFER (w->contents)->text->redisplay)
> >+		  || XBUFFER (w->contents)->text->redisplay
> >+		  || w->last_point != marker_position (w->pointm))
> > 		{
> > 		  windows = Fcons (this, windows);
> > 		}
> 
> Yes, now this seems to solve the issue for the two use cases described.

Thanks, I installed a slightly modified variant of this on the
emacs-28 branch.