bug#25482: 26.0.50; Allow setting `query-replace-from-to-separator` to nil

[Thierry Volpiatto]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Thierry Volpiatto <thierry.volpiatto@gmail.com>
>> Cc: 25482@debbugs.gnu.org
>> Date: Sun, 22 Jan 2017 21:14:03 +0100
>> 
>> What would be much more simple would be to not use at all properties,
>> the actual implementation is too complex for something quite simple.
>
> But why is it a problem in the first place?

The problem is that you endup in the history with a crap string, see
https://github.com/emacs-helm/helm/issues/1667.

> In any case, some change is still needed, I believe, because you said
> customization of this is not easy.  Can you show a simple recipe that
> demonstrates the problems with customizing this option?

I think just looking at the defcustom using a sexp which need delay in
the defcustom to ensure char-displayable-p is ready to use and the need
to reevaluate this defcustom at each call of query-replace-read-from
explain all.

This patch don't change the actual behavior, I have removed the fallback
to emacs-24.5 behavior, so now setting query-replace-from-to-separator
to nil just fallback to " -> ".
With this patch the whole string is added to history.
Also usage of setq is possible, with actual implementation it is not possible.
Finally the implementation is much simpler not relaying on props.

I will not have the time for further comments or modifications of patch,
I leave it here for memory, if you want to merge it let me know.

Thanks.

1 file changed, 28 insertions(+), 35 deletions(-)
lisp/replace.el | 63 +++++++++++++++++++++++++--------------------------------

modified   lisp/replace.el
@@ -79,15 +79,12 @@ That becomes the \"string to replace\".")
 to the minibuffer that reads the string to replace, or invoke replacements
 from Isearch by using a key sequence like `C-s C-s M-%'." "24.3")
 
-(defcustom query-replace-from-to-separator
-  (propertize (if (char-displayable-p ?→) " → " " -> ")
-              'face 'minibuffer-prompt)
-  "String that separates FROM and TO in the history of replacement pairs."
-  ;; Avoids error when attempt to autoload char-displayable-p fails
-  ;; while preparing to dump, also stops customize-rogue listing this.
-  :initialize 'custom-initialize-delay
+(defcustom query-replace-from-to-separator " → "
+  "String that separates FROM and TO in the history of replacement pairs.
+When nil or the string provided not displayable the default separator \" -> \"
+will be used instead."
   :group 'matching
-  :type '(choice string (sexp :tag "Display specification"))
+  :type '(choice string)
   :version "25.1")
 
 (defcustom query-replace-from-history-variable 'query-replace-history
@@ -150,14 +147,12 @@ See `replace-regexp' and `query-replace-regexp-eval'.")
   (mapconcat 'isearch-text-char-description string ""))
 
 (defun query-replace--split-string (string)
-  "Split string STRING at a character with property `separator'"
-  (let* ((length (length string))
-         (split-pos (text-property-any 0 length 'separator t string)))
-    (if (not split-pos)
-        (substring-no-properties string)
-      (cl-assert (not (text-property-any (1+ split-pos) length 'separator t 
string)))
-      (cons (substring-no-properties string 0 split-pos)
-            (substring-no-properties string (1+ split-pos) length)))))
+  "Split string STRING at `query-replace-from-to-separator'."
+  (let ((separator (or query-replace-from-to-separator " -> ")))
+    (cond ((string-match separator string)
+           (cons (substring-no-properties string 0 (match-beginning 0))
+                 (substring-no-properties string (match-end 0))))
+          (t (substring-no-properties string)))))
 
 (defun query-replace-read-from (prompt regexp-flag)
   "Query and return the `from' argument of a query-replace operation.
@@ -165,43 +160,41 @@ The return value can also be a pair (FROM . TO) 
indicating that the user
 wants to replace FROM with TO."
   (if query-replace-interactive
       (car (if regexp-flag regexp-search-ring search-ring))
-    ;; Reevaluating will check char-displayable-p that is
-    ;; unavailable while preparing to dump.
-    (custom-reevaluate-setting 'query-replace-from-to-separator)
     (let* ((history-add-new-input nil)
-          (separator
-           (when query-replace-from-to-separator
-             (propertize "\0"
-                         'display query-replace-from-to-separator
-                         'separator t)))
+           (sep-char (and (stringp query-replace-from-to-separator)
+                          (replace-regexp-in-string
+                           " " "" query-replace-from-to-separator)))
+           (separator (propertize
+                       (if (and sep-char
+                                (char-displayable-p (string-to-char sep-char)))
+                           query-replace-from-to-separator
+                         " -> ")
+                       'face 'minibuffer-prompt))
            (minibuffer-history
             (append
-            (when separator
              (mapcar (lambda (from-to)
                        (concat (query-replace-descr (car from-to))
                                separator
                                (query-replace-descr (cdr from-to))))
-                      query-replace-defaults))
+                     query-replace-defaults)
              (symbol-value query-replace-from-history-variable)))
-          (minibuffer-allow-text-properties t) ; separator uses text-properties
            (prompt
-           (if (and query-replace-defaults separator)
-               (format "%s (default %s): " prompt (car minibuffer-history))
+            (if query-replace-defaults
+                (format "%s (default %s %s %s): "
+                        prompt
+                        (query-replace-descr (caar query-replace-defaults))
+                        separator
+                        (query-replace-descr (cdar query-replace-defaults)))
               (format "%s: " prompt)))
            (from
             ;; The save-excursion here is in case the user marks and copies
             ;; a region in order to specify the minibuffer input.
             ;; That should not clobber the region for the query-replace itself.
             (save-excursion
-              (minibuffer-with-setup-hook
-                  (lambda ()
-                    (setq-local text-property-default-nonsticky
-                                (cons '(separator . t) 
text-property-default-nonsticky)))
               (if regexp-flag
                   (read-regexp prompt nil 'minibuffer-history)
                 (read-from-minibuffer
-                   prompt nil nil nil nil
-                   (car (if regexp-flag regexp-search-ring search-ring)) t)))))
+                 prompt nil nil nil nil (car search-ring) t))))
            (to))
       (if (and (zerop (length from)) query-replace-defaults)
           (cons (caar query-replace-defaults)

-- 
Thierry