bug#61098: 29.0.60; Confusing behavior of show-paren-mode

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

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

Dear Emacs maintainers,
   After updating to Emacs 29, I saw in NEWS that show-paren-mode
   is no longer enabled in special-mode buffers. I think that is a
   welcome change. However in some cases (like reading the elisp
   manual) it is helpful to have it enabled. My impression after
   reading the NEWS entry, the manual entry for show-paren-mode
   and documentation for show-paren-local-mode was that I should
   be able to able to turn on show-paren-mode buffer locally by

            M-x show-paren-local-mode RET

    However this doesn't work and after much confusion I realized
    that calling show-paren-local-mode has effect only if I customize
    show-paren-predicate. My (incorrect) impression after reading
    the documentation was that show-paren-predicate determines only
    whether show-paren-mode is enabled in a buffer by default but
    doesn't prohibit the possibility of turning it off.

    I think a better default would be the equivalent of customizing
    show-buffer-predicate to t and then using

    (add-hook 'special-mode-hook
          (defun my-turn-off-show-paren () (show-paren-local-mode -1)))

    which is what I have added to my config. This was the result I was
    expecting from the NEWS and manual entry and I think those should
    be clarified.


In GNU Emacs 29.0.60 (build 1, x86_64-suse-linux-gnu, GTK+ Version
 3.24.35, cairo version 1.17.6) of 2023-01-22 built on sheep83
System Description: openSUSE Tumbleweed

Configured using:
 'configure --host=x86_64-suse-linux-gnu --build=x86_64-suse-linux-gnu
 --program-prefix= --disable-dependency-tracking --prefix=/usr
 --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin
 --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include
 --libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var
 --sharedstatedir=/var/lib --mandir=/usr/share/man
 --infodir=/usr/share/info --with-pgtk --with-native-compilation=aot
 --with-cairo --with-libotf --with-jpeg --with-tiff --with-gif
 --with-png --with-rsvg --with-xft --with-xml2 --with-dbus --with-sound
 --with-json --with-mailutils --with-gnutls --with-tree-sitter
 --enable-locallisppath=/usr/share/emacs/29.0.60/site-lisp:/usr/share/emacs/site-lisp
 'CFLAGS=-O2 -Wall -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3
 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables
 -fstack-clash-protection -Werror=return-type -flto=auto'
 LDFLAGS=-Wl,-O2'

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

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

Major mode: mu4e:main

Minor modes in effect:
  delete-selection-mode: t
  global-evil-surround-mode: t
  evil-surround-mode: t
  eros-mode: t
  save-place-mode: t
  global-so-long-mode: t
  projectile-mode: t
  which-key-mode: t
  savehist-mode: t
  better-jumper-mode: t
  better-jumper-local-mode: t
  global-company-mode: t
  vertico-mode: t
  all-the-icons-completion-mode: t
  marginalia-mode: t
  evil-goggles-mode: t
  evil-escape-mode: t
  evil-snipe-override-mode: t
  evil-snipe-mode: t
  gcmh-mode: t
  winner-mode: t
  smartparens-global-mode: t
  ws-butler-global-mode: t
  global-undo-fu-session-mode: t
  undo-fu-mode: t
  recentf-mode: t
  mu4e-search-minor-mode: t
  global-hl-line-mode: t
  hl-line-mode: t
  mu4e-update-minor-mode: t
  mu4e-context-minor-mode: t
  dirvish-override-dired-mode: t
  server-mode: t
  solaire-global-mode: t
  solaire-mode: t
  shell-dirtrack-mode: t
  evil-mode: t
  evil-local-mode: t
  windmove-mode: t
  workroom-mode: t
  +popup-mode: t
  +modeline-global-mode: t
  +modeline-mode: t
  override-global-mode: t
  general-override-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  window-divider-mode: t
  buffer-read-only: t
  size-indication-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  abbrev-mode: t

Load-path shadows:
/home/azeem/.emacs.d/.local/straight/build-29.0.60/citar-embark/citar-embark hides /home/azeem/.emacs.d/.local/straight/build-29.0.60/citar/citar-embark

Features:
(shadow emacsbug vertico-directory consult-flymake tabify
evil-collection-vc-git vc-git evil-collection-diff-mode diff-mode
vc-dispatcher char-fold company-ispell company-dabbrev adaptive-wrap
spell-fu evil-collection-embark embark-org embark-consult embark ffap
network-stream url-http url-gw nsm url-cache url-auth gts-engine-deepl
gts-implements gts-faces gts-core guess-language ispell
evil-collection-indent jka-compr evil-collection-helpful helpful
evil-collection-imenu imenu trace evil-collection-edebug edebug
evil-collection-debug debug backtrace info-look evil-collection-info
info evil-collection-elisp-refs elisp-refs evil-collection-xref xref
help-fns radix-tree evil-collection-help descr-text use-package-lint
hide-mode-line company-yasnippet delsel auto-minor-mode disp-table
whitespace bicycle hideshow evil-embrace evil-surround embrace
expand-region text-mode-expansions the-org-mode-expansions
er-basic-expansions expand-region-core expand-region-custom eros
highlight-quoted rainbow-delimiters highlight-numbers parent-mode
display-line-numbers hl-todo evil-collection-elisp-mode elisp-mode
saveplace evil-collection-so-long so-long cursor-sensor vertico-repeat
vc-svn projectile lisp-mnt evil-collection-grep grep ibuffer-vc ibuf-ext
evil-collection-ibuffer ibuffer ibuffer-loaddefs shr-color sort
gnus-cite smiley qp mm-archive mail-extr textsec uni-scripts
idna-mapping ucs-normalize uni-confusable textsec-check
evil-collection-which-key which-key savehist better-jumper company-capf
company consult-vertico evil-collection-vertico vertico orderless
all-the-icons-completion marginalia evil-goggles pulse color
evil-easymotion evil-escape evil-snipe autorevert filenotify gcmh winner
smartparens-config smartparens-rst smartparens-markdown smartparens-text
smartparens ws-butler undo-fu-session undo-fu desktop frameset
evil-collection-consult consult compat-28 recentf tree-widget
evil-collection-mu4e mu4e mu4e-org org-element org-persist xdg org-id
org-refile avl-tree generator oc-natbib oc-csl citeproc
citeproc-itemgetters citeproc-biblatex citeproc-bibtex parsebib
ol-bibtex evil-collection-org smartparens-org org ob ob-tangle ob-ref
ob-lob ob-table ob-exp org-macro org-src ob-comint org-pcomplete
org-list org-footnote org-faces org-entities noutline outline
ob-emacs-lisp org-table ol org-keys citeproc-cite citeproc-subbibs
citeproc-sort citeproc-name citeproc-formatters citeproc-number rst
citeproc-proc citeproc-disamb citeproc-itemdata
citeproc-generic-elements citeproc-macro citeproc-choose citeproc-date
citeproc-context citeproc-prange citeproc-style citeproc-locale
citeproc-term f f-shortdoc citeproc-rt citeproc-lib citeproc-s s queue
dash bibtex oc-biblatex oc org-loaddefs find-func mu4e-main mu4e-view
gnus-art mm-uu mml2015 mm-view mml-smime smime gnutls dig gnus-sum
gnus-group gnus-undo gnus-start gnus-dbus dbus gnus-cloud nnimap nnmail
mail-source utf7 nnoo parse-time iso8601 gnus-spec gnus-int gnus-range
gnus-win evil-collection-gnus gnus nnheader range
evil-collection-calendar cal-menu calendar cal-loaddefs mu4e-headers
mu4e-compose mu4e-draft mu4e-actions smtpmail mu4e-search mu4e-lists
mu4e-bookmarks mu4e-mark mu4e-message shr pixel-fill kinsoku url-file
svg xml dom browse-url url url-proxy url-privacy url-expand url-methods
url-history url-cookie generate-lisp-file url-domsuf url-util url-parse
auth-source json map url-vars flow-fill mule-util hl-line mu4e-contacts
mu4e-update mu4e-folders mu4e-server mu4e-context mu4e-vars mu4e-helpers
mu4e-config ido message sendmail mailcap yank-media puny dirvish
transient eieio eieio-core evil-collection-dired dired dired-loaddefs
rfc822 mml mml-sec password-cache evil-collection-epa epa epg rfc6068
epg-config gnus-util time-date mm-decode mm-bodies mm-encode mail-parse
rfc2231 rfc2047 rfc2045 mm-util ietf-drums mail-prsvr mailabbrev
mail-utils gmm-utils mailheader server dtrt-indent doom-themes-ext-org
solaire-mode face-remap doom-earl-grey-theme doom-themes
doom-themes-base evil-collection-flymake flymake-proc flymake
evil-collection-compile compile html2text ob-core org-cycle org-fold
org-fold-core org-compat ob-eval org-version org-macs format-spec
ibuf-macs evil-collection-bookmark evil-collection-custom cus-edit
cus-load wid-edit evil-collection-comint evil-collection annalist evil
evil-integration evil-maps evil-commands reveal flyspell evil-jumps
evil-command-window evil-search shell pcomplete comint ansi-osc
ansi-color evil-types evil-macros evil-repeat evil-states evil-core comp
comp-cstr warnings icons advice evil-common windmove calc calc-loaddefs
calc-macs thingatpt rect evil-digraphs evil-vars ring workroom compat
compat-macs project byte-opt bookmark text-property-search all-the-icons
all-the-icons-faces data-material data-weathericons data-octicons
data-fileicons data-faicons data-alltheicons let-alist derived edmacro
kmacro use-package-bind-key bind-key rx doom-editor doom-projects
doom-ui easy-mmode doom-keybinds pp cl-extra help-mode use-package-core
bytecomp byte-compile general tex-site doom-start doom-modules cl-seq
doom doom-lib cl-macs cl-loaddefs cl-lib pcase gv jansson
dynamic-modules subr-x rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/pgtk-win
pgtk-win term/common-win pgtk-dnd tool-bar dnd fontset image regexp-opt
fringe tabulated-list replace newcomment text-mode lisp-mode prog-mode
register page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify dynamic-setting system-font-setting
font-render-setting cairo gtk pgtk multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 1014671 320177)
 (symbols 48 61381 35)
 (strings 32 396235 10485)
 (string-bytes 1 9425734)
 (vectors 16 116749)
 (vector-slots 8 3990496 136139)
 (floats 8 1721 1576)
 (intervals 56 9262 358)
 (buffers 984 40))

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

[Eli Zaretskii]

> Date: Fri, 27 Jan 2023 17:03:14 +0100
> From:  Rah Guzar via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> 
> Dear Emacs maintainers,
>    After updating to Emacs 29, I saw in NEWS that show-paren-mode
>    is no longer enabled in special-mode buffers. I think that is a
>    welcome change. However in some cases (like reading the elisp
>    manual) it is helpful to have it enabled. My impression after
>    reading the NEWS entry, the manual entry for show-paren-mode
>    and documentation for show-paren-local-mode was that I should
>    be able to able to turn on show-paren-mode buffer locally by
> 
>             M-x show-paren-local-mode RET
> 
>     However this doesn't work and after much confusion I realized
>     that calling show-paren-local-mode has effect only if I customize
>     show-paren-predicate. My (incorrect) impression after reading
>     the documentation was that show-paren-predicate determines only
>     whether show-paren-mode is enabled in a buffer by default but
>     doesn't prohibit the possibility of turning it off.

Your impression was correct: there should be no need to customize
show-paren-predicate to turn on show-paren-local-mode in a buffer that
is under a descendant of special-mode.  The patch below fixes the bug
and allows you to do what you want without messing with
show-paren-predicate.

However, there's one glitch: after applying the patch, when you invoke

   M-x show-paren-local-mode RET

in a special-mode buffer, it says the mode is _disabled_, and indeed
disables the mode.  You need to enable it by invoking the same command
again.

I'm not sure I understand the reason, but it is probably related to
the fact that we use the same show-paren-mode variable for the
localized mode, and the default value of that variable is t.

Stefan, can you help here? how do we make show-paren-local-mode be
enabled when the user explicitly invokes it?

Here's the patch I promised above:

diff --git a/lisp/paren.el b/lisp/paren.el
index b2a7962..de14de2 100644
--- a/lisp/paren.el
+++ b/lisp/paren.el
@@ -167,8 +167,11 @@ show-paren-local-mode
     (unless show-paren-mode
       (show-paren--delete-overlays))
     (kill-local-variable 'show-paren-mode))
-   ((not (default-value 'show-paren-mode))
-    ;; Locally enabled, but globally disabled.
+   ((or
+     ;; Locally enabled, but globally disabled.
+     (not (default-value 'show-paren-mode))
+     ;; Disabled by 'show-paren-predicate'.
+     (not (buffer-match-p show-paren-predicate (current-buffer))))
     (show-paren-mode 1)                ; Setup the timer.
     (setq-default show-paren-mode nil) ; But keep it globally disabled.
     )

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

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

Hi,
  Thanks for the patch, it works as you described.

However I have questions about the show-paren-predicate variable.

When I see its default value  (not (derived-mode . special-mode))
it makes me think there is some dsl involved. The customize interface
mentions Lisp Expression as a possibility but the documentation doesn't
explain what form this expression is supposed to take. This makes me
think that the default is just a pseudo sentence intended to convey
what it does and there are only three possibilities.

If the latter is correct, I think a simpler way would be to disable
the mode in the body of special-mode if the value of `show-paren-predicate`
demands it. I think this will get rid of the problem that the mode is
disabled the first time show-paren-local-mode is called.

I am using the equivalent of this which adds a hook to special-mode-hook
to achieve the outcome and I am pretty happy with it.

Thanks again,
Rahguzar


Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Fri, 27 Jan 2023 17:03:14 +0100
>> From:  Rah Guzar via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>>
>> Dear Emacs maintainers,
>>    After updating to Emacs 29, I saw in NEWS that show-paren-mode
>>    is no longer enabled in special-mode buffers. I think that is a
>>    welcome change. However in some cases (like reading the elisp
>>    manual) it is helpful to have it enabled. My impression after
>>    reading the NEWS entry, the manual entry for show-paren-mode
>>    and documentation for show-paren-local-mode was that I should
>>    be able to able to turn on show-paren-mode buffer locally by
>>
>>             M-x show-paren-local-mode RET
>>
>>     However this doesn't work and after much confusion I realized
>>     that calling show-paren-local-mode has effect only if I customize
>>     show-paren-predicate. My (incorrect) impression after reading
>>     the documentation was that show-paren-predicate determines only
>>     whether show-paren-mode is enabled in a buffer by default but
>>     doesn't prohibit the possibility of turning it off.
>
> Your impression was correct: there should be no need to customize
> show-paren-predicate to turn on show-paren-local-mode in a buffer that
> is under a descendant of special-mode.  The patch below fixes the bug
> and allows you to do what you want without messing with
> show-paren-predicate.
>
> However, there's one glitch: after applying the patch, when you invoke
>
>    M-x show-paren-local-mode RET
>
> in a special-mode buffer, it says the mode is _disabled_, and indeed
> disables the mode.  You need to enable it by invoking the same command
> again.
>
> I'm not sure I understand the reason, but it is probably related to
> the fact that we use the same show-paren-mode variable for the
> localized mode, and the default value of that variable is t.
>
> Stefan, can you help here? how do we make show-paren-local-mode be
> enabled when the user explicitly invokes it?
>
> Here's the patch I promised above:
>
> diff --git a/lisp/paren.el b/lisp/paren.el
> index b2a7962..de14de2 100644
> --- a/lisp/paren.el
> +++ b/lisp/paren.el
> @@ -167,8 +167,11 @@ show-paren-local-mode
>      (unless show-paren-mode
>        (show-paren--delete-overlays))
>      (kill-local-variable 'show-paren-mode))
> -   ((not (default-value 'show-paren-mode))
> -    ;; Locally enabled, but globally disabled.
> +   ((or
> +     ;; Locally enabled, but globally disabled.
> +     (not (default-value 'show-paren-mode))
> +     ;; Disabled by 'show-paren-predicate'.
> +     (not (buffer-match-p show-paren-predicate (current-buffer))))
>      (show-paren-mode 1)                ; Setup the timer.
>      (setq-default show-paren-mode nil) ; But keep it globally disabled.
>      )

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

[Dmitry Gutov]

On 28/01/2023 14:20, Eli Zaretskii wrote:
> Here's the patch I promised above:

Your patch modifies the part which called the show-paren-mode to set up 
the timer, but the timer is started unconditionally, so predicate check 
is inside the timer function. So it seems incorrect to me.

I suggest trying the patch below.

It makes sure the buffer-local value of show-paren-mode is kept 
buffer-local even if it matches the global one (while it's = t), so that 
the later check

   (or (local-variable-p 'show-paren-mode)
       ;; If not, check that the predicate matches.
       (buffer-match-p show-paren-predicate (current-buffer)))

works correctly.

diff --git a/lisp/paren.el b/lisp/paren.el
index b2a79624c0f..437dc057c46 100644
--- a/lisp/paren.el
+++ b/lisp/paren.el
@@ -165,8 +165,8 @@ show-paren-local-mode
    (cond
     ((eq show-paren-mode (default-value 'show-paren-mode))
      (unless show-paren-mode
-      (show-paren--delete-overlays))
-    (kill-local-variable 'show-paren-mode))
+      (show-paren--delete-overlays)
+      (kill-local-variable 'show-paren-mode)))
     ((not (default-value 'show-paren-mode))
      ;; Locally enabled, but globally disabled.
      (show-paren-mode 1)                ; Setup the timer.

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

[Eli Zaretskii]

> From: Rah Guzar <rahguzar@zohomail.eu>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 61098@debbugs.gnu.org
> Date: Mon, 30 Jan 2023 20:21:33 +0100
> 
> Hi,
>   Thanks for the patch, it works as you described.

Thanks for testing.

I'm still waiting for Stefan to help me understand why after that
patch "M-x show-paren-local-mode" _disables_ the mode rather than
enabling it.  Stefan, any wisdom?

> However I have questions about the show-paren-predicate variable.
> 
> When I see its default value  (not (derived-mode . special-mode))
> it makes me think there is some dsl involved. The customize interface
> mentions Lisp Expression as a possibility but the documentation doesn't
> explain what form this expression is supposed to take.

It should be an expression that yields a boolean value (that's why the
variable is named "predicate").  I will improve the doc string when I
install the final version of the changes.

> This makes me think that the default is just a pseudo sentence
> intended to convey what it does and there are only three
> possibilities.

No, it's a real expression in Lisp, just quoted.

> If the latter is correct, I think a simpler way would be to disable
> the mode in the body of special-mode if the value of `show-paren-predicate`
> demands it. I think this will get rid of the problem that the mode is
> disabled the first time show-paren-local-mode is called.
> 
> I am using the equivalent of this which adds a hook to special-mode-hook
> to achieve the outcome and I am pretty happy with it.

I don't think I understand what you have in mind, but let's wait for
Stefan to chime in.

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

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

Eli Zaretskii <eliz@gnu.org> writes:

> It should be an expression that yields a boolean value (that's why the
> variable is named "predicate").  I will improve the doc string when I
> install the final version of the changes.

What I had been missing was how is that expression supposed to yield
a boolean value. I think I gleaned the answer from Dmitry Gutov's message
which is that the expression corresponds to the "condition"
argument of buffer-match-p.

Rahguzar

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

[Eli Zaretskii]

> From: Rah Guzar <rahguzar@zohomail.eu>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 61098@debbugs.gnu.org
> Date: Tue, 31 Jan 2023 14:03:20 +0100
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It should be an expression that yields a boolean value (that's why the
> > variable is named "predicate").  I will improve the doc string when I
> > install the final version of the changes.
> 
> What I had been missing was how is that expression supposed to yield
> a boolean value.

The function 'not' returns a boolean value.

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

[Dmitry Gutov]

On 31/01/2023 16:13, Eli Zaretskii wrote:
>> From: Rah Guzar<rahguzar@zohomail.eu>
>> Cc: Stefan Monnier<monnier@iro.umontreal.ca>,61098@debbugs.gnu.org
>> Date: Tue, 31 Jan 2023 14:03:20 +0100
>>
>>
>> Eli Zaretskii<eliz@gnu.org>  writes:
>>
>>> It should be an expression that yields a boolean value (that's why the
>>> variable is named "predicate").  I will improve the doc string when I
>>> install the final version of the changes.
>> What I had been missing was how is that expression supposed to yield
>> a boolean value.
> The function 'not' returns a boolean value.

'not' is just a symbol in this case -- from a DSL.

(buffer-match-p show-paren-predicate (current-buffer)) is supposed to 
return non-nil.

See its docstring for the description of the DSL format.

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

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

> I'm still waiting for Stefan to help me understand why after that
> patch "M-x show-paren-local-mode" _disables_ the mode rather than
> enabling it.  Stefan, any wisdom?

Not sure.  I don't quite understand how your patch intends to work.
From where I sit I get the impression that it will result in an
inconsistent state in various "corner" cases.

Dmitry's patch seems closer to my intuition (I might even suggest
removing the `kill-local-variable` altogether).


        Stefan

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Rah Guzar <rahguzar@zohomail.eu>,  61098@debbugs.gnu.org
> Date: Tue, 31 Jan 2023 17:42:09 -0500
> 
> > I'm still waiting for Stefan to help me understand why after that
> > patch "M-x show-paren-local-mode" _disables_ the mode rather than
> > enabling it.  Stefan, any wisdom?
> 
> Not sure.  I don't quite understand how your patch intends to work.
> >From where I sit I get the impression that it will result in an
> inconsistent state in various "corner" cases.
> 
> Dmitry's patch seems closer to my intuition (I might even suggest
> removing the `kill-local-variable` altogether).

I guess I don't understand what show-paren-local-mode does, then.  Let
me explain my line of reasoning:

  (define-minor-mode show-paren-local-mode
    "Toggle `show-paren-mode' only in this buffer."
    :variable ( show-paren-mode .
		(lambda (val) (setq-local show-paren-mode val)))
    (cond
     ((eq show-paren-mode (default-value 'show-paren-mode))
      (unless show-paren-mode
	(show-paren--delete-overlays))
      (kill-local-variable 'show-paren-mode))
     ((not (default-value 'show-paren-mode))
      ;; Locally enabled, but globally disabled.
      (show-paren-mode 1)                ; Setup the timer.
      (setq-default show-paren-mode nil) ; But keep it globally disabled.
      )
     (t ;; Locally disabled only.
      (show-paren--delete-overlays))))

When I looked at the above function and stepped through it in the OP's
scenario, what I saw was that the 2nd condition of 'cond' doesn't
fire, because the default-value of show-paren-mode is t nowadays.  So
I concluded that this code relies on the mode being globally off by
default, and attempted to fix the condition that turns the mode on
locally even though it is on globally.

The predicate used by the timer requires either a match of the buffer
(which doesn't happen in this case) or that the local value of
show-paren-mode is non-nil.  Setting the local value non-nil is the
job of the mode function, isn't it?  So I tried to get the mode
function do its job in this scenario.

What Dmitry's patch does, and why it is supposed to fix this, I simply
don't understand, perhaps for the same reason that I misinterpret what
the mode function does.

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

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

> I guess I don't understand what show-paren-local-mode does, then.  Let
> me explain my line of reasoning:
>
>   (define-minor-mode show-paren-local-mode
>     "Toggle `show-paren-mode' only in this buffer."
>     :variable ( show-paren-mode .
> 		(lambda (val) (setq-local show-paren-mode val)))
>     (cond
>      ((eq show-paren-mode (default-value 'show-paren-mode))
>       (unless show-paren-mode
> 	(show-paren--delete-overlays))
>       (kill-local-variable 'show-paren-mode))
>      ((not (default-value 'show-paren-mode))
>       ;; Locally enabled, but globally disabled.
>       (show-paren-mode 1)                ; Setup the timer.
>       (setq-default show-paren-mode nil) ; But keep it globally disabled.
>       )
>      (t ;; Locally disabled only.
>       (show-paren--delete-overlays))))
>
> When I looked at the above function and stepped through it in the OP's
> scenario, what I saw was that the 2nd condition of 'cond' doesn't
> fire, because the default-value of show-paren-mode is t nowadays.  So
> I concluded that this code relies on the mode being globally off by
> default, and attempted to fix the condition that turns the mode on
> locally even though it is on globally.

But if the mode is already globally enabled, the (setq-default show-paren-mode nil)
will definitely not be right: you'd end up disabling it globally (while
maybe keeping it enabled locally).

> The predicate used by the timer requires either a match of the buffer
> (which doesn't happen in this case) or that the local value of
> show-paren-mode is non-nil.  Setting the local value non-nil is the
> job of the mode function, isn't it?

Yes, but the first branch of the `cond` incorrectly does
`kill-local-variable` under the assumption that the local value doesn't
matter if it's the same as the global one, but that's not true here
because (as a wise man said):

   The predicate used by the timer requires either a match of the buffer
   (which doesn't happen in this case) or that the local value of
   show-paren-mode is non-nil.


-- Stefan

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rahguzar@zohomail.eu,  61098@debbugs.gnu.org,  dgutov@yandex.ru
> Date: Sun, 05 Feb 2023 10:19:24 -0500
> 
> > I guess I don't understand what show-paren-local-mode does, then.  Let
> > me explain my line of reasoning:
> >
> >   (define-minor-mode show-paren-local-mode
> >     "Toggle `show-paren-mode' only in this buffer."
> >     :variable ( show-paren-mode .
> > 		(lambda (val) (setq-local show-paren-mode val)))
> >     (cond
> >      ((eq show-paren-mode (default-value 'show-paren-mode))
> >       (unless show-paren-mode
> > 	(show-paren--delete-overlays))
> >       (kill-local-variable 'show-paren-mode))
> >      ((not (default-value 'show-paren-mode))
> >       ;; Locally enabled, but globally disabled.
> >       (show-paren-mode 1)                ; Setup the timer.
> >       (setq-default show-paren-mode nil) ; But keep it globally disabled.
> >       )
> >      (t ;; Locally disabled only.
> >       (show-paren--delete-overlays))))
> >
> > When I looked at the above function and stepped through it in the OP's
> > scenario, what I saw was that the 2nd condition of 'cond' doesn't
> > fire, because the default-value of show-paren-mode is t nowadays.  So
> > I concluded that this code relies on the mode being globally off by
> > default, and attempted to fix the condition that turns the mode on
> > locally even though it is on globally.
> 
> But if the mode is already globally enabled, the (setq-default show-paren-mode nil)
> will definitely not be right: you'd end up disabling it globally (while
> maybe keeping it enabled locally).
> 
> > The predicate used by the timer requires either a match of the buffer
> > (which doesn't happen in this case) or that the local value of
> > show-paren-mode is non-nil.  Setting the local value non-nil is the
> > job of the mode function, isn't it?
> 
> Yes, but the first branch of the `cond` incorrectly does
> `kill-local-variable` under the assumption that the local value doesn't
> matter if it's the same as the global one, but that's not true here
> because (as a wise man said):
> 
>    The predicate used by the timer requires either a match of the buffer
>    (which doesn't happen in this case) or that the local value of
>    show-paren-mode is non-nil.

This answers only half of my questions, but if you and Dmitry think
Dmitry's patch will solve this problem, then Dmitry, please install it
and close this bug.

Thanks.

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

[Dmitry Gutov]

On 05/02/2023 17:24, Eli Zaretskii wrote:
>> From: Stefan Monnier<monnier@iro.umontreal.ca>
>> Cc:rahguzar@zohomail.eu,61098@debbugs.gnu.org,dgutov@yandex.ru
>> Date: Sun, 05 Feb 2023 10:19:24 -0500
>>
>>> I guess I don't understand what show-paren-local-mode does, then.  Let
>>> me explain my line of reasoning:
>>>
>>>    (define-minor-mode show-paren-local-mode
>>>      "Toggle `show-paren-mode' only in this buffer."
>>>      :variable ( show-paren-mode .
>>> 		(lambda (val) (setq-local show-paren-mode val)))
>>>      (cond
>>>       ((eq show-paren-mode (default-value 'show-paren-mode))
>>>        (unless show-paren-mode
>>> 	(show-paren--delete-overlays))
>>>        (kill-local-variable 'show-paren-mode))
>>>       ((not (default-value 'show-paren-mode))
>>>        ;; Locally enabled, but globally disabled.
>>>        (show-paren-mode 1)                ; Setup the timer.
>>>        (setq-default show-paren-mode nil) ; But keep it globally disabled.
>>>        )
>>>       (t ;; Locally disabled only.
>>>        (show-paren--delete-overlays))))
>>>
>>> When I looked at the above function and stepped through it in the OP's
>>> scenario, what I saw was that the 2nd condition of 'cond' doesn't
>>> fire, because the default-value of show-paren-mode is t nowadays.  So
>>> I concluded that this code relies on the mode being globally off by
>>> default, and attempted to fix the condition that turns the mode on
>>> locally even though it is on globally.
>> But if the mode is already globally enabled, the (setq-default show-paren-mode nil)
>> will definitely not be right: you'd end up disabling it globally (while
>> maybe keeping it enabled locally).
>>
>>> The predicate used by the timer requires either a match of the buffer
>>> (which doesn't happen in this case) or that the local value of
>>> show-paren-mode is non-nil.  Setting the local value non-nil is the
>>> job of the mode function, isn't it?
>> Yes, but the first branch of the `cond` incorrectly does
>> `kill-local-variable` under the assumption that the local value doesn't
>> matter if it's the same as the global one, but that's not true here
>> because (as a wise man said):
>>
>>     The predicate used by the timer requires either a match of the buffer
>>     (which doesn't happen in this case) or that the local value of
>>     show-paren-mode is non-nil.
> This answers only half of my questions, but if you and Dmitry think
> Dmitry's patch will solve this problem, then Dmitry, please install it
> and close this bug.

Installed.

Some testing revealed a certain issue, which until now was not really a 
problem:

The first time I 'M-x show-paren-local-mode' with show-paren-mode 
enabled globally, it _turns off_ the mode locally. Which kind of makes 
sense for a lot of cases, but not when the user's intention is to 
forcibly enable it.

Evaluating (show-paren-local-mode 1) still does the intended thing, though.

Just as well as (add-hook 'some-special-mode #'show-paren-local-mode).

Toggling 'M-x show-paren-local-mdoe' twice also does that.

If we do consider this a problem, a further fix could look like this:

diff --git a/lisp/paren.el b/lisp/paren.el
index 7ee4e9ae682..f2756db6cd9 100644
--- a/lisp/paren.el
+++ b/lisp/paren.el
@@ -161,7 +161,9 @@ show-paren--delete-overlays
  ;;;###autoload
  (define-minor-mode show-paren-local-mode
    "Toggle `show-paren-mode' only in this buffer."
-  :variable ( show-paren-mode .
+  :variable ( (and show-paren-mode
+                   (buffer-match-p show-paren-predicate (current-buffer)))
+              .
                (lambda (val) (setq-local show-paren-mode val)))
    (cond
     ((eq show-paren-mode (default-value 'show-paren-mode))

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

[Eli Zaretskii]

> Date: Sun, 5 Feb 2023 19:21:29 +0200
> Cc: rahguzar@zohomail.eu, 61098@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> Some testing revealed a certain issue, which until now was not really a 
> problem:
> 
> The first time I 'M-x show-paren-local-mode' with show-paren-mode 
> enabled globally, it _turns off_ the mode locally.

That's exactly what I asked about in

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=61098#8

> Which kind of makes sense for a lot of cases, but not when the
> user's intention is to forcibly enable it.

In a buffer where the mode is turned off, either effectively by virtue
of the predicate, or explicitly via the local value of the variable,
"M-x show-paren-local-mode" should turn the mode ON, not OFF.

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

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

> +  :variable ( (and show-paren-mode
> +                   (buffer-match-p show-paren-predicate (current-buffer)))
> +              .
>                (lambda (val) (setq-local show-paren-mode val)))

This doesn't look quote right because it doesn't pay attention to
`local-variable-p`.  We should extract the test performed in the timer
and reuse it here.


        Stefan

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

[Dmitry Gutov]

On 05/02/2023 20:29, Eli Zaretskii wrote:
>> Date: Sun, 5 Feb 2023 19:21:29 +0200
>> Cc:rahguzar@zohomail.eu,61098@debbugs.gnu.org
>> From: Dmitry Gutov<dgutov@yandex.ru>
>>
>> Some testing revealed a certain issue, which until now was not really a
>> problem:
>>
>> The first time I 'M-x show-paren-local-mode' with show-paren-mode
>> enabled globally, it_turns off_  the mode locally.
> That's exactly what I asked about in
> 
>    https://debbugs.gnu.org/cgi/bugreport.cgi?bug=61098#8
> 
>> Which kind of makes sense for a lot of cases, but not when the
>> user's intention is to forcibly enable it.
> In a buffer where the mode is turned off, either effectively by virtue
> of the predicate, or explicitly via the local value of the variable,
> "M-x show-paren-local-mode" should turn the mode ON, not OFF.

All right.

bug#61098: 29.0.60; Confusing behavior of show-paren-mode

[Dmitry Gutov]

On 05/02/2023 20:57, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
>> +  :variable ( (and show-paren-mode
>> +                   (buffer-match-p show-paren-predicate (current-buffer)))
>> +              .
>>                 (lambda (val) (setq-local show-paren-mode val)))
> This doesn't look quote right because it doesn't pay attention to
> `local-variable-p`.  We should extract the test performed in the timer
> and reuse it here.

Thank you, makes sense.

Here's the patch I installed:

diff --git a/lisp/paren.el b/lisp/paren.el
index 7ee4e9ae682..4c91fd29490 100644
--- a/lisp/paren.el
+++ b/lisp/paren.el
@@ -161,8 +161,9 @@ show-paren--delete-overlays
  ;;;###autoload
  (define-minor-mode show-paren-local-mode
    "Toggle `show-paren-mode' only in this buffer."
-  :variable ( show-paren-mode .
-              (lambda (val) (setq-local show-paren-mode val)))
+  :variable ((show-paren--enabled-p)
+             .
+             (lambda (val) (setq-local show-paren-mode val)))
    (cond
     ((eq show-paren-mode (default-value 'show-paren-mode))
      (unless show-paren-mode
@@ -428,14 +429,17 @@ show-paren--show-context-in-overlay
  ;; `show-paren-delay'.
  (defvar-local show-paren--last-pos nil)

+(defun show-paren--enabled-p ()
+  (and show-paren-mode
+       ;; If we're using `show-paren-local-mode', then
+       ;; always heed the value.
+       (or (local-variable-p 'show-paren-mode)
+           ;; If not, check that the predicate matches.
+           (buffer-match-p show-paren-predicate (current-buffer)))))
+
  (defun show-paren-function ()
    "Highlight the parentheses until the next input arrives."
-  (let ((data (and show-paren-mode
-                   ;; If we're using `show-paren-local-mode', then
-                   ;; always heed the value.
-                   (or (local-variable-p 'show-paren-mode)
-                       ;; If not, check that the predicate matches.
-                       (buffer-match-p show-paren-predicate 
(current-buffer)))
+  (let ((data (and (show-paren--enabled-p)
                     (funcall show-paren-data-function))))
      (if (not data)
          (progn