bug#73175: 30.0.60; toggling emacs-news-view-mode doesn't remove buttons

bug#73175: 30.0.60; toggling emacs-news-view-mode doesn't remove buttons

[Robert Pluim]

src/emacs -Q etc/NEWS
C-x C-q ; buttons appear
C-x C-q ; buttons donʼt disappear

(personally I donʼt mind the buttons, but the intent of faf1f037987c
was to make them disappear).

I came up with the following, but I can never remember all the details
of text property removal (maybe we need a `button-debuttonize-region'?)

diff --git a/lisp/textmodes/emacs-news-mode.el b/lisp/textmodes/emacs-news-mode.el
index edeb1540feb..2cca63071b5 100644
--- a/lisp/textmodes/emacs-news-mode.el
+++ b/lisp/textmodes/emacs-news-mode.el
@@ -112,10 +112,17 @@ emacs-news--mode-common
 (define-derived-mode emacs-news-mode text-mode "NEWS"
   "Major mode for editing the Emacs NEWS file."
   ;; Disable buttons.
-  (button-mode nil)
+  (button-mode -1)
   ;; And make the buffer writable.  This is used when toggling
   ;; emacs-news-mode.
   (setq buffer-read-only nil)
+  (save-excursion
+    (with-silent-modifications
+      (widen)
+      (remove-text-properties (point-min) (point-max)
+                            (button--properties nil nil nil))
+      (add-face-text-property (point-min) (point-max)
+                              'button nil)))
   (setq-local fill-paragraph-function #'emacs-news--fill-paragraph)
   (emacs-news--mode-common))
 



In GNU Emacs 30.0.60 (build 4, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.16.0) of 2024-09-05 built on rltb
Repository revision: fc7581ae2ee9db1e3189b76c66be9cd13f72f004
Repository branch: emacs-30
System Description: Debian GNU/Linux 12 (bookworm)

Robert
-- 

bug#73175: 30.0.60; toggling emacs-news-view-mode doesn't remove buttons

[Juri Linkov]

> src/emacs -Q etc/NEWS
> C-x C-q ; buttons appear
> C-x C-q ; buttons donʼt disappear
>
> (personally I donʼt mind the buttons, but the intent of faf1f037987c
> was to make them disappear).
>
> I came up with the following, but I can never remember all the details
> of text property removal (maybe we need a `button-debuttonize-region'?)

Or `button-unbuttonize-region' (like in `gnus-unbuttonized-mime-types').

>  (define-derived-mode emacs-news-mode text-mode "NEWS"
>    "Major mode for editing the Emacs NEWS file."
>    ;; Disable buttons.
> -  (button-mode nil)
> +  (button-mode -1)
>    ;; And make the buffer writable.  This is used when toggling
>    ;; emacs-news-mode.
>    (setq buffer-read-only nil)
> +  (save-excursion
> +    (with-silent-modifications
> +      (widen)
> +      (remove-text-properties (point-min) (point-max)
> +                            (button--properties nil nil nil))
> +      (add-face-text-property (point-min) (point-max)
> +                              'button nil)))

Probably it would be the right thing to move this code to `button-mode'
to remove properties when it's disabled.

bug#73175: 30.0.60; toggling emacs-news-view-mode doesn't remove buttons

[Robert Pluim]

>>>>> On Fri, 20 Sep 2024 09:52:25 +0300, Juri Linkov <juri@linkov.net> said:

    >> src/emacs -Q etc/NEWS
    >> C-x C-q ; buttons appear
    >> C-x C-q ; buttons donʼt disappear
    >> 
    >> (personally I donʼt mind the buttons, but the intent of faf1f037987c
    >> was to make them disappear).
    >> 
    >> I came up with the following, but I can never remember all the details
    >> of text property removal (maybe we need a `button-debuttonize-region'?)

    Juri> Or `button-unbuttonize-region' (like in `gnus-unbuttonized-mime-types').

Sure.

    >> (define-derived-mode emacs-news-mode text-mode "NEWS"
    >> "Major mode for editing the Emacs NEWS file."
    >> ;; Disable buttons.
    >> -  (button-mode nil)
    >> +  (button-mode -1)
    >> ;; And make the buffer writable.  This is used when toggling
    >> ;; emacs-news-mode.
    >> (setq buffer-read-only nil)
    >> +  (save-excursion
    >> +    (with-silent-modifications
    >> +      (widen)
    >> +      (remove-text-properties (point-min) (point-max)
    >> +                            (button--properties nil nil nil))
    >> +      (add-face-text-property (point-min) (point-max)
    >> +                              'button nil)))

    Juri> Probably it would be the right thing to move this code to `button-mode'
    Juri> to remove properties when it's disabled.

That would be best (assuming Iʼve got the incantation right ;-))

Robert
-- 

bug#73175: 30.0.60; toggling emacs-news-view-mode doesn't remove buttons

[Robert Pluim]

>>>>> On Fri, 20 Sep 2024 10:57:53 +0200, Robert Pluim <rpluim@gmail.com> said:
    Juri> Probably it would be the right thing to move this code to `button-mode'
    Juri> to remove properties when it's disabled.

    Robert> That would be best (assuming Iʼve got the incantation right ;-))

Although `button-mode' is described as a mode for button
*navigation*, so changing its behaviour might annoy people. Weʼll let the
maintainers decide that one ☺️

Robert
-- 

bug#73175: 30.0.60; toggling emacs-news-view-mode doesn't remove buttons

[Juri Linkov]

>     Juri> Probably it would be the right thing to move this code to `button-mode'
>     Juri> to remove properties when it's disabled.
>
>     Robert> That would be best (assuming Iʼve got the incantation right ;-))
>
> Although `button-mode' is described as a mode for button
> *navigation*, so changing its behaviour might annoy people. Weʼll let the
> maintainers decide that one ☺️

The change will fix its behaviour only for disabling with
(button-mode -1).  I don't see how this might annoy people.
It's the standard thing for a mode to clean up after itself.

bug#73175: 30.0.60; toggling emacs-news-view-mode doesn't remove buttons

[Robert Pluim]

>>>>> On Tue, 24 Sep 2024 09:39:53 +0300, Juri Linkov <juri@linkov.net> said:

    Juri> Probably it would be the right thing to move this code to `button-mode'
    Juri> to remove properties when it's disabled.
    >> 
    Robert> That would be best (assuming Iʼve got the incantation right ;-))
    >> 
    >> Although `button-mode' is described as a mode for button
    >> *navigation*, so changing its behaviour might annoy people. Weʼll let the
    >> maintainers decide that one ☺️

    Juri> The change will fix its behaviour only for disabling with
    Juri> (button-mode -1).  I don't see how this might annoy people.
    Juri> It's the standard thing for a mode to clean up after itself.

OK. Are we agreed that `unbuttonize-region' is a good name before I
update the manual?

(Hmm, `buttonize-region' isnʼt in the elisp manual. Iʼm assuming
emacs-30 is still open for documentation changes, if not I can fix
that on master.)

Robert
-- 

bug#73175: 30.0.60; toggling emacs-news-view-mode doesn't remove buttons

[Eli Zaretskii]

> Cc: 73175@debbugs.gnu.org
> From: Robert Pluim <rpluim@gmail.com>
> Date: Tue, 24 Sep 2024 11:29:45 +0200
> 
> Hmm, `buttonize-region' isnʼt in the elisp manual. Iʼm assuming
> emacs-30 is still open for documentation changes

Yes, thanks.

bug#73175: 30.0.60; toggling emacs-news-view-mode doesn't remove buttons

[Robert Pluim]

>>>>> On Tue, 24 Sep 2024 15:25:14 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> Cc: 73175@debbugs.gnu.org
    >> From: Robert Pluim <rpluim@gmail.com>
    >> Date: Tue, 24 Sep 2024 11:29:45 +0200
    >> 
    >> Hmm, `buttonize-region' isnʼt in the elisp manual. Iʼm assuming
    >> emacs-30 is still open for documentation changes

    Eli> Yes, thanks.

OK. Done.

Iʼve just remembered that button.el can use either overlays or text
properties to make buttons, so `unbuttonize-region' should look like
this:

(defun unbuttonize-region (start end)
  "Remove all the buttons between START and END.
This removes both text-property and overlay based buttons."
  (dolist (o (overlays-in start end))
    (when (overlay-get o 'button)
      (delete-overlay o)))
  (with-silent-modifications
    (remove-text-properties start end
                            (button--properties nil nil nil))
    (add-face-text-property start end
                            'button nil)))


Robert
--