Re: master f51f963: Fix some side-effecting uses of make-text-button

[Paul Eggert <eggert@cs.ucla.edu>]

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

On 6/5/20 6:02 AM, Stefan Monnier wrote:
> So, IIUC `make-text-button` should ideally work functionally, but for
> historical reasons it works by side-effect.  What's the long term plan?
> Do we plan to live with the current side-effecting behavior, or do we
> plan to move to the "pure" functional behavior?

Given the comments in this thread it seems that there's consensus that it should
move to the "pure" functional behavior, as the side-effecting behavior is
confusing (and this is independent of whether string literals are constant). I
installed the attached patch, which is along the lines that I proposed a couple
of days ago; it has the doc fix that Pip Cet suggested.

[-- Attachment #2: 0001-make-text-button-no-longer-modifies-its-string-arg.patch --]
[-- Type: text/x-patch, Size: 3787 bytes --]

From 481e57cd310177452fe5d4e733370337eb35ba96 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 6 Jun 2020 12:05:10 -0700
Subject: [PATCH] make-text-button no longer modifies its string arg
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* etc/NEWS: Mention this.
* lisp/apropos.el (apropos-library-button):
* lisp/ibuf-ext.el (ibuffer-old-saved-filters-warning):
There’s no longer a need copy make-text-button’s string arg.
* lisp/button.el (make-text-button): Return a copy of a string arg.
Delay making the copy until after error-checking.
---
 etc/NEWS         | 5 +++++
 lisp/apropos.el  | 2 +-
 lisp/button.el   | 9 ++++++---
 lisp/ibuf-ext.el | 2 +-
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 27e511047e..edad5b37d6 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -476,6 +476,11 @@ are 'eq'.  To compare contents, use 'compare-window-configurations'
 instead.  This change helps fix a bug in 'sxhash-equal', which returned
 incorrect hashes for window configurations and some other objects.
 
+** When its first argument is a string, 'make-text-button' no longer
+modifies the string's text properties; instead, it uses and returns
+a copy of the string.  This helps avoid trouble when strings are
+shared or constants.
+
 ---
 ** The obsolete function 'thread-alive-p' has been removed.
 
diff --git a/lisp/apropos.el b/lisp/apropos.el
index 22866cd2cc..2566d44dfc 100644
--- a/lisp/apropos.el
+++ b/lisp/apropos.el
@@ -661,7 +661,7 @@ apropos
 (defun apropos-library-button (sym)
   (if (null sym)
       "<nothing>"
-    (let ((name (copy-sequence (symbol-name sym))))
+    (let ((name (symbol-name sym)))
       (make-text-button name nil
                         'type 'apropos-library
                         'face 'apropos-symbol
diff --git a/lisp/button.el b/lisp/button.el
index 3a6a6de774..d9c36a0375 100644
--- a/lisp/button.el
+++ b/lisp/button.el
@@ -341,15 +341,14 @@ make-text-button
 as the argument for the `action' callback function instead of the
 default argument, which is the button itself.
 
-BEG can also be a string, in which case it is made into a button.
+BEG can also be a string, in which case a copy of it is made into
+a button and returned.
 
 Also see `insert-text-button'."
   (let ((object nil)
         (type-entry
 	 (or (plist-member properties 'type)
 	     (plist-member properties :type))))
-    (when (stringp beg)
-      (setq object beg beg 0 end (length object)))
     ;; Disallow setting the `category' property directly.
     (when (plist-get properties 'category)
       (error "Button `category' property may not be set directly"))
@@ -362,6 +361,10 @@ make-text-button
       (setcar type-entry 'category)
       (setcar (cdr type-entry)
               (button-category-symbol (cadr type-entry))))
+    (when (stringp beg)
+      (setq object (copy-sequence beg))
+      (setq beg 0)
+      (setq end (length object)))
     ;; Now add all the text properties at once.
     (add-text-properties beg end
                          ;; Each button should have a non-eq `button'
diff --git a/lisp/ibuf-ext.el b/lisp/ibuf-ext.el
index c39000b488..bfb9787a96 100644
--- a/lisp/ibuf-ext.el
+++ b/lisp/ibuf-ext.el
@@ -202,7 +202,7 @@ ibuffer-old-saved-filters-warning
 You can save the current value through the customize system by
 either clicking or hitting return "
           (make-text-button
-           (copy-sequence "here") nil
+           "here" nil
            'face '(:weight bold :inherit button)
            'mouse-face '(:weight normal :background "gray50" :inherit button)
            'follow-link t
-- 
2.17.1