Re: First two elements of search-ring shown twice in minibuffer when using M-p multiple times?

Re: First two elements of search-ring shown twice in minibuffer when using M-p multiple times?

[Juri Linkov]

> Thank you for the bug report.  I hope it is possible to fix this
> undesirable behavior by using the HISTPOS argument of read-from-minibuffer
> where HISTPOS will point to the correct history position in the search
> ring.  This also gives us the opportunity to rewrite isearch-edit-string
> to remove unnecessary ad-hoc minibuffer precessing tricks that cause the
> incorrect behavior you described in the second part of your bug report.

Below is a patch that fixes all these problems.  It uses
search-ring-yank-pointer and regexp-search-ring-yank-pointer
for the HISTPOS argument of read-from-minibuffer that gives
the correct initial minibuffer search history position for
isearch-edit-string.

It also gets rid of all trickery used to read the first character
typed in the minibuffer (that removes another set of problems;
see related old bug reports).  It adds a new backward-compatible
command `isearch-edit-string-set-word' bound to C-w in the minibuffer
that calls `kill-region' when the mark is active, and otherwise does
word search after exiting `isearch-edit-string' (the mark is not active
when `isearch-edit-string' just created the minibuffer, and without
the mark `kill-region' would fail anyway).

This preserves the behavior described in the Emacs manual:

`C-s <RET> C-w WORDS <RET>'
     Search for WORDS, ignoring details of punctuation.

Index: lisp/isearch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.313
diff -c -r1.313 isearch.el
*** lisp/isearch.el	28 Feb 2008 01:57:42 -0000	1.313
--- lisp/isearch.el	9 Mar 2008 21:57:02 -0000
***************
*** 436,441 ****
--- 436,442 ----
      (define-key map "\M-\t" 'isearch-complete-edit)
      (define-key map "\C-s"  'isearch-forward-exit-minibuffer)
      (define-key map "\C-r"  'isearch-reverse-exit-minibuffer)
+     (define-key map "\C-w"  'isearch-edit-string-set-word)
      (define-key map "\C-f"  'isearch-yank-char-in-minibuffer)
      (define-key map [right] 'isearch-yank-char-in-minibuffer)
      map)
***************
*** 1025,1061 ****
  	  ;; that can change their values.
  	  (setq old-point (point) old-other-end isearch-other-end)
  
- 	  (isearch-message) ;; for read-char
  	  (unwind-protect
! 	      (let* (;; Why does following read-char echo?
! 		     ;;(echo-keystrokes 0) ;; not needed with above message
! 		     (e (let ((cursor-in-echo-area t))
! 			  (read-event)))
  		     ;; Binding minibuffer-history-symbol to nil is a work-around
  		     ;; for some incompatibility with gmhist.
! 		     (minibuffer-history-symbol)
! 		     (message-log-max nil))
! 		;; If the first character the user types when we prompt them
! 		;; for a string is the yank-word character, then go into
! 		;; word-search mode.  Otherwise unread that character and
! 		;; read a key the normal way.
! 		;; Word search does not apply (yet) to regexp searches,
! 		;; no check is made here.
! 		(message "%s" (isearch-message-prefix nil nil t))
! 		(if (memq (lookup-key isearch-mode-map (vector e))
! 			  '(isearch-yank-word
! 			    isearch-yank-word-or-char))
! 		    (setq isearch-word t;; so message-prefix is right
! 			  isearch-new-word t)
! 		  (cancel-kbd-macro-events)
! 		  (isearch-unread e))
! 		(setq cursor-in-echo-area nil)
  		(setq isearch-new-string
                        (read-from-minibuffer
                         (isearch-message-prefix nil nil isearch-nonincremental)
                         isearch-string
                         minibuffer-local-isearch-map nil
!                        (if isearch-regexp 'regexp-search-ring 'search-ring)
                         nil t)
  		      isearch-new-message
  		      (mapconcat 'isearch-text-char-description
--- 1026,1046 ----
  	  ;; that can change their values.
  	  (setq old-point (point) old-other-end isearch-other-end)
  
  	  (unwind-protect
! 	      (let* ((message-log-max nil)
  		     ;; Binding minibuffer-history-symbol to nil is a work-around
  		     ;; for some incompatibility with gmhist.
! 		     (minibuffer-history-symbol))
  		(setq isearch-new-string
                        (read-from-minibuffer
                         (isearch-message-prefix nil nil isearch-nonincremental)
                         isearch-string
                         minibuffer-local-isearch-map nil
!                        (if isearch-regexp
! 			   (cons 'regexp-search-ring
! 				 (1+ (or regexp-search-ring-yank-pointer -1)))
! 			 (cons 'search-ring
! 			       (1+ (or search-ring-yank-pointer -1))))
                         nil t)
  		      isearch-new-message
  		      (mapconcat 'isearch-text-char-description
***************
*** 1116,1121 ****
--- 1101,1116 ----
       (isearch-abort)  ;; outside of let to restore outside global values
       )))
  
+ (defun isearch-edit-string-set-word ()
+   "Do word search after exiting `isearch-edit-string'.
+ If the mark is not active in the search string editing minibuffer,
+ then after exiting `isearch-edit-string', do word search.
+ Otherwise, kill text between point and mark in the minibuffer."
+   (interactive)
+   (if mark-active
+       (kill-region (point) (mark))
+     (setq isearch-word t isearch-new-word t)))
+ 
  (defun isearch-nonincremental-exit-minibuffer ()
    (interactive)
    (setq isearch-nonincremental t)

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: First two elements of search-ring shown twice in minibuffer when using M-p multiple times?

[Stefan Monnier]

> Below is a patch that fixes all these problems.  It uses
> search-ring-yank-pointer and regexp-search-ring-yank-pointer
> for the HISTPOS argument of read-from-minibuffer that gives
> the correct initial minibuffer search history position for
> isearch-edit-string.

Thanks.

> It also gets rid of all trickery used to read the first character
> typed in the minibuffer (that removes another set of problems;
> see related old bug reports).  It adds a new backward-compatible
> command `isearch-edit-string-set-word' bound to C-w in the minibuffer
> that calls `kill-region' when the mark is active, and otherwise does
> word search after exiting `isearch-edit-string' (the mark is not active
> when `isearch-edit-string' just created the minibuffer, and without
> the mark `kill-region' would fail anyway).

> This preserves the behavior described in the Emacs manual:

> `C-s <RET> C-w WORDS <RET>'
>      Search for WORDS, ignoring details of punctuation.

This seems unrelated, right?
It looks like a good change.  But I wonder why we don't use an approach
similar to the M-r binding to isearch-toggle-regexp.  Of course, we'd
rather not eat yet-another key (e.g. bind M-w to isearch-toggle-word),
but maybe we could change isearch-toggle-regexp into
isearch-cycle-regexp-word, such that the command cycles between
plain/regexp/word searches.


        Stefan


> Index: lisp/isearch.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/isearch.el,v
> retrieving revision 1.313
> diff -c -r1.313 isearch.el
> *** lisp/isearch.el	28 Feb 2008 01:57:42 -0000	1.313
> --- lisp/isearch.el	9 Mar 2008 21:57:02 -0000
> ***************
> *** 436,441 ****
> --- 436,442 ----
>       (define-key map "\M-\t" 'isearch-complete-edit)
>       (define-key map "\C-s"  'isearch-forward-exit-minibuffer)
>       (define-key map "\C-r"  'isearch-reverse-exit-minibuffer)
> +     (define-key map "\C-w"  'isearch-edit-string-set-word)
>       (define-key map "\C-f"  'isearch-yank-char-in-minibuffer)
>       (define-key map [right] 'isearch-yank-char-in-minibuffer)
>       map)
> ***************
> *** 1025,1061 ****
>   	  ;; that can change their values.
>   	  (setq old-point (point) old-other-end isearch-other-end)
  
> - 	  (isearch-message) ;; for read-char
>   	  (unwind-protect
> ! 	      (let* (;; Why does following read-char echo?
> ! 		     ;;(echo-keystrokes 0) ;; not needed with above message
> ! 		     (e (let ((cursor-in-echo-area t))
> ! 			  (read-event)))
>   		     ;; Binding minibuffer-history-symbol to nil is a work-around
>   		     ;; for some incompatibility with gmhist.
> ! 		     (minibuffer-history-symbol)
> ! 		     (message-log-max nil))
> ! 		;; If the first character the user types when we prompt them
> ! 		;; for a string is the yank-word character, then go into
> ! 		;; word-search mode.  Otherwise unread that character and
> ! 		;; read a key the normal way.
> ! 		;; Word search does not apply (yet) to regexp searches,
> ! 		;; no check is made here.
> ! 		(message "%s" (isearch-message-prefix nil nil t))
> ! 		(if (memq (lookup-key isearch-mode-map (vector e))
> ! 			  '(isearch-yank-word
> ! 			    isearch-yank-word-or-char))
> ! 		    (setq isearch-word t;; so message-prefix is right
> ! 			  isearch-new-word t)
> ! 		  (cancel-kbd-macro-events)
> ! 		  (isearch-unread e))
> ! 		(setq cursor-in-echo-area nil)
>   		(setq isearch-new-string
>                         (read-from-minibuffer
>                          (isearch-message-prefix nil nil isearch-nonincremental)
>                          isearch-string
>                          minibuffer-local-isearch-map nil
> !                        (if isearch-regexp 'regexp-search-ring 'search-ring)
>                          nil t)
>   		      isearch-new-message
>   		      (mapconcat 'isearch-text-char-description
> --- 1026,1046 ----
>   	  ;; that can change their values.
>   	  (setq old-point (point) old-other-end isearch-other-end)
  
>   	  (unwind-protect
> ! 	      (let* ((message-log-max nil)
>   		     ;; Binding minibuffer-history-symbol to nil is a work-around
>   		     ;; for some incompatibility with gmhist.
> ! 		     (minibuffer-history-symbol))
>   		(setq isearch-new-string
>                         (read-from-minibuffer
>                          (isearch-message-prefix nil nil isearch-nonincremental)
>                          isearch-string
>                          minibuffer-local-isearch-map nil
> !                        (if isearch-regexp
> ! 			   (cons 'regexp-search-ring
> ! 				 (1+ (or regexp-search-ring-yank-pointer -1)))
> ! 			 (cons 'search-ring
> ! 			       (1+ (or search-ring-yank-pointer -1))))
>                          nil t)
>   		      isearch-new-message
>   		      (mapconcat 'isearch-text-char-description
> ***************
> *** 1116,1121 ****
> --- 1101,1116 ----
>        (isearch-abort)  ;; outside of let to restore outside global values
>        )))
  
> + (defun isearch-edit-string-set-word ()
> +   "Do word search after exiting `isearch-edit-string'.
> + If the mark is not active in the search string editing minibuffer,
> + then after exiting `isearch-edit-string', do word search.
> + Otherwise, kill text between point and mark in the minibuffer."
> +   (interactive)
> +   (if mark-active
> +       (kill-region (point) (mark))
> +     (setq isearch-word t isearch-new-word t)))
> + 
>   (defun isearch-nonincremental-exit-minibuffer ()
>     (interactive)
>     (setq isearch-nonincremental t)

> -- 
> Juri Linkov
> http://www.jurta.org/emacs/

Word search (was: First two elements of search-ring shown twice in minibuffer when using M-p multiple times?)

[Juri Linkov]

>> It also gets rid of all trickery used to read the first character
>> typed in the minibuffer (that removes another set of problems;
>> see related old bug reports).  It adds a new backward-compatible
>> command `isearch-edit-string-set-word' bound to C-w in the minibuffer
>> that calls `kill-region' when the mark is active, and otherwise does
>> word search after exiting `isearch-edit-string' (the mark is not active
>> when `isearch-edit-string' just created the minibuffer, and without
>> the mark `kill-region' would fail anyway).
>
>> This preserves the behavior described in the Emacs manual:
>
>> `C-s <RET> C-w WORDS <RET>'
>>      Search for WORDS, ignoring details of punctuation.
>
> This seems unrelated, right?

This is slightly related, but now I see it can be installed with
a separate patch.

> It looks like a good change.  But I wonder why we don't use an approach
> similar to the M-r binding to isearch-toggle-regexp.

Keeping C-w to specify word search in the minibuffer is necessary
for backward compatibility.  When we find a better method to toggle
word search, we could remove the description of the old method
from the manual, and remove its code in later releases.

> Of course, we'd rather not eat yet-another key (e.g. bind M-w to
> isearch-toggle-word), but maybe we could change isearch-toggle-regexp
> into isearch-cycle-regexp-word, such that the command cycles between
> plain/regexp/word searches.

One disadvantage of M-r is that it's not mnemonic to toggle word search.
OTOH, it is convenient to cycle between search types.

We could also add another keybinding to separately toggle only
word search `M-s w' and regexp search `M-s r'.  Their key sequences are
longer than `M-r' but they will allow specifying the exact search type.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Word search

[Stefan Monnier]

>>> It also gets rid of all trickery used to read the first character
>>> typed in the minibuffer (that removes another set of problems;
>>> see related old bug reports).  It adds a new backward-compatible
>>> command `isearch-edit-string-set-word' bound to C-w in the minibuffer
>>> that calls `kill-region' when the mark is active, and otherwise does
>>> word search after exiting `isearch-edit-string' (the mark is not active
>>> when `isearch-edit-string' just created the minibuffer, and without
>>> the mark `kill-region' would fail anyway).
>> 
>>> This preserves the behavior described in the Emacs manual:
>> 
>>> `C-s <RET> C-w WORDS <RET>'
>>> Search for WORDS, ignoring details of punctuation.
>> 
>> This seems unrelated, right?

> This is slightly related, but now I see it can be installed with
> a separate patch.

>> It looks like a good change.  But I wonder why we don't use an approach
>> similar to the M-r binding to isearch-toggle-regexp.

> Keeping C-w to specify word search in the minibuffer is necessary
> for backward compatibility.  When we find a better method to toggle
> word search, we could remove the description of the old method
> from the manual, and remove its code in later releases.

Backward compatibility with old elisp packages is important.
Backward compatibility with old Emacs users is a bit less important.

>> Of course, we'd rather not eat yet-another key (e.g. bind M-w to
>> isearch-toggle-word), but maybe we could change isearch-toggle-regexp
>> into isearch-cycle-regexp-word, such that the command cycles between
>> plain/regexp/word searches.

> One disadvantage of M-r is that it's not mnemonic to toggle word search.
> OTOH, it is convenient to cycle between search types.

> We could also add another keybinding to separately toggle only
> word search `M-s w' and regexp search `M-s r'.  Their key sequences are
> longer than `M-r' but they will allow specifying the exact search type.

Using M-s as a prefix might be a good idea.  What do others think?


        Stefan

Re: Word search

[Juri Linkov]

>> Keeping C-w to specify word search in the minibuffer is necessary
>> for backward compatibility.  When we find a better method to toggle
>> word search, we could remove the description of the old method
>> from the manual, and remove its code in later releases.
>
> Backward compatibility with old elisp packages is important.
> Backward compatibility with old Emacs users is a bit less important.

Then we could use C-w for something more useful like getting the next
word from the buffer and appending it to the search string in the
minibuffer.  This would be very convenient since this is exactly what
C-w does in isearch mode.

This will allow us finally to get rid of isearch-yank-char-in-minibuffer
which does almost the same but in an unpredictable way.

I also noticed a similar command `bookmark-yank-word' in bookmark.el.
But when the mark is active, it doesn't kill the region.  I think
it should check for the active mark, and call `kill-region' in this
case like:

(defun bookmark-yank-word ()
  (interactive)
  ;; get the next word from the buffer and append it to the name of
  ;; the bookmark currently being set.
  (if mark-active
      (kill-region (point) (mark))
    (let ((string (save-excursion
		    (set-buffer bookmark-current-buffer)
		    (goto-char bookmark-yank-point)
		    (buffer-substring-no-properties
		     (point)
		     (progn
		       (forward-word 1)
		       (setq bookmark-yank-point (point)))))))
      (insert string))))

Generally, the logic could be the following: when C-w will fail anyway
due to the absence of the active mark in the minibuffer, do a different
action like pulling the next word from the buffer to the minibuffer.

BTW, in bookmark.el I see:

    ;; This C-u binding might not be very useful any more now that we
    ;; provide access to the default via the standard M-n binding.
    ;; Maybe we should just remove it?  --Stef-08
    (define-key map "\C-u" 'bookmark-insert-current-bookmark)

I agree that this is unnecessary any more, and if no one will object,
maybe we could just remove it?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Word search

[Stefan Monnier]

> BTW, in bookmark.el I see:

>     ;; This C-u binding might not be very useful any more now that we
>     ;; provide access to the default via the standard M-n binding.
>     ;; Maybe we should just remove it?  --Stef-08
>     (define-key map "\C-u" 'bookmark-insert-current-bookmark)

> I agree that this is unnecessary any more, and if no one will object,
> maybe we could just remove it?

Actually, I decided to leave it in, hence the comment.
The reason why I decided to leave it in, is that some users may like to
type something and then hit C-u which will *add* the default, whereas
M-n will repalce what they typed with the default.
Maybe I'm being overly cautious here and we can just throw away this
binding, but I don't know enough how other people may use bookmarks to
feel confident about it.


        Stefan


PS: Oh and for your C-w, I personally think we should have a generic C-w
binding in minibuffer-local-map.

Re: Word search

[Juri Linkov]

> PS: Oh and for your C-w, I personally think we should have a generic
> C-w binding in minibuffer-local-map.

The following patch implements this:

Index: lisp/bindings.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/bindings.el,v
retrieving revision 1.198
diff -c -r1.198 bindings.el
*** lisp/bindings.el	5 Mar 2008 04:09:24 -0000	1.198
--- lisp/bindings.el	12 Mar 2008 00:32:07 -0000
***************
*** 773,779 ****
    ;; indent-for-tab-command).  The alignment that indent-relative tries to
    ;; do doesn't make much sense here since the prompt messes it up.
    (define-key map "\t"    'self-insert-command)
!   (define-key minibuffer-local-map [C-tab] 'file-cache-minibuffer-complete))
  
  (define-key global-map "\C-u" 'universal-argument)
  (let ((i ?0))
--- 773,780 ----
    ;; indent-for-tab-command).  The alignment that indent-relative tries to
    ;; do doesn't make much sense here since the prompt messes it up.
    (define-key map "\t"    'self-insert-command)
!   (define-key map [C-tab] 'file-cache-minibuffer-complete)
!   (define-key map "\C-w"  'minibuffer-yank-word-or-char))
  
  (define-key global-map "\C-u" 'universal-argument)
  (let ((i ?0))

Index: lisp/simple.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/simple.el,v
retrieving revision 1.904
diff -c -r1.904 simple.el
*** lisp/simple.el	3 Mar 2008 02:16:00 -0000	1.904
--- lisp/simple.el	12 Mar 2008 00:32:31 -0000
***************
*** 1393,1398 ****
--- 1407,1435 ----
    ;; Return the width of everything before the field at the end of
    ;; the buffer; this should be 0 for normal buffers.
    (1- (minibuffer-prompt-end)))
+ 
+ (defun minibuffer-yank-word-or-char ()
+   "Pull next character or word from buffer into the minibuffer.
+ 
+ If the mark is active in the minibuffer, then kill text between point
+ and mark in the minibuffer using `kill-region'.
+ 
+ Otherwise, get the next word or character from the original buffer
+ and append it to the minibuffer."
+   (interactive)
+   (if mark-active
+       (kill-region (point) (mark))
+     (insert
+      (with-current-buffer (other-buffer (current-buffer) t)
+        (buffer-substring-no-properties
+         (point)
+         (progn
+           (if (or (= (char-syntax (or (char-after) 0)) ?w)
+                   (= (char-syntax (or (char-after (1+ (point))) 0)) ?w))
+               (forward-word 1)
+             (forward-char 1))
+           (point)))))))
+ 
  \f
  ;; isearch minibuffer history
  (add-hook 'minibuffer-setup-hook 'minibuffer-history-isearch-setup)

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Word search

[Stefan Monnier]

> +      (with-current-buffer (other-buffer (current-buffer) t)

Are you sure this gives the right buffer in all cases?
Have you tried it after ? displayed the completions, for example?


        Stefan

Re: Word search

[Juri Linkov]

>> +      (with-current-buffer (other-buffer (current-buffer) t)
>
> Are you sure this gives the right buffer in all cases?
> Have you tried it after ? displayed the completions, for example?

Yes, it works after ? displayed the completions, i.e. it still yanks chars
from the original buffer, not from the completions buffer.

But I agree that this is not a reliable method.  Maybe we should record
the original buffer in a special variable before activating the minibuffer?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Word search

[Stefan Monnier]

>>> +      (with-current-buffer (other-buffer (current-buffer) t)
>> 
>> Are you sure this gives the right buffer in all cases?
>> Have you tried it after ? displayed the completions, for example?

> Yes, it works after ? displayed the completions, i.e. it still yanks chars
> from the original buffer, not from the completions buffer.

> But I agree that this is not a reliable method.  Maybe we should record
> the original buffer in a special variable before activating the minibuffer?

What about using `minibuffer-selected-window'?


        Stefan

Re: Word search

[Richard Stallman]

    > PS: Oh and for your C-w, I personally think we should have a generic
    > C-w binding in minibuffer-local-map.

It is a really bad idea to start making editing commands work
differently in the minibuffer.

Re: Word search

[Richard Stallman]

If we are thinking of eliminating the C-w word-search feature,
we should poll the users about it.

Maybe lots of people use it.
Maybe hardly anyone uses it.

Re: Word search

[Juri Linkov]

> If we are thinking of eliminating the C-w word-search feature,
> we should poll the users about it.
>
> Maybe lots of people use it.
> Maybe hardly anyone uses it.

The current method of entering word search is very inconvenient.
So even if lots of people use it and we will find a better method,
they might be even grateful for this.

But I doubt that many people use it because for many years of existence
of word search, very few people tried to find a way to enable incremental
word search (according to mailing list archives) until recently.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Word search

[Richard Stallman]

    But I doubt that many people use it because for many years of existence
    of word search, very few people tried to find a way to enable incremental
    word search (according to mailing list archives) until recently.

Maybe you're right.  But let's ask them.

Re: Word search

[Juri Linkov]

>     But I doubt that many people use it because for many years of existence
>     of word search, very few people tried to find a way to enable incremental
>     word search (according to mailing list archives) until recently.
>
> Maybe you're right.  But let's ask them.

I don't understand what does asking them mean?  Creating a webpage
with a poll?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Word search

[Richard Stallman]

    I don't understand what does asking them mean?  Creating a webpage
    with a poll?

I used to poll the users by sending mail to info-gnu-emacs and
help-gnu-emacs.  Putting the poll in a web page also might be good.

It's up to the maintainers to decide if they want to poll the users.

Re: Word search

[René Kyllingstad]

* Richard Stallman:
>      But I doubt that many people use it because for many years of
>      existence of word search, very few people tried to find a way to
>      enable incremental word search (according to mailing list archives)
>      until recently.
>  
>  Maybe you're right.  But let's ask them.

I never realized it was there.  I remember trying C-y hoping for that, but
it inserts to the end of the line, something I never need.

I would prefer to have bindings similar to normal editing: C-y to yank, but
use M-y to insert the word at point, unless the previous command was yank.
Makes it easier to remember.


-- René

Re: Word search

[Juri Linkov]

>>      But I doubt that many people use it because for many years of
>>      existence of word search, very few people tried to find a way to
>>      enable incremental word search (according to mailing list archives)
>>      until recently.
>>
>>  Maybe you're right.  But let's ask them.
>
> I never realized it was there.  I remember trying C-y hoping for that, but
> it inserts to the end of the line, something I never need.
>
> I would prefer to have bindings similar to normal editing: C-y to yank, but
> use M-y to insert the word at point, unless the previous command was yank.
> Makes it easier to remember.

We currently have:

C-w isearch-yank-word-or-char
C-y isearch-yank-line
M-y isearch-yank-kill

Do you propose this change:

C-w isearch-yank-word-or-char
M-y isearch-yank-word-or-char
C-y isearch-yank-kill

But where do you put `isearch-yank-line'?

-- 
Juri Linkov
http://www.jurta.org/emacs/

RE: Word search

[Drew Adams]

1. It might be OK to have C-w do something different when the region is
empty. At least for those people who use transient-mark mode (so that
"active region" means something).

2. That is not limited to the minibuffer, logically. That is, there is
nothing special about the minibuffer in this regard.

3. It is OK to have some minibuffer key yank text from the current buffer -
for example, text at or near point.

4. We have discussed #3 before. I suggested behavior that lets you yank
different kinds of things at point and behavior that lets you yank
successive things at point. Nothing has come of that, so far.

5. HOWEVER: If an approach such as #1 is used, it should not be combined
with #3, and especially not using `C-w'. That is trying to be too clever by
half, and it doesn't help.

That is, there is no reason to combine these features you propose. And doing
so limits design for the future. And the use of C-w then becomes confusing
for users: sometimes cut (kill), sometimes paste (yank). That's not good UI,
IMO.

So if you do #3, please use a different key. And if you do #1, consider
doing so in every buffer, not just the minibuffer.

Re: Word search

[Richard Stallman]

    1. It might be OK to have C-w do something different when the region is
    empty. At least for those people who use transient-mark mode (so that
    "active region" means something).

This is the sort of thing we should poll the users about.

Re: Word search

[Juri Linkov]

> 1. It might be OK to have C-w do something different when the region is
> empty. At least for those people who use transient-mark mode (so that
> "active region" means something).
>
> 2. That is not limited to the minibuffer, logically. That is, there is
> nothing special about the minibuffer in this regard.
>
> 3. It is OK to have some minibuffer key yank text from the current buffer -
> for example, text at or near point.
>
> 4. We have discussed #3 before. I suggested behavior that lets you yank
> different kinds of things at point and behavior that lets you yank
> successive things at point. Nothing has come of that, so far.
>
> 5. HOWEVER: If an approach such as #1 is used, it should not be combined
> with #3, and especially not using `C-w'. That is trying to be too clever by
> half, and it doesn't help.
>
> That is, there is no reason to combine these features you propose. And doing
> so limits design for the future. And the use of C-w then becomes confusing
> for users: sometimes cut (kill), sometimes paste (yank). That's not good UI,
> IMO.
>
> So if you do #3, please use a different key. And if you do #1, consider
> doing so in every buffer, not just the minibuffer.

C-w in the search string editing minibuffer is a natural extension of
C-w in isearch mode (`isearch-yank-word-or-char') because there will be
no difference between `C-s C-w C-w ...' and `C-s M-e C-w C-w ...'.

Since `isearch-edit-string' uses a normal minibuffer, it would be
natural to extend this C-w feature to other minibuffers.

But it is true that such dual nature of C-w might cause troubles
when transient-mark-mode is off.

Maybe, we could accept C-w as a yanking key only at the beginning of
the minibuffer?

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Word search

[Richard Stallman]

    C-w in the search string editing minibuffer is a natural extension of
    C-w in isearch mode (`isearch-yank-word-or-char') because there will be
    no difference between `C-s C-w C-w ...' and `C-s M-e C-w C-w ...'.

    Since `isearch-edit-string' uses a normal minibuffer, it would be
    natural to extend this C-w feature to other minibuffers.

This seems like a hair on the tail wagging the dog.

Re: First two elements of search-ring shown twice in minibuffer when using M-p multiple times?

[Tobias Bading]

Hi Juri,

thanks a million! I managed to "port" your fix down to the 1.297  
version (tagged EMACS_22_1) by only applying your patch of isearch- 
edit-string and it seems to work fine. Funnily enough the patch only  
works if I copy the whole definition of the patched isearch-edit- 
string into my .emacs file. If I put the patch into the original  
isearch.el under c:\emacs\lisp at work or /usr/local/share/emacs/22.1/ 
lisp on my Mac at home, the patch is ignored :-(. I *did* compile the  
file in both cases, the isearch.elc file was created without a  
warning or error. What's up with that? Is isearch.el dumped into my  
emacs binaries and emacs doesn't realize that the version on disk is  
newer or something?

Thanks again,
Tobias


On 09.03.2008, at 22:59, Juri Linkov wrote:

>> Thank you for the bug report.  I hope it is possible to fix this
>> undesirable behavior by using the HISTPOS argument of read-from- 
>> minibuffer
>> where HISTPOS will point to the correct history position in the  
>> search
>> ring.  This also gives us the opportunity to rewrite isearch-edit- 
>> string
>> to remove unnecessary ad-hoc minibuffer precessing tricks that  
>> cause the
>> incorrect behavior you described in the second part of your bug  
>> report.
>
> Below is a patch that fixes all these problems.  It uses
> search-ring-yank-pointer and regexp-search-ring-yank-pointer
> for the HISTPOS argument of read-from-minibuffer that gives
> the correct initial minibuffer search history position for
> isearch-edit-string.
>
> It also gets rid of all trickery used to read the first character
> typed in the minibuffer (that removes another set of problems;
> see related old bug reports).  It adds a new backward-compatible
> command `isearch-edit-string-set-word' bound to C-w in the minibuffer
> that calls `kill-region' when the mark is active, and otherwise does
> word search after exiting `isearch-edit-string' (the mark is not  
> active
> when `isearch-edit-string' just created the minibuffer, and without
> the mark `kill-region' would fail anyway).
>
> This preserves the behavior described in the Emacs manual:
>
> `C-s <RET> C-w WORDS <RET>'
>      Search for WORDS, ignoring details of punctuation.
>
> Index: lisp/isearch.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/isearch.el,v
> retrieving revision 1.313
> diff -c -r1.313 isearch.el
> *** lisp/isearch.el	28 Feb 2008 01:57:42 -0000	1.313
> --- lisp/isearch.el	9 Mar 2008 21:57:02 -0000
> ***************
> *** 436,441 ****
> --- 436,442 ----
>       (define-key map "\M-\t" 'isearch-complete-edit)
>       (define-key map "\C-s"  'isearch-forward-exit-minibuffer)
>       (define-key map "\C-r"  'isearch-reverse-exit-minibuffer)
> +     (define-key map "\C-w"  'isearch-edit-string-set-word)
>       (define-key map "\C-f"  'isearch-yank-char-in-minibuffer)
>       (define-key map [right] 'isearch-yank-char-in-minibuffer)
>       map)
> ***************
> *** 1025,1061 ****
>   	  ;; that can change their values.
>   	  (setq old-point (point) old-other-end isearch-other-end)
>
> - 	  (isearch-message) ;; for read-char
>   	  (unwind-protect
> ! 	      (let* (;; Why does following read-char echo?
> ! 		     ;;(echo-keystrokes 0) ;; not needed with above message
> ! 		     (e (let ((cursor-in-echo-area t))
> ! 			  (read-event)))
>   		     ;; Binding minibuffer-history-symbol to nil is a work-around
>   		     ;; for some incompatibility with gmhist.
> ! 		     (minibuffer-history-symbol)
> ! 		     (message-log-max nil))
> ! 		;; If the first character the user types when we prompt them
> ! 		;; for a string is the yank-word character, then go into
> ! 		;; word-search mode.  Otherwise unread that character and
> ! 		;; read a key the normal way.
> ! 		;; Word search does not apply (yet) to regexp searches,
> ! 		;; no check is made here.
> ! 		(message "%s" (isearch-message-prefix nil nil t))
> ! 		(if (memq (lookup-key isearch-mode-map (vector e))
> ! 			  '(isearch-yank-word
> ! 			    isearch-yank-word-or-char))
> ! 		    (setq isearch-word t;; so message-prefix is right
> ! 			  isearch-new-word t)
> ! 		  (cancel-kbd-macro-events)
> ! 		  (isearch-unread e))
> ! 		(setq cursor-in-echo-area nil)
>   		(setq isearch-new-string
>                         (read-from-minibuffer
>                          (isearch-message-prefix nil nil isearch- 
> nonincremental)
>                          isearch-string
>                          minibuffer-local-isearch-map nil
> !                        (if isearch-regexp 'regexp-search-ring  
> 'search-ring)
>                          nil t)
>   		      isearch-new-message
>   		      (mapconcat 'isearch-text-char-description
> --- 1026,1046 ----
>   	  ;; that can change their values.
>   	  (setq old-point (point) old-other-end isearch-other-end)
>
>   	  (unwind-protect
> ! 	      (let* ((message-log-max nil)
>   		     ;; Binding minibuffer-history-symbol to nil is a work-around
>   		     ;; for some incompatibility with gmhist.
> ! 		     (minibuffer-history-symbol))
>   		(setq isearch-new-string
>                         (read-from-minibuffer
>                          (isearch-message-prefix nil nil isearch- 
> nonincremental)
>                          isearch-string
>                          minibuffer-local-isearch-map nil
> !                        (if isearch-regexp
> ! 			   (cons 'regexp-search-ring
> ! 				 (1+ (or regexp-search-ring-yank-pointer -1)))
> ! 			 (cons 'search-ring
> ! 			       (1+ (or search-ring-yank-pointer -1))))
>                          nil t)
>   		      isearch-new-message
>   		      (mapconcat 'isearch-text-char-description
> ***************
> *** 1116,1121 ****
> --- 1101,1116 ----
>        (isearch-abort)  ;; outside of let to restore outside global  
> values
>        )))
>
> + (defun isearch-edit-string-set-word ()
> +   "Do word search after exiting `isearch-edit-string'.
> + If the mark is not active in the search string editing minibuffer,
> + then after exiting `isearch-edit-string', do word search.
> + Otherwise, kill text between point and mark in the minibuffer."
> +   (interactive)
> +   (if mark-active
> +       (kill-region (point) (mark))
> +     (setq isearch-word t isearch-new-word t)))
> +
>   (defun isearch-nonincremental-exit-minibuffer ()
>     (interactive)
>     (setq isearch-nonincremental t)
>
> -- 
> Juri Linkov
> http://www.jurta.org/emacs/

Re: First two elements of search-ring shown twice in minibuffer when using M-p multiple times?

[Juri Linkov]

> thanks a million! I managed to "port" your fix down to the 1.297 version
> (tagged EMACS_22_1) by only applying your patch of isearch- 
> edit-string and it seems to work fine. Funnily enough the patch only works
> if I copy the whole definition of the patched isearch-edit- 
> string into my .emacs file. If I put the patch into the original
> isearch.el under c:\emacs\lisp at work or /usr/local/share/emacs/22.1/
> lisp on my Mac at home, the patch is ignored :-(. I *did* compile the
> file in both cases, the isearch.elc file was created without a  warning or
> error. What's up with that? Is isearch.el dumped into my  emacs binaries
> and emacs doesn't realize that the version on disk is  newer or something?

Yes, your guess is right: isearch.el is dumped into the Emacs executable,
but you can reload the newest version of isearch.el in your .emacs with
(load "isearch").

-- 
Juri Linkov
http://www.jurta.org/emacs/

isearch-push-state (was: First two elements of search-ring shown twice in minibuffer when using M-p multiple times?)

[Juri Linkov]

The recent change that highlights the failed part of the search
string helped to discover the following bug: typing `C-s M-e foo C-s'
(where `foo' is a string that doesn't exist in the current buffer)
doesn't highlight the failed search string in the isearch-fail face
after exiting from the edit-string minibuffer.

This is because `isearch-edit-string' puts the wrong search state
to the search status stack.  It calls `isearch-push-state' that
stores the new search string (that will fail to find) and the old
successfull state to `isearch-cmds'.

There are two calls of `isearch-push-state' in `isearch-edit-string'
and I think the first one is unnecessary because the state before M-e
is already recorded on the stack.

I also discovered another bug that the ring advancing command M-p puts
one unnecessary search state to the search status stack, but this is
only necessary when `search-ring-update' is non-nil.  Otherwise,
`isearch-edit-string' already has the call to `isearch-push-state'.

I tried different test cases and see no possible scenario that there
the following patch would fail:

Index: lisp/isearch.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.313
diff -c -r1.313 isearch.el
*** lisp/isearch.el	28 Feb 2008 01:57:42 -0000	1.313
--- lisp/isearch.el	10 Mar 2008 17:56:09 -0000
***************
*** 1096,1104 ****
  	    ;; Only the string actually used should be saved.
  	    ))
  
- 	;; Push the state as of before this C-s.
- 	(isearch-push-state)
- 
  	;; Reinvoke the pending search.
  	(isearch-search)
  	(isearch-push-state)
--- 1081,1086 ----
***************
*** 1895,1904 ****
    (if search-ring-update
        (progn
  	(isearch-search)
  	(isearch-update))
!     (isearch-edit-string)
!     )
!   (isearch-push-state))
  
  (defun isearch-ring-advance ()
    "Advance to the next search string in the ring."
--- 1887,1895 ----
    (if search-ring-update
        (progn
  	(isearch-search)
+ 	(isearch-push-state)
  	(isearch-update))
!     (isearch-edit-string)))
  
  (defun isearch-ring-advance ()
    "Advance to the next search string in the ring."

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: isearch-push-state

[Stefan Monnier]

Thanks Juri.  I'm not sure I understand what you said: are you saying
that those extra state-pushes are just unnecessary or are they
really harmful?  It seems they are harmful w.r.t the new
failed-search-highlight thingy, but shouldn't the failed-search-highlight
thingy try to pop several states until findind one that
does succeed?

I'm not sure I like the idea that "spurious" state pushes can really
be harmful.

This said, wherever you remove those pushes, please replace them with
a comment mentioning that it was there and why you think
it's unnecessary.


        Stefan



> Index: lisp/isearch.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/isearch.el,v
> retrieving revision 1.313
> diff -c -r1.313 isearch.el
> *** lisp/isearch.el	28 Feb 2008 01:57:42 -0000	1.313
> --- lisp/isearch.el	10 Mar 2008 17:56:09 -0000
> ***************
> *** 1096,1104 ****
>   	    ;; Only the string actually used should be saved.
>   	    ))
  
> - 	;; Push the state as of before this C-s.
> - 	(isearch-push-state)
> - 
>   	;; Reinvoke the pending search.
>   	(isearch-search)
>   	(isearch-push-state)
> --- 1081,1086 ----
> ***************
> *** 1895,1904 ****
>     (if search-ring-update
>         (progn
>   	(isearch-search)
>   	(isearch-update))
> !     (isearch-edit-string)
> !     )
> !   (isearch-push-state))
  
>   (defun isearch-ring-advance ()
>     "Advance to the next search string in the ring."
> --- 1887,1895 ----
>     (if search-ring-update
>         (progn
>   	(isearch-search)
> + 	(isearch-push-state)
>   	(isearch-update))
> !     (isearch-edit-string)))
  
>   (defun isearch-ring-advance ()
>     "Advance to the next search string in the ring."

> -- 
> Juri Linkov
> http://www.jurta.org/emacs/

Re: isearch-push-state

[Juri Linkov]

> Thanks Juri.  I'm not sure I understand what you said: are you saying
> that those extra state-pushes are just unnecessary or are they
> really harmful?

They are both unnecessary and harmful for different reasons.

They are unnecessary when doing backtracking with DEL.  They add an
unnecessary and inconsistent state that puts point to the beginning
of the previous successful match, lazy-highlights matches
for the string later entered in the minibuffer, and no string
is highlighted at the current successful match.

So for instance, `C-s foo M-e C-a C-k bar C-s DEL' in a buffer
where both strings "foo" and "bar" exist, isearch enters the
state where point is on "foo", the echo area displays
"I-search: bar", "bar" is highlighted in `lazy-highlight' face,
and no string is highlighted in `isearch' face even though
the echo area says it returned to the state where it successfully
found the string "bar".

The first part of my patch removes this inconsistent state in
`isearch-edit-string', but keeps the second call to `isearch-push-state'
that records the correct state after searching for a new string.

The second part of the patch (in `isearch-ring-adjust') removes the state
that is a duplicate of the state added by `isearch-edit-string'.

> It seems they are harmful w.r.t the new failed-search-highlight
> thingy, but shouldn't the failed-search-highlight thingy try to pop
> several states until findind one that does succeed?

This is what it already tries to do, but fails when encounters an
inconsistent state, e.g. `C-s foo M-e C-a C-k bar C-s in a buffer
where only the string "foo" exists, the stack contains a state
marked as successful with the failed search string:

(["bar" "bar" 194 nil t nil nil nil nil 192 t nil]
 ["bar" "bar" 194 197 t nil nil nil nil 192 t nil] <-- wrong state
 ["foo" "foo" 197 197 t 194 nil nil nil 192 t nil]
 ["fo"  "fo"  196 196 t 194 nil nil nil 192 t nil]
 ["f"   "f"   195 195 t 194 nil nil nil 192 t nil]
 [""    ""    192 t   t nil nil nil nil 192 t nil])

> I'm not sure I like the idea that "spurious" state pushes can really
> be harmful.
>
> This said, wherever you remove those pushes, please replace them with
> a comment mentioning that it was there and why you think
> it's unnecessary.

OK, will do.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: isearch-push-state

[Stefan Monnier]

>> Thanks Juri.  I'm not sure I understand what you said: are you saying
>> that those extra state-pushes are just unnecessary or are they
>> really harmful?
> They are both unnecessary and harmful for different reasons.

Oh, so one of the two pushes is just redundant (unnecessary but not
harmful), but the other one is harmful because it pushes an inconsistent
state.  Thanks.


        Stefan

Re: First two elements of search-ring shown twice in minibuffer when using M-p multiple times?

[Juri Linkov]

> Below is a patch that fixes all these problems.  It uses
> search-ring-yank-pointer and regexp-search-ring-yank-pointer
> for the HISTPOS argument of read-from-minibuffer that gives
> the correct initial minibuffer search history position for
> isearch-edit-string.

I noticed that this change also fixed `C-s M-n M-n ...' to work correctly,
i.e. to start with the last element of the search ring and advance down
to the first element of the search ring in the minibuffer.

But I see one inconvenience in using M-p.  It is intended to retrieve
the previous search string from the search ring.  So when the ring is
'("3" "2" "1"), `C-s M-p' could retrieve "3".  I suppose it currently
retrieves "2" because there exists another convenient key sequence
`C-s C-s' to retrieve the last search string "3"?

However, when the current search string is not the same as the last
search string in the search ring, it seems more natural to expect M-p
to retrieve the last search string "3", i.e. what `C-s 4 M-p'
should retrieve: "3" or "2", I'm not sure.

-- 
Juri Linkov
http://www.jurta.org/emacs/