bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

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

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

emacs -q:

1. 'C-h e' to display *Messages* in a new window
2. 'C-SPC C-n' to mark an active region
3. 'M-x window-swap-states'
4. 'C-g C-n' to deactivate the mark

Notice how the region overlay stays there.

To make this easier to debug, instrument 'window-swap-states' by
applying the following diff

diff --git a/lisp/window.el b/lisp/window.el
index 905803b19e..daddd18b74 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -6518,8 +6518,14 @@ window-swap-states
            (height-2 (and height (window-text-height window-2 t)))
            old preserved)
       ;; Swap basic states.
+      (message "Before %S \n %S"
+               (window-parameter window-1 'internal-region-overlay)
+               (window-parameter window-2 'internal-region-overlay))
       (window-state-put state-1 window-2 t)
       (window-state-put state-2 window-1 t)
+      (message "After: %S \n %S"
+               (window-parameter window-1 'internal-region-overlay)
+               (window-parameter window-2 'internal-region-overlay))
       ;; Swap overlays with `window' property.
       (with-current-buffer (window-buffer window-1)
         (dolist (overlay (overlays-in (point-min) (point-max)))

Follow the bug recipe and notice the following messages:

Before: #<overlay from 77 to 123 in *GNU Emacs*> 
 nil
After: nil 
 nil

window-state-put sets 'internal-region-overlay' window parameter to nil
without removing the actual region overlay, so it remains there in the
buffer. I'm not sure how to fix this. Perhaps we should add
'internal-region-overlay' to 'window-persistent-parameters'? Are other
commands that use window-state-get + window-state-put affected by this
bug as well?

In GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.34, cairo version 1.17.6) of 2022-09-29 built on miha-pc
Repository revision: 0edd7770e0ce70cac59f239134962d10f48dec79
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Arch Linux

Configured using:
 'configure -C --prefix=/usr --without-libsystemd
 --enable-checking=yes,glyphs --enable-check-lisp-object-type
 'CFLAGS=-O0 -g3''

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBXML2 M17N_FLT MODULES NOTIFY INOTIFY PDUMPER PNG
RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11
XDBE XIM XINPUT2 XPM GTK3 ZLIB

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

Major mode: Shell

Minor modes in effect:
  shell-dirtrack-mode: t
  comint-fl-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils sh-script smie
executable files-x shell pcomplete comint osc ansi-color ring tabify
thingatpt help-fns radix-tree help-mode cus-edit pp cus-start cus-load
icons wid-edit time-date subr-x cl-loaddefs cl-lib rmc iso-transl
tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/x-win x-win term/common-win x-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 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 lcms2
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit xinput2 x multi-tty make-network-process
emacs)

Memory information:
((conses 16 77394 9282)
 (symbols 48 8002 0)
 (strings 32 23223 1949)
 (string-bytes 1 634193)
 (vectors 16 13518)
 (vector-slots 8 208026 15295)
 (floats 8 46 42)
 (intervals 56 1050 0)
 (buffers 1000 16))

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

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[Eli Zaretskii]

> Date: Thu, 29 Sep 2022 19:29:46 +0200
> From: miha--- via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> 1. 'C-h e' to display *Messages* in a new window
> 2. 'C-SPC C-n' to mark an active region
> 3. 'M-x window-swap-states'
> 4. 'C-g C-n' to deactivate the mark
> 
> Notice how the region overlay stays there.

"There" where?

What did you expect to happen in the above scenario, and why?

> window-state-put sets 'internal-region-overlay' window parameter to nil
> without removing the actual region overlay

Where in the code do you see that?  I see this:

      (with-current-buffer (window-buffer window-1)
        (dolist (overlay (overlays-in (point-min) (point-max)))
          (let ((window (overlay-get overlay 'window)))
            (cond
             ((not window))
             ((eq window window-1)
              (overlay-put overlay 'window window-2))
             ((eq window window-2)
              (overlay-put overlay 'window window-1))))))

AFAIU, this _swaps_ the 'window' property of the overlays, so that the
overlay now belongs (and should be visible) in the other window.
Which is what I should expect.

What am I missing?

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

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

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Thu, 29 Sep 2022 19:29:46 +0200
>> From: miha--- via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> 1. 'C-h e' to display *Messages* in a new window
>> 2. 'C-SPC C-n' to mark an active region
>> 3. 'M-x window-swap-states'
>> 4. 'C-g C-n' to deactivate the mark
>> 
>> Notice how the region overlay stays there.
>
> "There" where?

In the *GNU Emacs* buffer, where we have originally marked a region in
step 2.

> What did you expect to happen in the above scenario, and why?

I expect the overlay to disappear after deactivating the mark with 'C-g'
in step 4.

>> window-state-put sets 'internal-region-overlay' window parameter to nil
>> without removing the actual region overlay
>
> Where in the code do you see that?  I see this:
>
>       (with-current-buffer (window-buffer window-1)
>         (dolist (overlay (overlays-in (point-min) (point-max)))
>           (let ((window (overlay-get overlay 'window)))
>             (cond
>              ((not window))
>              ((eq window window-1)
>               (overlay-put overlay 'window window-2))
>              ((eq window window-2)
>               (overlay-put overlay 'window window-1))))))
>
> AFAIU, this _swaps_ the 'window' property of the overlays, so that the
> overlay now belongs (and should be visible) in the other window.
> Which is what I should expect.

Indeed the overlay is shown in the correct window after step 3. The
problem is that it doesn't disappear after step 4
('redisplay--update-region-highlight' doesn't remove it).

> What am I missing?

Perhaps you forgot to deactivate the mark in step 4? If there is
anything unclear in the bug recipe, go ahead and ask.

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

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[Eli Zaretskii]

> From: <miha@kamnitnik.top>
> Cc: 58175@debbugs.gnu.org
> Date: Thu, 29 Sep 2022 21:17:56 +0200
> 
> >> 1. 'C-h e' to display *Messages* in a new window
> >> 2. 'C-SPC C-n' to mark an active region
> >> 3. 'M-x window-swap-states'
> >> 4. 'C-g C-n' to deactivate the mark
> >> 
> >> Notice how the region overlay stays there.
> >
> > "There" where?
> 
> In the *GNU Emacs* buffer, where we have originally marked a region in
> step 2.
> 
> > What did you expect to happen in the above scenario, and why?
> 
> I expect the overlay to disappear after deactivating the mark with 'C-g'
> in step 4.

Ah!  Now it's clear what this issue is about.

The problem is that deactivate-mark doesn't work in this case, for
some reason, and neither is setting mark-active to nil.

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[Eli Zaretskii]

> Cc: 58175@debbugs.gnu.org
> Date: Thu, 29 Sep 2022 22:19:34 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: <miha@kamnitnik.top>
> > Cc: 58175@debbugs.gnu.org
> > Date: Thu, 29 Sep 2022 21:17:56 +0200
> > 
> > >> 1. 'C-h e' to display *Messages* in a new window
> > >> 2. 'C-SPC C-n' to mark an active region
> > >> 3. 'M-x window-swap-states'
> > >> 4. 'C-g C-n' to deactivate the mark
> > >> 
> > >> Notice how the region overlay stays there.
> > >
> > > "There" where?
> > 
> > In the *GNU Emacs* buffer, where we have originally marked a region in
> > step 2.
> > 
> > > What did you expect to happen in the above scenario, and why?
> > 
> > I expect the overlay to disappear after deactivating the mark with 'C-g'
> > in step 4.
> 
> Ah!  Now it's clear what this issue is about.
> 
> The problem is that deactivate-mark doesn't work in this case, for
> some reason, and neither is setting mark-active to nil.

Looks like the internal-region-overlay window parameter, which is
important for correct workings of region-highlight, isn't copied
correctly to the other window as part of swapping state, because its
value ends up as "overlay N in no buffer", i.e. the overlay's buffer
is lost in transition.

Martin, Stefan: any suggestions or ideas?

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[martin rudalics]

 > Looks like the internal-region-overlay window parameter, which is
 > important for correct workings of region-highlight, isn't copied
 > correctly to the other window as part of swapping state, because its
 > value ends up as "overlay N in no buffer", i.e. the overlay's buffer
 > is lost in transition.

If you want a window parameter to get copied when swapping window states,
you have to explicitly mark it as "persistent".  For example with:

(push '(internal-region-overlay . t) window-persistent-parameters)

martin

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[Eli Zaretskii]

> Date: Tue, 4 Oct 2022 10:23:49 +0200
> Cc: 58175@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
> From: martin rudalics <rudalics@gmx.at>
> 
>  > Looks like the internal-region-overlay window parameter, which is
>  > important for correct workings of region-highlight, isn't copied
>  > correctly to the other window as part of swapping state, because its
>  > value ends up as "overlay N in no buffer", i.e. the overlay's buffer
>  > is lost in transition.
> 
> If you want a window parameter to get copied when swapping window states,
> you have to explicitly mark it as "persistent".  For example with:
> 
> (push '(internal-region-overlay . t) window-persistent-parameters)

Thanks.  Does this affect only window-swap-states, or does this affect
anything else?  If the former, I guess the above should be done
globally when Emacs is dumped?

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

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

>>  > Looks like the internal-region-overlay window parameter, which is
>>  > important for correct workings of region-highlight, isn't copied
>>  > correctly to the other window as part of swapping state, because its
>>  > value ends up as "overlay N in no buffer", i.e. the overlay's buffer
>>  > is lost in transition.
>> 
>> If you want a window parameter to get copied when swapping window states,
>> you have to explicitly mark it as "persistent".  For example with:
>> 
>> (push '(internal-region-overlay . t) window-persistent-parameters)

Hmm... I must say I misunderstood the report when I read it originally.
Now that I see a bit more clearly what it's about I wonder why we'd have
to do something special (w.r.t `internal-region-overlay`) for
`window-swap-states` compared to what we do (i.e. nothing at all) when
we do `set-window-buffer`.

More specifically, AFAICT the code that uses `internal-region-overlay`
just tries to reuse that info to try and reduce memory churn, but it
should work correctly even when `internal-region-overlay` points to the
wrong buffer or even if it's not an overlay at all.

IOW, I suspect the bug is in `redisplay-(un)highlight-region-function`
and adding `internal-region-overlay` to `window-persistent-parameters`
would likely just cover it for that use-case but it could
reoccur elsewhere.


        Stefan

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

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

> IOW, I suspect the bug is in `redisplay-(un)highlight-region-function`
> and adding `internal-region-overlay` to `window-persistent-parameters`
> would likely just cover it for that use-case but it could
> reoccur elsewhere.

Hmm... then again not.  Still thinking about it.


        Stefan

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

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

>> IOW, I suspect the bug is in `redisplay-(un)highlight-region-function`
>> and adding `internal-region-overlay` to `window-persistent-parameters`
>> would likely just cover it for that use-case but it could
>> reoccur elsewhere.
> Hmm... then again not.  Still thinking about it.

I guess it boils down to whether it's OK for a function like
`window--state-put-2` to "unilaterally" set window parameters to nil as
it does in:

      ;; Reset window's parameters and assign saved ones (we might want
      ;; a `remove-window-parameters' function here).
      (dolist (parameter (window-parameters window))
	(set-window-parameter window (car parameter) nil))

I don't think it's right to add `internal-region-overlay` to
`window-persistent-parameters` since we don't want/need to store those
overlays in window-state objects.

We could change the above code so it only sets to nil those
parameters that are listed in `window-persistent-parameters`, but I'm
not sure if that's the right choice.  It might be, tho: it seems odd to
just zap properties owned by arbitrary packages without giving them
a chance to "say goodbye".

Or we could add some kind of hook (similar to a `change-major-mode-hook`
but for window state changes rather than major mode changes) so code
like the region-highlight code can register itself there to throw away
its overlays before a new window-state is installed.

Or we need to change the `redisplay--(un)highlight-overlay-function`s so
as to keep their overlays (and similar info) elsewhere, probably in
a variable rather than a window-parameter since window-parameters can
disappear without warning.


        Stefan

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: martin rudalics <rudalics@gmx.at>,  58175@debbugs.gnu.org,
>   miha@kamnitnik.top
> Date: Tue, 04 Oct 2022 17:25:13 -0400
> 
>       ;; Reset window's parameters and assign saved ones (we might want
>       ;; a `remove-window-parameters' function here).
>       (dolist (parameter (window-parameters window))
> 	(set-window-parameter window (car parameter) nil))
> 
> I don't think it's right to add `internal-region-overlay` to
> `window-persistent-parameters` since we don't want/need to store those
> overlays in window-state objects.
> 
> We could change the above code so it only sets to nil those
> parameters that are listed in `window-persistent-parameters`, but I'm
> not sure if that's the right choice.  It might be, tho: it seems odd to
> just zap properties owned by arbitrary packages without giving them
> a chance to "say goodbye".

Martin will tell, but I'm pretty sure this wasn't born out of thin
air.  I'm sure there are window parameters that will do harm if
copied.  Look at the list of window parameters in the "Window
Parameters" node of the ELisp manual, and try to convince yourself
that you want to copy all of them by default (we currently only copy
clone-of, AFAIU).

> Or we could add some kind of hook (similar to a `change-major-mode-hook`
> but for window state changes rather than major mode changes) so code
> like the region-highlight code can register itself there to throw away
> its overlays before a new window-state is installed.

Why is this cleaner than maintaining a list of "persistent"
parameters?

> Or we need to change the `redisplay--(un)highlight-overlay-function`s so
> as to keep their overlays (and similar info) elsewhere, probably in
> a variable rather than a window-parameter since window-parameters can
> disappear without warning.

Maybe.  But why complicate a mechanism that is already extremely
complicated and hard to understand for a bystander, and needed several
fixes until we got it right?  The use case in this bug report is
pretty marginal, so much so that from where I stand we could
legitimately say "this is not supported".  It hardly warrants making
questionable changes in mechanisms that are so central to routine
Emacs operation.

Btw, if we do want to consider changes in the region-overlay
machinery, then how about having
redisplay--unhighlight-overlay-function clean up by deleting overlays
whose buffer is nil or dead?  That would at least avoid leaving around
"stale" overlays that were once the region, which is what happens in
this case.

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

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

>> We could change the above code so it only sets to nil those
>> parameters that are listed in `window-persistent-parameters`, but I'm
>> not sure if that's the right choice.  It might be, tho: it seems odd to
>> just zap properties owned by arbitrary packages without giving them
>> a chance to "say goodbye".
>
> Martin will tell, but I'm pretty sure this wasn't born out of thin
> air.

Could be, but the behavior is not documented, AFAICT: the doc seems to
suggest that `window-state-put` doesn't touch the parameters that are
not mentioned in `window-persistent-parameters` (whereas it actually
throws them out unconditionally).

> I'm sure there are window parameters that will do harm if
> copied.

I'm not talking about copying.  I'm talking about leaving them where
they are.

>> Or we could add some kind of hook (similar to a `change-major-mode-hook`
>> but for window state changes rather than major mode changes) so code
>> like the region-highlight code can register itself there to throw away
>> its overlays before a new window-state is installed.
> Why is this cleaner than maintaining a list of "persistent"
> parameters?

Notice there are two notions of "persistent" here.

Let's say we use `window-stat-save` in window A and then
`window-state-put` in window B:

- `window-persistent-parameters` lets you control which parameters of
  window A are "persisted/copied" to B.  `internal-region-overlay`
  doesn't want to be among those copied parameters.

- I'm suggesting we add some way to control what happens to parameters
  that were in window B.  Clearly, for those parameters in
  `window-persistent-parameters` they'll have to be overwritten.
  But currently they are all wiped out unconditionally just before
  putting the new state, which is a problem in the case of
  `internal-region-overlay` where we don't necessarily need to preserve
  its value (tho that would work as well), but we'd need to remove it
  a bit more carefully at least.

I see `window-state-put` as something similar to calling a major-mode:
it starts by "killing all local variables" (i.e. removing all window
parameters) and then sets up its own state.

I see 3 options:

- Change `window-state-put` so it doesn't touch those parameters not
  mentioned in `window-persistent-parameters`.  This is arguably the
  simplest change and IMO it would make it behave closer to what its
  doc suggests.

- Add a `before-clearing-window-parameters-hook`, just like
  `kill-all-local-variables` runs the `change-major-mode-hook` (tho, if
  so, we should design it such that it's a bit easier for that hook
  to make some parameters survive unscathed).

- Add a new variable (or some new special value for
  `window-persistent-parameters`) listing those window parameters that
  should not be touched by `window-state-put` (i.e. the equivalent of
  "persistent variables").


        Stefan

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[martin rudalics]

 > Could be, but the behavior is not documented, AFAICT: the doc seems to
 > suggest that `window-state-put` doesn't touch the parameters that are
 > not mentioned in `window-persistent-parameters` (whereas it actually
 > throws them out unconditionally).

AFAICT that's consistent with the remaining behavior of these functions.
The values of the window where the state is put are completely replaced
by the values of the window where the state has been obtained from.

 > I see 3 options:

I'm still not convinced that window parameters are the best choice for
keeping information about the highlighted region.  I think they should
be used only for things that do not depend on the buffer shown in that
window or on some global variable.

The parameter used here is a conglomerate - 'window-point' is window
local, the mark is buffer local and which window is the selected one is
global.  But since, as Eli said, we also may want to highlight the
region in non-selected windows, there might be no better choice.  In
either case, please keep in mind that the persistence of parameters must
be also handled by ‘set-window-configuration’ though that one never has
to transfer properties from one window to another.

martin

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

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

>> Could be, but the behavior is not documented, AFAICT: the doc seems to
>> suggest that `window-state-put` doesn't touch the parameters that are
>> not mentioned in `window-persistent-parameters` (whereas it actually
>> throws them out unconditionally).
> AFAICT that's consistent with the remaining behavior of these functions.

Could you clarify what you mean by that?
Which other functions and what "remaining behavior" are you thinking of?

> The values of the window where the state is put are completely replaced
> by the values of the window where the state has been obtained from.

That's indeed the behavior of the code, as I pointed it out, but the
docstring of `window-persistent-parameters` says:

    Parameters not saved by ‘current-window-configuration’ or
    ‘window-state-get’ are left alone by ‘set-window-configuration’
    respectively are not installed by ‘window-state-put’.

"not installed" is not the same as "thrown away".

If we want to keep this behavior, we should document it a bit more
clearly, I think.  Maybe that's what you meant by the distinction
between "left alone" and "not installed"?

Also, I think it's worthwhile then to add some hook run before throwing
away that info.

>> I see 3 options:
> I'm still not convinced that window parameters are the best choice for
> keeping information about the highlighted region.

You might be right, but that's a somewhat orthogonal discussion.
I don't think this choice should be imposed by a specific choice of
behavior of `window-state-put`.

> The parameter used here is a conglomerate - 'window-point' is window
> local, the mark is buffer local and which window is the selected one is
> global.  But since, as Eli said, we also may want to highlight the
> region in non-selected windows, there might be no better choice.

Yes, I think we'll have to use a hash-table (weakly) indexed by windows :-(

> In either case, please keep in mind that the persistence of parameters
> must be also handled by ‘set-window-configuration’ though that one
> never has to transfer properties from one window to another.

AFAIK the current code works fine with `set-window-configuration`.
And indeed `set-window-configuration` behaves the way I suggest
`window-state-put` should behave:

	  /* Restore any window parameters that have been saved.
	     Parameters that have not been saved are left alone.  */


-- Stefan

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[martin rudalics]

 > Could you clarify what you mean by that?
 > Which other functions and what "remaining behavior" are you thinking of?

I meant the functions 'window-state-put' and 'set-window-configuration'
and the "remaining behavior" stands for how these functions restore
components like a window's scroll bars or fringes from the saved values.

 >> The values of the window where the state is put are completely replaced
 >> by the values of the window where the state has been obtained from.
 >
 > That's indeed the behavior of the code, as I pointed it out, but the
 > docstring of `window-persistent-parameters` says:
 >
 >      Parameters not saved by ‘current-window-configuration’ or
 >      ‘window-state-get’ are left alone by ‘set-window-configuration’
 >      respectively are not installed by ‘window-state-put’.
 >
 > "not installed" is not the same as "thrown away".

 > If we want to keep this behavior, we should document it a bit more
 > clearly, I think.  Maybe that's what you meant by the distinction
 > between "left alone" and "not installed"?

What we talk about here is this form in 'window--state-put-2':

       (dolist (parameter (window-parameters window))
	(set-window-parameter window (car parameter) nil))

which has no equivalent in 'set-window-configuration'.

The distinction is due to the fact that 'set-window-configuration' works
on already existing (albeit temporarily deleted) windows while
'window-state-put' as a rule has to work on pristine windows.  The
latter have no parameters from the outset.  'window-swap-states', which
was added later, is an exclusion from that rule because it has to work
on an already existing window.  Maybe that aspect is biting us here and
we should leave pre-existing parameters alone as you suggest.

But doesn't the fact that 'set-window-configuration' handles a nil value
of a parameter distinctly from "no value for that parameter has been
stored yet" mean that it handles window parameters specially, unlike
other window components or the way we handle frame parameters?

And haven't we discussed these issues already here

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=10348

and here?

https://lists.gnu.org/archive/html/emacs-diffs/2012-01/msg00140.html

 > AFAIK the current code works fine with `set-window-configuration`.
 > And indeed `set-window-configuration` behaves the way I suggest
 > `window-state-put` should behave:
 >
 > 	  /* Restore any window parameters that have been saved.
 > 	     Parameters that have not been saved are left alone.  */

But 'set-window-configuration' does not DTRT when the region overlay
parameter is made persistent.

martin

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

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

> What we talk about here is this form in 'window--state-put-2':
>
>       (dolist (parameter (window-parameters window))
> 	(set-window-parameter window (car parameter) nil))

Yes.

> which has no equivalent in 'set-window-configuration'.
>
> The distinction is due to the fact that 'set-window-configuration' works
> on already existing (albeit temporarily deleted) windows while
> 'window-state-put' as a rule has to work on pristine windows.

But the above `dolist` does nothing on pristine windows.
So it's been put there explicitly for the case of non-pristine windows.

> Maybe that aspect is biting us here and we should leave pre-existing
> parameters alone as you suggest.

It seems like the simpler option, yes.

> But doesn't the fact that 'set-window-configuration' handles a nil value
> of a parameter distinctly from "no value for that parameter has been
> stored yet" mean that it handles window parameters specially, unlike
> other window components or the way we handle frame parameters?

I don't follow.  AFAIK the special handling of nil there is simply an
optimization to avoid adding useless nil entries in the parameter list.

> And haven't we discussed these issues already here
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=10348
>
> and here?
>
> https://lists.gnu.org/archive/html/emacs-diffs/2012-01/msg00140.html

Thanks for these links.

It was very helpful for me to re-read what those past Stefan and Martin
guys had to say.  Tho it seems they didn't talk very much about what
happens to the poor window-parameters of the target windows, they only
talked about the parameters of the source windows and how/if they should
be transferred to a target window (potentially via a file).

The current bug report points out that we should also pay attention to
the parameters of the target window (because we overwrite them or throw
them away).

>> AFAIK the current code works fine with `set-window-configuration`.
>> And indeed `set-window-configuration` behaves the way I suggest
>> `window-state-put` should behave:
>>
>> 	  /* Restore any window parameters that have been saved.
>> 	     Parameters that have not been saved are left alone.  */
>
> But 'set-window-configuration' does not DTRT when the region overlay
> parameter is made persistent.

But that's because that window parameter should *not* be persistent.


        Stefan

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[martin rudalics]

 >> What we talk about here is this form in 'window--state-put-2':
 >>
 >>        (dolist (parameter (window-parameters window))
 >> 	(set-window-parameter window (car parameter) nil))
 >
 > Yes.
 >
 >> which has no equivalent in 'set-window-configuration'.
 >>
 >> The distinction is due to the fact that 'set-window-configuration' works
 >> on already existing (albeit temporarily deleted) windows while
 >> 'window-state-put' as a rule has to work on pristine windows.
 >
 > But the above `dolist` does nothing on pristine windows.
 > So it's been put there explicitly for the case of non-pristine windows.

Because the idea was to keep the behaviors consistent: Regardless of
whether a window is new or old, it should get the same values for all
parameters.  But maybe that idea was misguided: 'window-swap-states' was
added two versions after 'window-state-get'.  So lets get rid of these
lines.

 >> But doesn't the fact that 'set-window-configuration' handles a nil value
 >> of a parameter distinctly from "no value for that parameter has been
 >> stored yet" mean that it handles window parameters specially, unlike
 >> other window components or the way we handle frame parameters?
 >
 > I don't follow.  AFAIK the special handling of nil there is simply an
 > optimization to avoid adding useless nil entries in the parameter list.

'frame-parameters' gets me entries for all parameters including those
that have never been assigned a value to - more than 130 entries here,
half of them nil.  So when you want to save these values from Lisp you
do add "useless nil entries in the parameter list".

 > It was very helpful for me to re-read what those past Stefan and Martin
 > guys had to say.  Tho it seems they didn't talk very much about what
 > happens to the poor window-parameters of the target windows, they only
 > talked about the parameters of the source windows and how/if they should
 > be transferred to a target window (potentially via a file).
 >
 > The current bug report points out that we should also pay attention to
 > the parameters of the target window (because we overwrite them or throw
 > them away).

If you tried to transfer the value of ‘frame-parameters’ from one frame
to another, you would overwrite them in the target frame too.

 >> But 'set-window-configuration' does not DTRT when the region overlay
 >> parameter is made persistent.
 >
 > But that's because that window parameter should *not* be persistent.

The region highlighting code should DTRT regardless of whether its
parameter has been made persistent or not.  So once you removed the
lines above to fix the state transferal, you should try to fix this
case too.

martin

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[martin rudalics]

 >> (push '(internal-region-overlay . t) window-persistent-parameters)
 >
 > Thanks.  Does this affect only window-swap-states, or does this affect
 > anything else?

It affects every use of window states like saving and restoring the
desktop as well as saving and restoring window configurations.

Now keeping the mark active when restoring a window configuration is
problematic in the first place since it restores the mark from the saved
state while taking point from the current state possibly ending up in
some arbitrarily specified region.  OTOH deactivating the mark in such
case is hardly feasible because restoring a window configurations should
be barely perceptible for the user.

 > If the former, I guess the above should be done
 > globally when Emacs is dumped?

I would try to get rid of the window parameter used here.  Active region
highlighting is an activity that affects the selected window only and
not any window.  The 'window' property of any overlay used for it must
always refer to the selected window and not any other window.  So I see
no use for window parameters here which are mainly useful for overriding
a global variable or the local value of the buffer shown in a window.

I'd rather use one global overlay and move it (by setting its 'window'
property) whenever 'window-selection-change-functions' tell me that the
selected window has changed.  But maybe I'm missing something here.

martin

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[Eli Zaretskii]

> Date: Wed, 5 Oct 2022 09:36:40 +0200
> Cc: miha@kamnitnik.top, 58175@debbugs.gnu.org, monnier@iro.umontreal.ca
> From: martin rudalics <rudalics@gmx.at>
> 
> Now keeping the mark active when restoring a window configuration is
> problematic in the first place since it restores the mark from the saved
> state while taking point from the current state possibly ending up in
> some arbitrarily specified region.  OTOH deactivating the mark in such
> case is hardly feasible because restoring a window configurations should
> be barely perceptible for the user.

In the scenario described in this bug report, point is copied to the
new window, so the result is as expected.  Deactivating the mark also
does the expected job.  So it looks like adding
internal-region-overlay to the persistent window parameters is a good
solution in this case.  I suggest that you try that, maybe you will
see some problems that I missed.

>  > If the former, I guess the above should be done
>  > globally when Emacs is dumped?
> 
> I would try to get rid of the window parameter used here.  Active region
> highlighting is an activity that affects the selected window only and
> not any window.  The 'window' property of any overlay used for it must
> always refer to the selected window and not any other window.  So I see
> no use for window parameters here which are mainly useful for overriding
> a global variable or the local value of the buffer shown in a window.

So you are saying we should redesign how region overlay is implemented
and managed?  I'd prefer not to go there.

> I'd rather use one global overlay and move it (by setting its 'window'
> property) whenever 'window-selection-change-functions' tell me that the
> selected window has changed.  But maybe I'm missing something here.

highlight-nonselected-windows, I guess?  How can we have a single
global overlay and still support that option?

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[martin rudalics]

 > In the scenario described in this bug report, point is copied to the
 > new window, so the result is as expected.  Deactivating the mark also
 > does the expected job.  So it looks like adding
 > internal-region-overlay to the persistent window parameters is a good
 > solution in this case.  I suggest that you try that, maybe you will
 > see some problems that I missed.

With emacs -Q put the following text into *scratch*


(push '(internal-region-overlay . t) window-persistent-parameters)

(defvar foo-conf nil)

(defun foo-save ()
   (interactive)
   (setq foo-conf (current-window-configuration)))

(defun foo-restore ()
   (interactive)
   (set-window-configuration foo-conf))

(split-window)

;; (eval-buffer)


evaluate it and do M-x foo-save followed by C-x o.  Move point and
activate the region.  M-x foo-restore now gets me two overlays, one in
the upper and one in the lower window.

Now try again with 'internal-region-overlay' not made persistent, that
is, the first line commented out.  The same scenario gets me one (albeit
illogical) overlay in the upper window only.

martin

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[Eli Zaretskii]

> Date: Thu, 6 Oct 2022 09:48:12 +0200
> Cc: miha@kamnitnik.top, 58175@debbugs.gnu.org, monnier@iro.umontreal.ca
> From: martin rudalics <rudalics@gmx.at>
> 
> With emacs -Q put the following text into *scratch*
> 
> 
> (push '(internal-region-overlay . t) window-persistent-parameters)
> 
> (defvar foo-conf nil)
> 
> (defun foo-save ()
>    (interactive)
>    (setq foo-conf (current-window-configuration)))
> 
> (defun foo-restore ()
>    (interactive)
>    (set-window-configuration foo-conf))
> 
> (split-window)
> 
> ;; (eval-buffer)
> 
> 
> evaluate it and do M-x foo-save followed by C-x o.  Move point and
> activate the region.  M-x foo-restore now gets me two overlays, one in
> the upper and one in the lower window.
> 
> Now try again with 'internal-region-overlay' not made persistent, that
> is, the first line commented out.  The same scenario gets me one (albeit
> illogical) overlay in the upper window only.

Both results in this scenario are wrong, IMO, so I don't see why we'd
prefer one of them.  I also don't understand how this scenario could
happen in real life, FWIW.

bug#58175: 29.0.50; M-x window-swap-states during an active mark leaves behind a region overlay

[martin rudalics]

 > Both results in this scenario are wrong, IMO, so I don't see why we'd
 > prefer one of them.

The one with the persistent overlay leaves behind an overlay in the
lower window that is not removed by the region highlighting code.  So
that gets us back another incarnation of the original bug.

 > I also don't understand how this scenario could
 > happen in real life, FWIW.

Given the plethora of 'set-window-configuration' and
'save-window-excursion' calls in the Emacs code base (some of them with
separate position handling instructions), I'm quite confident that it
eventually will happen.

martin