bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

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

Hi recently I found that I can't access help on read-only buffers like
*Occur*.

To reproduce:
emacs -Q
M-x context-menu-mode
C-h b

I get this text inserted in the buffer: `<menu>          context-menu-open`

And an out of range message.

`describe-buffer-bindings: Args out of range: #<buffer *scratch*>, 173, 174`

So there is the bug.

Exposed initially by this:

emacs -Q
M-x toggle-read-only
M-x context-menu-mode
C-h b

Just a message is shown in the echo area like an error because the
buffer is read only (and such text can't be inserted...).

BTW: I toggled the debug-on-entry and tried the second set of steps and
debugger was not triggered, so I am not sure if this is another issue.

Best,
Ergus.


In GNU Emacs 29.0.50 (build 62, x86_64-pc-linux-gnu, GTK+ Version 3.24.31, cairo version 1.17.4)
  of 2022-02-09 built on Ergus
Repository revision: f06915c93c0755a708f9c600e90674c68b5326dc
Repository branch: master
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:
   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
   counsel-mode: t
   ivy-mode: t
   composable-mark-mode: t
   composable-mode: t
   repeat-mode: t
   xterm-mouse-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-20220130.1941/transient hides /home/ergo/.local/share/emacs/29.0.50/lisp/transient

Features:
(shadow sort mail-extr autorevert filenotify xclip emacsbug message
mailcap yank-media rmc puny rfc822 mml mml-sec password-cache epa
derived epg rfc6068 epg-config gnus-util time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils elec-pair
flyspell-correct-ivy flyspell-correct flyspell ispell company-semantic
company-template company-capf company flycheck json map find-func dash
pcase diff-hl-margin diff-hl-dired diff-hl log-view pcvs-util vc-dir
ewoc vc vc-dispatcher diff-mode thingatpt amx comp comp-cstr warnings s
cape counsel xdg advice xref project dired dired-loaddefs compile
text-property-search comint ansi-color swiper ivy-avy avy ivy flx
ivy-faces ivy-overlay colir color term/tmux term/xterm xterm init
composable composable-mark repeat xt-mouse mb-depth edmacro kmacro
simple-16-theme 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 258691 16183)
  (symbols 48 17962 1)
  (strings 32 61649 6520)
  (string-bytes 1 2099350)
  (vectors 16 29163)
  (vector-slots 8 540047 206780)
  (floats 8 183 979)
  (intervals 56 347 0)
  (buffers 992 11))

bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Lars Ingebrigtsen]

Ergus <spacibba@aol.com> writes:

> Exposed initially by this:
>
> emacs -Q
> M-x toggle-read-only
> M-x context-menu-mode
> C-h b
>
> Just a message is shown in the echo area like an error because the
> buffer is read only (and such text can't be inserted...).
>
> BTW: I toggled the debug-on-entry and tried the second set of steps and
> debugger was not triggered, so I am not sure if this is another issue.

I can reproduce this bug, and I can't get a backtrace either, even with
debug-on-signal, which is unusual.

I had a brief peek at context-menu-mode to see whether it was obvious
what was breaking, but nothing really stood out (but then again, I'm not
very familiar with that code).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[martin rudalics]

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

 > `describe-buffer-bindings: Args out of range: #<buffer *scratch*>, 173, 174`

Please try the attached diff.

martin

[-- Attachment #2: help.el.diff --]
[-- Type: text/x-patch, Size: 417 bytes --]

diff --git a/lisp/help.el b/lisp/help.el
index 975be497e7..5b70474958 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -1349,7 +1349,8 @@ describe-map-tree
       (save-excursion
         (goto-char start-point)
         (when (eolp)
-          (delete-region (point) (1+ (point))))
+          (delete-region
+           (point) (min (1+ (point)) (point-max))))
         (insert
          (concat
           (if title

bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Juri Linkov]

>> M-x context-menu-mode
>> C-h b
>>
>> Just a message is shown in the echo area like an error because the
>> buffer is read only (and such text can't be inserted...).
>
> I can reproduce this bug, and I can't get a backtrace either, even with
> debug-on-signal, which is unusual.
>
> I had a brief peek at context-menu-mode to see whether it was obvious
> what was breaking, but nothing really stood out (but then again, I'm not
> very familiar with that code).

I can't reproduce this bug in Emacs 28, only in Emacs 29.
This means that the problem is new.  In describe-map
this line switches buffers from *Help* to the window buffer:

  (eq definition (lookup-key tail (vector event) t))

And indeed

  (with-temp-buffer
    (message "%S" (current-buffer))
    (lookup-key (cddr context-menu-mode-map) [down-mouse-3] t)
    (message "%S" (current-buffer)))

displays:

  #<buffer  *temp*>
  #<buffer *scratch*>

This is because of this line recently added to context-menu-map:

  (select-window (posn-window (event-start click)))

that switches buffers.

But the question remains: does describe-map really need to build
the complete context menu or should it ignore its :filter keyword?

bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

>   (with-temp-buffer
>     (message "%S" (current-buffer))
>     (lookup-key (cddr context-menu-mode-map) [down-mouse-3] t)
>     (message "%S" (current-buffer)))
>
> displays:
>
>   #<buffer  *temp*>
>   #<buffer *scratch*>
>
> This is because of this line recently added to context-menu-map:
>
>   (select-window (posn-window (event-start click)))
>
> that switches buffers.
>
> But the question remains: does describe-map really need to build
> the complete context menu or should it ignore its :filter keyword?

Hm...  I don't know, but I don't think a call to `lookup-key' should
result in a different buffer being selected in any case.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Juri Linkov]

close 53910 29.0.50
thanks

>> This is because of this line recently added to context-menu-map:
>>
>>   (select-window (posn-window (event-start click)))
>>
>> that switches buffers.
>>
>> But the question remains: does describe-map really need to build
>> the complete context menu or should it ignore its :filter keyword?
>
> Hm...  I don't know, but I don't think a call to `lookup-key' should
> result in a different buffer being selected in any case.

It was surprising that select-window invoked on the same selected window
switches the buffer to the originally displayed window-buffer.
Maybe Martin could explain this.

Anyway, this is fixed now by not calling select-window on the already
selected window.

bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[martin rudalics]

 > It was surprising that select-window invoked on the same selected window
 > switches the buffer to the originally displayed window-buffer.
 > Maybe Martin could explain this.

Do you mean that

(with-temp-buffer
   (let ((buffer (current-buffer)))
     (select-window (selected-window))
     (message "before %S after %S" buffer (current-buffer))))

is surprising?  But 'select-window' only does

   In addition, make WINDOW’s buffer current and set its
   buffer’s value of ‘point’ to the value of WINDOW’s ‘window-point’.

as advertised.  Or do you mean something else?

martin

bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Juri Linkov]

>> It was surprising that select-window invoked on the same selected window
>> switches the buffer to the originally displayed window-buffer.
>> Maybe Martin could explain this.
>
> Do you mean that
>
> (with-temp-buffer
>   (let ((buffer (current-buffer)))
>     (select-window (selected-window))
>     (message "before %S after %S" buffer (current-buffer))))
>
> is surprising?  But 'select-window' only does
>
>   In addition, make WINDOW’s buffer current and set its
>   buffer’s value of ‘point’ to the value of WINDOW’s ‘window-point’.
>
> as advertised.  Or do you mean something else?

Yep, this is what I meant.  I expected it no-op in this case,
but the documented behavior is fine.

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Drew Adams]

> > (with-temp-buffer
> >   (let ((buffer (current-buffer)))
> >     (select-window (selected-window))
> >     (message "before %S after %S" buffer (current-buffer))))
> >
> > is surprising?  But 'select-window' only does
> >
> >   In addition, make WINDOW’s buffer current and set its
> >   buffer’s value of ‘point’ to the value of WINDOW’s ‘window-point’.
> >
> > as advertised.  Or do you mean something else?
> 
> Yep, this is what I meant.  I expected it no-op
> in this case, but the documented behavior is fine.

Maybe it would help to draw some more attention
to this in the doc somehow?

I don't find this obvious at all, even if the doc
does specify it.  The function names don't give
the impression that the behavior you speak of is
part of the what the functions do.

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Juri Linkov]

>> > But 'select-window' only does
>> >
>> >   In addition, make WINDOW’s buffer current and set its
>> >   buffer’s value of ‘point’ to the value of WINDOW’s ‘window-point’.
>> >
>> > as advertised.  Or do you mean something else?
>>
>> Yep, this is what I meant.  I expected it no-op
>> in this case, but the documented behavior is fine.
>
> Maybe it would help to draw some more attention
> to this in the doc somehow?
>
> I don't find this obvious at all, even if the doc
> does specify it.  The function names don't give
> the impression that the behavior you speak of is
> part of the what the functions do.

I don't know, maybe the docstring could warn about this case too.

What worries me more is that the following idiom is not always safe:

  (with-selected-window (or window (selected-window))
    body
    ...)

because it might switch the buffer of the already selected window.
This idiom is used to prevent duplicating body in e.g.:

  (if window
      (with-selected-window window
        body
        ...)
    ;; Else call `body' in the selected window:
    body
    ...)

To avoid this problem, maybe the macro `with-selected-window'
could select the window only if it is non-nil, like this:

diff --git a/lisp/subr.el b/lisp/subr.el
index a78af09c40..2e528f5c8c 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4224,7 +4224,8 @@ with-selected-window
           (internal--before-with-selected-window ,window)))
      (save-current-buffer
        (unwind-protect
-           (progn (select-window (car save-selected-window--state) 'norecord)
+           (progn (when (car save-selected-window--state)
+                    (select-window (car save-selected-window--state) 'norecord))
 		  ,@body)
          (internal--after-with-selected-window save-selected-window--state)))))

Then this could be used even when 'window' is nil:

  (with-selected-window window
    body
    ...)

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[martin rudalics]

 > What worries me more is that the following idiom is not always safe:
 >
 >    (with-selected-window (or window (selected-window))
 >      body
 >      ...)
 >
 > because it might switch the buffer of the already selected window.

IIUC you mean that it might make another buffer current?  But the whole
idea of selecting a window is that it gets you in a consistent state
where, for example, the window is the selected window of its frame which
is also the selected frame and point returns the position of point of
the selected window.  Everything else is of evil (on the Lisp level).

I don't know why you need that idiom in tab-line.el but, for example,
the completely misguided

(defun window-safely-shrinkable-p (&optional window)
   "Return t if WINDOW can be shrunk without shrinking other windows.
WINDOW defaults to the selected window."
   (with-selected-window (or window (selected-window))
     (let ((edges (window-edges)))
       (or (= (nth 2 edges) (nth 2 (window-edges (previous-window))))
	  (= (nth 0 edges) (nth 0 (window-edges (next-window))))))))

should be written as

(defun window-safely-shrinkable-p (&optional window)
   "Return t if WINDOW can be shrunk without shrinking other windows.
WINDOW defaults to the selected window."
   (let ((edges (window-edges window)))
     (or (= (nth 2 edges) (nth 2 (window-edges (previous-window window))))
	(= (nth 0 edges) (nth 0 (window-edges (next-window window)))))))

So I'd urge you to rewrite the tab-line functions in a more safe way.

martin

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Juri Linkov]

>> What worries me more is that the following idiom is not always safe:
>>
>>    (with-selected-window (or window (selected-window))
>>      body
>>      ...)
>>
>> because it might switch the buffer of the already selected window.
>
> IIUC you mean that it might make another buffer current?  But the whole
> idea of selecting a window is that it gets you in a consistent state
> where, for example, the window is the selected window of its frame which
> is also the selected frame and point returns the position of point of
> the selected window.  Everything else is of evil (on the Lisp level).

Indeed, what describe-bindings does by calling context-menu after
switching buffers is evil.  Here is another attempt to prevent this:

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 4c0d455808..1ad6fd089b 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -541,7 +541,9 @@ context-menu-ffap
 
 (defvar context-menu-entry
   `(menu-item ,(purecopy "Context Menu") ,(make-sparse-keymap)
-              :filter ,(lambda (_) (context-menu-map)))
+              :filter ,(lambda (_) (unless help-buffer-under-preparation
+                                     ;; No need to build menu to describe keys
+                                     (context-menu-map))))
   "Menu item that creates the context menu and can be bound to a mouse key.")

> I don't know why you need that idiom in tab-line.el but, for example,
> the completely misguided
>
> (defun window-safely-shrinkable-p (&optional window)
>   "Return t if WINDOW can be shrunk without shrinking other windows.
> WINDOW defaults to the selected window."
>   (with-selected-window (or window (selected-window))
>     (let ((edges (window-edges)))
>       (or (= (nth 2 edges) (nth 2 (window-edges (previous-window))))
> 	  (= (nth 0 edges) (nth 0 (window-edges (next-window))))))))
>
> should be written as
>
> (defun window-safely-shrinkable-p (&optional window)
>   "Return t if WINDOW can be shrunk without shrinking other windows.
> WINDOW defaults to the selected window."
>   (let ((edges (window-edges window)))
>     (or (= (nth 2 edges) (nth 2 (window-edges (previous-window window))))
> 	(= (nth 0 edges) (nth 0 (window-edges (next-window window)))))))
>
> So I'd urge you to rewrite the tab-line functions in a more safe way.

While it's possible to use the 'window' argument in all functions used
in window-safely-shrinkable-p, tab-line functions use functions that
don't accept the 'window' argument, e.g. current-buffer, kill-buffer.
But there is no problem for tab-line to select the already selected window
since it operates on windows.

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

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

> --- a/lisp/mouse.el
> +++ b/lisp/mouse.el
> @@ -541,7 +541,9 @@ context-menu-ffap
>  
>  (defvar context-menu-entry
>    `(menu-item ,(purecopy "Context Menu") ,(make-sparse-keymap)
> -              :filter ,(lambda (_) (context-menu-map)))
> +              :filter ,(lambda (_) (unless help-buffer-under-preparation
> +                                     ;; No need to build menu to describe keys
> +                                     (context-menu-map))))
>    "Menu item that creates the context menu and can be bound to a mouse key.")

FWIW, I find this hideous.  `mouse.el` should not depend on `help-*` variables.

> While it's possible to use the 'window' argument in all functions used
> in window-safely-shrinkable-p, tab-line functions use functions that
> don't accept the 'window' argument, e.g. current-buffer, kill-buffer.

`window-buffer` is the function that returns the "current" buffer of a
window.  As for `kill-buffer`, I'm not sure what window arg you'd like
to use by I suspect (kill-buffer (window-buffer <WINDOW>)) is what
you're after.


        Stefan

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Juri Linkov]

>>  (defvar context-menu-entry
>>    `(menu-item ,(purecopy "Context Menu") ,(make-sparse-keymap)
>> -              :filter ,(lambda (_) (context-menu-map)))
>> +              :filter ,(lambda (_) (unless help-buffer-under-preparation
>> +                                     ;; No need to build menu to describe keys
>> +                                     (context-menu-map))))
>>    "Menu item that creates the context menu and can be bound to a mouse key.")
>
> FWIW, I find this hideous.  `mouse.el` should not depend on `help-*` variables.

I know, but there are too many problems when help functions are trying
to build the context menu in a non-displayed buffer.  Is there another way
to prevent this?  Recently this was changed:

  -  `(menu-item ,(purecopy "Context Menu") ignore
  +  `(menu-item ,(purecopy "Context Menu") ,(make-sparse-keymap)

to prevent where-is-internal from running context-menu-map by
describe-mode in the wrong buffer, but this was not enough.

>> While it's possible to use the 'window' argument in all functions used
>> in window-safely-shrinkable-p, tab-line functions use functions that
>> don't accept the 'window' argument, e.g. current-buffer, kill-buffer.
>
> `window-buffer` is the function that returns the "current" buffer of a
> window.  As for `kill-buffer`, I'm not sure what window arg you'd like
> to use by I suspect (kill-buffer (window-buffer <WINDOW>)) is what
> you're after.

Maybe (kill-buffer (window-buffer <WINDOW>)) has the same effect
when used in any window, but (bury-buffer (window-buffer <WINDOW>))
definitely should be called in the required window, because `bury-buffer`
uses `nil` for the WINDOW args, e.g.:

      (set-window-dedicated-p nil nil)
      (switch-to-prev-buffer nil 'bury)

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

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

>>>  (defvar context-menu-entry
>>>    `(menu-item ,(purecopy "Context Menu") ,(make-sparse-keymap)
>>> -              :filter ,(lambda (_) (context-menu-map)))
>>> +              :filter ,(lambda (_) (unless help-buffer-under-preparation
>>> +                                     ;; No need to build menu to describe keys
>>> +                                     (context-menu-map))))
>>>    "Menu item that creates the context menu and can be bound to a mouse key.")
>> FWIW, I find this hideous.  `mouse.el` should not depend on `help-*` variables.
> I know, but there are too many problems when help functions are trying
> to build the context menu in a non-displayed buffer.

Those are bugs in the context-menu functions that need to be fixed
because they'll bite us sooner or later in other cases anyway.
In a sense, we should be happy to have such an easy way to trigger those
bugs ;-)

> Is there another way to prevent this?

I think a slightly cleaner way (if you want to keep such a workaround
rather than chase&fix the underlying bugs) is to move the var to
`mouse.el` and call it `inhibit-context-menu`, and then let-bind at the
appropriate place with a prominent comment explaining why we're using
such a hack.

> Maybe (kill-buffer (window-buffer <WINDOW>)) has the same effect
> when used in any window, but (bury-buffer (window-buffer <WINDOW>))
> definitely should be called in the required window,

Indeed, there are several function that need the right `selected-window`
and where you can't pass an explicit window instead, and `bury-buffer`
is one of them.


        Stefan

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Juri Linkov]

>>>>  (defvar context-menu-entry
>>>>    `(menu-item ,(purecopy "Context Menu") ,(make-sparse-keymap)
>>>> -              :filter ,(lambda (_) (context-menu-map)))
>>>> +              :filter ,(lambda (_) (unless help-buffer-under-preparation
>>>> +                                     ;; No need to build menu to describe keys
>>>> +                                     (context-menu-map))))
>>>>    "Menu item that creates the context menu and can be bound to a mouse key.")
>>> FWIW, I find this hideous.  `mouse.el` should not depend on `help-*` variables.
>> I know, but there are too many problems when help functions are trying
>> to build the context menu in a non-displayed buffer.
>
> Those are bugs in the context-menu functions that need to be fixed
> because they'll bite us sooner or later in other cases anyway.
> In a sense, we should be happy to have such an easy way to trigger those
> bugs ;-)

If `context-menu-mode` is intended to work only on displayed buffers,
is it really important to ensure that it also doesn't fail on
non-window buffers?  I think the bug is in these functions that
are trying to call `context-menu-map` in a non-window buffer,
in this case the bug in the Help functions.  Then indeed instead
of using `help-buffer-under-preparation` in `mouse.el`, maybe
it should be fixed in `describe-map` somehow to not call
`lookup-key` that creates the context menu with:

  (eq definition (lookup-key tail (vector event) t))

>> Is there another way to prevent this?
>
> I think a slightly cleaner way (if you want to keep such a workaround
> rather than chase&fix the underlying bugs) is to move the var to
> `mouse.el` and call it `inhibit-context-menu`, and then let-bind at the
> appropriate place with a prominent comment explaining why we're using
> such a hack.

Then this means using something like this in `describe-map`:

  (let ((inhibit-context-menu t))
    (eq definition (lookup-key tail (vector event) t)))

But indeed, this looks like a workaround.

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

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

> If `context-menu-mode` is intended to work only on displayed buffers,
> is it really important to ensure that it also doesn't fail on
> non-window buffers?

Yes, a :filter function in a keymap should not make such assumptions
(nor have side-effects, as a general rule).


        Stefan

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[martin rudalics]

 > Maybe (kill-buffer (window-buffer <WINDOW>)) has the same effect
 > when used in any window, but (bury-buffer (window-buffer <WINDOW>))
 > definitely should be called in the required window, because `bury-buffer`
 > uses `nil` for the WINDOW args, e.g.:
 >
 >        (set-window-dedicated-p nil nil)
 >        (switch-to-prev-buffer nil 'bury)

I recommend against calling 'bury-buffer' in Lisp code: A function that
does

   If BUFFER-OR-NAME is nil or omitted, bury the
   current buffer.  Also, if BUFFER-OR-NAME is nil or omitted,
   remove the current buffer from the selected window if it is
   displayed there.

is clearly intended for interactive use only.

martin

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Eli Zaretskii]

> Date: Sat, 19 Feb 2022 10:41:09 +0100
> From: martin rudalics <rudalics@gmx.at>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, Ergus <spacibba@aol.com>,
>  "53910@debbugs.gnu.org" <53910@debbugs.gnu.org>
> 
> I recommend against calling 'bury-buffer' in Lisp code: A function that
> does
> 
>    If BUFFER-OR-NAME is nil or omitted, bury the
>    current buffer.  Also, if BUFFER-OR-NAME is nil or omitted,
>    remove the current buffer from the selected window if it is
>    displayed there.
> 
> is clearly intended for interactive use only.

What exactly do you mean by "interactive use only"?  Several commands
invoke bury-buffer as part of their job -- or do you consider that to
be "interactive use" as well?

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

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

> I recommend against calling 'bury-buffer' in Lisp code: A function that
> does
>
>   If BUFFER-OR-NAME is nil or omitted, bury the
>   current buffer.  Also, if BUFFER-OR-NAME is nil or omitted,
>   remove the current buffer from the selected window if it is
>   displayed there.
>
> is clearly intended for interactive use only.

I think I understand why you say that but the interactive spec only
allows passing nil as argument, so maybe calling `bury-buffer` with
a nil argument should not be done from Elisp, but if calling
`bury-buffer` *in general* should not be done for Elisp, then we can
simplify the docstring ;-)


        Stefan

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[martin rudalics]

 > What exactly do you mean by "interactive use only"?  Several commands
 > invoke bury-buffer as part of their job -- or do you consider that to
 > be "interactive use" as well?

No.  Consider the following two forms:

(defun foo (&optional buffer)
   (interactive)
   (let ((buffer (window-normalize-buffer buffer)))
     (if (or (and (window-next-sibling)
		 (eq (window-buffer (window-next-sibling)) buffer))
	    (and (window-prev-sibling)
		 (eq (window-buffer (window-prev-sibling)) buffer)))
	(bury-buffer buffer)
       (kill-buffer buffer))))

(defun foo (&optional buffer)
   (interactive)
   (if (let ((buffer (window-normalize-buffer buffer)))
	(or (and (window-next-sibling)
		 (eq (window-buffer (window-next-sibling)) buffer))
	    (and (window-prev-sibling)
		 (eq (window-buffer (window-prev-sibling)) buffer))))
       (bury-buffer buffer)
     (kill-buffer buffer)))

If, after C-x 2, I do M-x foo, the first will leave the windows' buffers
unchanged while the second will show another buffer in the selected
window.  So my conclusion is to never use 'bury-buffer' in Lisp code.

martin

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Eli Zaretskii]

> Date: Sat, 19 Feb 2022 18:07:03 +0100
> Cc: juri@linkov.net, monnier@iro.umontreal.ca, larsi@gnus.org,
>  spacibba@aol.com, 53910@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  > What exactly do you mean by "interactive use only"?  Several commands
>  > invoke bury-buffer as part of their job -- or do you consider that to
>  > be "interactive use" as well?
> 
> No.  Consider the following two forms:
> 
> (defun foo (&optional buffer)
>    (interactive)
>    (let ((buffer (window-normalize-buffer buffer)))
>      (if (or (and (window-next-sibling)
> 		 (eq (window-buffer (window-next-sibling)) buffer))
> 	    (and (window-prev-sibling)
> 		 (eq (window-buffer (window-prev-sibling)) buffer)))
> 	(bury-buffer buffer)
>        (kill-buffer buffer))))
> 
> (defun foo (&optional buffer)
>    (interactive)
>    (if (let ((buffer (window-normalize-buffer buffer)))
> 	(or (and (window-next-sibling)
> 		 (eq (window-buffer (window-next-sibling)) buffer))
> 	    (and (window-prev-sibling)
> 		 (eq (window-buffer (window-prev-sibling)) buffer))))
>        (bury-buffer buffer)
>      (kill-buffer buffer)))
> 
> If, after C-x 2, I do M-x foo, the first will leave the windows' buffers
> unchanged while the second will show another buffer in the selected
> window.  So my conclusion is to never use 'bury-buffer' in Lisp code.

That bury-buffer can be mis-used or abused doesn't mean it should not
be used correctly, especially since we do that in many places.
Moreover, bury-buffer does little besides bury-buffer-internal, so I
still don't understand the emotional reaction.

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[martin rudalics]

 > I think I understand why you say that but the interactive spec only
 > allows passing nil as argument, so maybe calling `bury-buffer` with
 > a nil argument should not be done from Elisp, but if calling
 > `bury-buffer` *in general* should not be done for Elisp, then we can
 > simplify the docstring ;-)

'bury-buffer' violates referential transparency in the sense that it
does not allow allow a caller to substitute nil for (current-buffer) and
vice versa without also changing its semantics.  This is against what
all other functions with an optional BUFFER-OR-NAME argument do and can
only lead to confusion.

martin

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[martin rudalics]

 > That bury-buffer can be mis-used or abused

Do you mean that the functions I posted mis-use or abuse 'bury-buffer'?

 > doesn't mean it should not
 > be used correctly,

What distinguishes a "correct use" of 'bury-buffer' from an "incorrect"
one in your opinion?

 > especially since we do that in many places.

Many of those assign 'bury-buffer' to a key (which is perfectly valid)
or call it with an explicit BUFFER-OR-NAME argument (which is also valid
but could avoid the extra indirection by calling 'bury-buffer-internal'
right away).  Problematic are calls without arguments, especially when
they are wrapped in 'with-selected-window' like in 'tab-line-close-tab'.

 > Moreover, bury-buffer does little besides bury-buffer-internal, so I
 > still don't understand the emotional reaction.

What's emotional about a recommendation?  My

   I recommend against calling 'bury-buffer' in Lisp code:

has to be seen in the context of Juri's

   Maybe (kill-buffer (window-buffer <WINDOW>)) has the same effect
   when used in any window, but (bury-buffer (window-buffer <WINDOW>))
   definitely should be called in the required window, because `bury-buffer`
   uses `nil` for the WINDOW args, e.g.:

	(set-window-dedicated-p nil nil)
	(switch-to-prev-buffer nil 'bury)

martin

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

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

> 'bury-buffer' violates referential transparency in the sense that it
> does not allow allow a caller to substitute nil for (current-buffer) and
> vice versa without also changing its semantics.  This is against what
> all other functions with an optional BUFFER-OR-NAME argument do and can
> only lead to confusion.

Yes, its API is weird.  We should probably split it into two separate
functions.


        Stefan

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Juri Linkov]

> Many of those assign 'bury-buffer' to a key (which is perfectly valid)
> or call it with an explicit BUFFER-OR-NAME argument (which is also valid
> but could avoid the extra indirection by calling 'bury-buffer-internal'
> right away).  Problematic are calls without arguments, especially when
> they are wrapped in 'with-selected-window' like in 'tab-line-close-tab'.

Then 'bury-buffer' could have a new optional argument WINDOW,
thus changing this line:

          (not (eq buffer (window-buffer)))

to:

          (not (eq buffer (window-buffer window)))

and using WINDOW in other places in 'bury-buffer' too.

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Juri Linkov]

>> If `context-menu-mode` is intended to work only on displayed buffers,
>> is it really important to ensure that it also doesn't fail on
>> non-window buffers?
>
> Yes, a :filter function in a keymap should not make such assumptions
> (nor have side-effects, as a general rule).

I can't find a way to prevent :filter from executing on context menus in help
buffers, so I just removed `help-buffer-under-preparation` from :filter.

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[martin rudalics]

 > Then 'bury-buffer' could have a new optional argument WINDOW,
 > thus changing this line:
 >
 >            (not (eq buffer (window-buffer)))
 >
 > to:
 >
 >            (not (eq buffer (window-buffer window)))
 >
 > and using WINDOW in other places in 'bury-buffer' too.

Even without such an argument 'bury-buffer' is complex enough so I doubt
that many people will understand what it does in all its consequences.
Can you tell how burying a buffer affects the next C-x <left> sequence
you type?

martin

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[Juri Linkov]

> Can you tell how burying a buffer affects the next C-x <left> sequence
> you type?

'bury-buffer' should remove the buffer from the C-x <left> sequence
of previous buffers.

bug#53910: [External] : bug#53910: 29.0.50; context-menu-mode breaks help in read-only buffers

[martin rudalics]

 >> Can you tell how burying a buffer affects the next C-x <left> sequence
 >> you type?
 >
 > 'bury-buffer' should remove the buffer from the C-x <left> sequence
 > of previous buffers.

Completely?  And add it to the sequence of next buffers?  We could
provide an option for that.  Personally, I'd profoundly dislike that
some application could remove a buffer from a window's list of previous
buffers by burying it.

And then we would have to decide whether 'unbury-buffer' should switch
to the last buffer in the selected window's buffer list or to the last
buffer in the selected frame's buffer list.

martin