Re: Patch that adds help to isearch

Re: Patch that adds help to isearch

[LENNART BORGMAN]

From: Juri Linkov <juri@jurta.org>

> What about enabling isearch-mode-help only when the search string 
> is empty?
> 
> I understand the need to exit the search with C-h when the search
> string is not empty like in the case of `C-s some-function-name C-
> h f RET'
> to call `describe-function' on the found function name.  But I 
> don't see
> the need to exit the search with C-h just after C-s as in `C-s C-h f'.
> 
> OTOH, I can imagine users (especially novices) after typing C-s 
> and seeing
> the prompt `I-search: ' wanting the help about what to do next.  
> It is
> natural for them to type C-h in this case to see the Help buffer from
> `isearch-mode-help'.

Thanks for the explanation about why C-h is needed for an experienced user. I think however that enabling the help only for the state when the search string is empty would be confusing. I would prefer a defcustom to enable/disable isearch-mode-help on C-h instead. Could that satisfy experienced users?

Re: Patch that adds help to isearch

[Juri Linkov]

> Thanks for the explanation about why C-h is needed for an
> experienced user.  I think however that enabling the help only for
> the state when the search string is empty would be confusing.
> I would prefer a defcustom to enable/disable isearch-mode-help on
> C-h instead.  Could that satisfy experienced users?

A key definition is as simple as defcustom.  Instead of:

(setq isearch-bind-C-h-to-isearch-mode-help t)

you can put in .emacs:

(define-key map "\C-h" 'isearch-mode-help)

The main question is what should be the default.  For novices, who
don't know how to configure Emacs, the default binding of C-h to
`isearch-mode-help' is very helpful.  More experienced users can
put in .emacs:

(define-key isearch-mode-map "\C-h" nil)

if C-h is bound to `isearch-mode-help' in isearch.el by default.

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

Re: Patch that adds help to isearch

[Lennart Borgman]

Juri Linkov wrote:

>>Thanks for the explanation about why C-h is needed for an
>>experienced user.  I think however that enabling the help only for
>>the state when the search string is empty would be confusing.
>>I would prefer a defcustom to enable/disable isearch-mode-help on
>>C-h instead.  Could that satisfy experienced users?
>>    
>>
>
>A key definition is as simple as defcustom.  Instead of:
>
>(setq isearch-bind-C-h-to-isearch-mode-help t)
>
>you can put in .emacs:
>
>(define-key map "\C-h" 'isearch-mode-help)
>
>The main question is what should be the default.  For novices, who
>don't know how to configure Emacs, the default binding of C-h to
>`isearch-mode-help' is very helpful.  More experienced users can
>put in .emacs:
>
>(define-key isearch-mode-map "\C-h" nil)
>
>if C-h is bound to `isearch-mode-help' in isearch.el by default.
>  
>
Unfortunately you need a bit more to make it useful. In the patch I sent 
before I also added scrolling of the help buffer. Scrolling of the help 
buffer is done by the keys the user has bound to `scroll-other-window' 
and 'scroll-other-window-down'. It is only active when the other window 
is the help buffer.

However the scrolling could of course be dependent on if C-h and f1 is 
bound to isearch-mode-help.A defcustom seems more easy to understand though.

Re: Patch that adds help to isearch

[Juri Linkov]

> In the patch I sent before I also added scrolling of the help
> buffer.  Scrolling of the help buffer is done by the keys the user
> has bound to scroll-other-window' and 'scroll-other-window-down'.
> It is only active when the other window is the help buffer.

Isn't an easier way to achieve this by setting `isearch-allow-scroll'
to t?

Note that isearch.el already contains:

(put 'scroll-other-window 'isearch-scroll t)
(put 'scroll-other-window-down 'isearch-scroll t)

> However the scrolling could of course be dependent on if C-h and f1
> is bound to isearch-mode-help.  A defcustom seems more easy to
> understand though.

I think it should be rather dependent on if the other window is the
help buffer created by isearch-mode-help, than if C-h and f1 is bound,
because users can bind isearch-mode-help to other keys.

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

Re: Patch that adds help to isearch

[Lennart Borgman]

Juri Linkov wrote:

>>In the patch I sent before I also added scrolling of the help
>>buffer.  Scrolling of the help buffer is done by the keys the user
>>has bound to scroll-other-window' and 'scroll-other-window-down'.
>>It is only active when the other window is the help buffer.
>>    
>>
>
>Isn't an easier way to achieve this by setting `isearch-allow-scroll'
>to t?
>  
>
Yes, of course. I just tried to minimize the impact of allowing help in 
my patch.

>I think it should be rather dependent on if the other window is the
>help buffer created by isearch-mode-help, than if C-h and f1 is bound,
>because users can bind isearch-mode-help to other keys.
>  
>
Yes, you are right.

Patch that adds help to isearch

[Lennart Borgman]

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

Some months ago we discussed help for isearch-forward/backward. I tried 
several alternatives at that time, but did not send in any patch. The 
attached patch is a simple way to give help. It simply defines C-h and 
f1 to call isearch-mode-help and allows scrolling of the help with keys 
that are bound to scroll-other-window[-down]. This was the only 
alternative I could get to work in a nice manner as far as I remember now.

I also found some small errors which I tried to correct. This was 
something with the isearch-ring but I can not remember the details right 
now. I included that also in the patch however.

I renamed search-ring-update to isearch-ring-update which seems more 
consistent. Maybe that should not be done if that breaks something?

BTW I think there may be some useful key definitions missing in the 
help. Could you for example not switch the direction with C-r? Should 
not that be added to the help?



[-- Attachment #2: isearch-20051124.path --]
[-- Type: text/plain, Size: 2827 bytes --]


Index: lisp/isearch.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.276
diff -u -r1.276 isearch.el
--- lisp/isearch.el	24 Nov 2005 09:59:11 -0000	1.276
+++ lisp/isearch.el	24 Nov 2005 17:30:35 -0000
@@ -193,7 +193,7 @@
   "Index in `regexp-search-ring' of last string reused.
 It is nil if none yet.")
 
-(defcustom search-ring-update nil
+(defcustom isearch-ring-update nil
   "*Non-nil if advancing or retreating in the search ring should cause search.
 Default value, nil, means edit the string instead."
   :type 'boolean
@@ -370,7 +370,8 @@
 
     ;; Turned off because I find I expect to get the global definition--rms.
     ;; ;; Instead bind C-h to special help command for isearch-mode.
-    ;; (define-key map "\C-h" 'isearch-mode-help)
+    (define-key map "\C-h" 'isearch-mode-help)
+    (define-key map [(f1)] 'isearch-mode-help)
 
     (define-key map "\M-n" 'isearch-ring-advance)
     (define-key map "\M-p" 'isearch-ring-retreat)
@@ -609,6 +610,12 @@
 (defun isearch-mode-help ()
   (interactive)
   (describe-function 'isearch-forward)
+  (save-excursion
+    (set-buffer "*Help*")
+    (let ((inhibit-read-only t))
+      (insert (substitute-command-keys
+               "To scroll help use \\[scroll-other-window-down] and \\[scroll-other-window].\n\n")))
+    )
   (isearch-update))
 
 \f
@@ -1702,6 +1709,18 @@
 	  ((eq search-exit-option 'edit)
 	   (apply 'isearch-unread keylist)
 	   (isearch-edit-string))
+          ;; Always scroll other window if help buffer
+          ((let ((binding (key-binding key))
+                 other-buffer-is-help)
+             (when (or (eq binding 'scroll-other-window-down)
+                       (eq binding 'scroll-other-window))
+                 (other-window 1)
+                 (setq other-buffer-is-help (equal (buffer-name) "*Help*"))
+                 (other-window -1)
+                 (when other-buffer-is-help
+                   (command-execute binding)
+                   (isearch-update)
+                   t))))
           ;; Handle a scrolling function.
           ((and isearch-allow-scroll
                 (progn (setq key (isearch-reread-key-sequence-naturally keylist))
@@ -1843,13 +1862,14 @@
 (defun isearch-ring-adjust (advance)
   ;; Helper for isearch-ring-advance and isearch-ring-retreat
   (isearch-ring-adjust1 advance)
-  (if search-ring-update
+  ;; Changed because isearch-edit-string calls isearch-push-state
+  ;; but isearch-search does not
+  (if isearch-ring-update
       (progn
 	(isearch-search)
+        (isearch-push-state)
 	(isearch-update))
-    (isearch-edit-string)
-    )
-  (isearch-push-state))
+    (isearch-edit-string)))
 
 (defun isearch-ring-advance ()
   "Advance to the next search string in the ring."

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Patch that adds help to isearch

[Richard M. Stallman]

    Some months ago we discussed help for isearch-forward/backward. I tried 
    several alternatives at that time, but did not send in any patch. The 
    attached patch is a simple way to give help. It simply defines C-h and 
    f1 to call isearch-mode-help

I don't want to make such a change now, and perhaps not ever.  Too
many control characters are special already in isearch.  (I turned off
one of them a couple of months ago.)

    I also found some small errors which I tried to correct. This was 
    something with the isearch-ring but I can not remember the details right 
    now. I included that also in the patch however.

Can you please separate those bug fixes out
and explain the bugs they fix?

    I renamed search-ring-update to isearch-ring-update which seems more 
    consistent. Maybe that should not be done if that breaks something?

I don't want to change these names now.

    BTW I think there may be some useful key definitions missing in the 
    help. Could you for example not switch the direction with C-r? Should 
    not that be added to the help?

Could you be more specific about the change you mean?

Re: Patch that adds help to isearch

[Lennart Borgman]

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

Richard M. Stallman wrote:

>    Some months ago we discussed help for isearch-forward/backward. I tried 
>    several alternatives at that time, but did not send in any patch. The 
>    attached patch is a simple way to give help. It simply defines C-h and 
>    f1 to call isearch-mode-help
>
>I don't want to make such a change now, and perhaps not ever.  Too
>many control characters are special already in isearch.  (I turned off
>one of them a couple of months ago.)
>  
>
I read in the code

    ;; Turned off because I find I expect to get the global definition--rms.
    ;; ;; Instead bind C-h to special help command for isearch-mode.
    ;; (define-key map "\C-h" 'isearch-mode-help)

I started working on that some months ago but gave up. It was rather 
complicated to stay in isearch-mode and do this and take care of 
different window configurations etc. I thought it was not worth the 
trouble and in the case of isearch-mode I also found it more helpful 
with the isearch-mode-help.

The change I proposed is simple and I do not beleive it will be any 
problem to remove it later if we decide so. However for all Emacs users 
that do not know the details of isearch-mode (like myself) I think it is 
very useful. Some keys are however used:

1) The help keys (C-h and f1). This of course blocks the global 
definition, but is this a problem here?

2) Scroll other window up or down. Keys bound to those keys are used for 
scrolling the help window, but only if the other window is displaying 
the help buffer.

I do not want to argue but I want to be clear.


>    I also found some small errors which I tried to correct. This was 
>    something with the isearch-ring but I can not remember the details right 
>    now. I included that also in the patch however.
>
>Can you please separate those bug fixes out
>and explain the bugs they fix?
>  
>
I looked at my old bug fixes but these are some months old. What I 
wanted to fix was the history handling for isearch edit. My old fix did 
not work any more so I have made a new one which I attach. This does a 
few things:

1) M-p shows the last used value saved in the history list for isearch 
(called `search-ring') when entering minibuff editing.
2) M-n shows an empty string in this case.
3) The default for editing is just the empty string, not the first 
string in the history list like now.

I might have misunderstood something here, so please test.

I believe there was a typo too. A lonely (quit ...).

>    I renamed search-ring-update to isearch-ring-update which seems more 
>    consistent. Maybe that should not be done if that breaks something?
>
>I don't want to change these names now.
>  
>
Ok. Could the isearch-mode-help be changed a bit instead? Currently it 
tells you to do

   M-x apropos RET, search-.* RET

That gives a lot of unwanted hits. I suggest to give a hint to use

   M-x customize-group RET, isearch RET

instead.

>    BTW I think there may be some useful key definitions missing in the 
>    help. Could you for example not switch the direction with C-r? Should 
>    not that be added to the help?
>
>Could you be more specific about the change you mean?
>  
>
Sorry, my mistake. It is actually the second time I do this I remember 
now. I did not find C-r in the help text because is it not in the left 
margin, but it is there.




[-- Attachment #2: isearch-hist.diff --]
[-- Type: text/plain, Size: 3919 bytes --]


Index: isearch.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/isearch.el,v
retrieving revision 1.276
diff -c -r1.276 isearch.el
*** isearch.el	24 Nov 2005 09:59:11 -0000	1.276
--- isearch.el	27 Nov 2005 22:28:09 -0000
***************
*** 1035,1041 ****
                         (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
--- 1035,1043 ----
                         (isearch-message-prefix nil nil isearch-nonincremental)
                         isearch-string
                         minibuffer-local-isearch-map nil
!                        (cons 
!                         (if isearch-regexp 'regexp-search-ring 'search-ring)
!                         1)
                         nil t)
  		      isearch-new-message
  		      (mapconcat 'isearch-text-char-description
***************
*** 1054,1068 ****
  		  isearch-word isearch-new-word))
  
  	  ;; Empty isearch-string means use default.
! 	  (if (= 0 (length isearch-string))
! 	      (setq isearch-string (or (car (if isearch-regexp
! 						regexp-search-ring
! 					      search-ring))
! 				       "")
! 
! 		    isearch-message
! 		    (mapconcat 'isearch-text-char-description
! 			       isearch-string ""))
  	    ;; This used to set the last search string,
  	    ;; but I think it is not right to do that here.
  	    ;; Only the string actually used should be saved.
--- 1056,1071 ----
  		  isearch-word isearch-new-word))
  
  	  ;; Empty isearch-string means use default.
!           ;; Why should it mean default?
! ;; 	  (if (= 0 (length isearch-string))
! ;; 	      (setq isearch-string (or (car (if isearch-regexp
! ;; 						regexp-search-ring
! ;; 					      search-ring))
! ;; 				       "")
! 
! ;; 		    isearch-message
! ;; 		    (mapconcat 'isearch-text-char-description
! ;; 			       isearch-string ""))
  	    ;; This used to set the last search string,
  	    ;; but I think it is not right to do that here.
  	    ;; Only the string actually used should be saved.
***************
*** 1081,1091 ****
  	      ;; The search done message is confusing when the string
  	      ;; is empty, so erase it.
  	      (if (equal isearch-string "")
! 		  (message "")))))
  
!     (quit  ; handle abort-recursive-edit
!      (isearch-abort)  ;; outside of let to restore outside global values
!      )))
  
  (defun isearch-nonincremental-exit-minibuffer ()
    (interactive)
--- 1084,1094 ----
  	      ;; The search done message is confusing when the string
  	      ;; is empty, so erase it.
  	      (if (equal isearch-string "")
! 		  (message ""))))
  
!         (quit  ; handle abort-recursive-edit
!          (isearch-abort)  ;; outside of let to restore outside global values
!          ))))
  
  (defun isearch-nonincremental-exit-minibuffer ()
    (interactive)
***************
*** 1833,1842 ****
  	()
        (set yank-pointer-name
  	   (setq yank-pointer
! 		 (mod (+ (or yank-pointer 0)
! 			 (if advance -1 1))
! 		      length)))
!       (setq isearch-string (nth yank-pointer ring)
  	    isearch-message (mapconcat 'isearch-text-char-description
  				       isearch-string "")))))
  
--- 1836,1847 ----
  	()
        (set yank-pointer-name
  	   (setq yank-pointer
!                  (if yank-pointer
!                      (mod (+ yank-pointer
!                              (if advance -1 1))
!                           length)
!                    (if advance -1 0))))
!       (setq isearch-string (if (< yank-pointer 0) "" (nth yank-pointer ring))
  	    isearch-message (mapconcat 'isearch-text-char-description
  				       isearch-string "")))))
  

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Patch that adds help to isearch

[Lennart Borgman]

Lennart Borgman wrote:

>***************
>*** 1081,1091 ****
>  	      ;; The search done message is confusing when the string
>  	      ;; is empty, so erase it.
>  	      (if (equal isearch-string "")
>! 		  (message "")))))
>  
>!     (quit  ; handle abort-recursive-edit
>!      (isearch-abort)  ;; outside of let to restore outside global values
>!      )))
>  
>  (defun isearch-nonincremental-exit-minibuffer ()
>    (interactive)
>--- 1084,1094 ----
>  	      ;; The search done message is confusing when the string
>  	      ;; is empty, so erase it.
>  	      (if (equal isearch-string "")
>! 		  (message ""))))
>  
>!         (quit  ; handle abort-recursive-edit
>!          (isearch-abort)  ;; outside of let to restore outside global values
>!          ))))
>  
>
Oops! There is an ")" too much after quit here.

Re: Patch that adds help to isearch

[Juri Linkov]

>     Some months ago we discussed help for isearch-forward/backward.
>     I tried several alternatives at that time, but did not send in
>     any patch.  The attached patch is a simple way to give help.
>     It simply defines C-h and f1 to call isearch-mode-help
>
> I don't want to make such a change now, and perhaps not ever.  Too
> many control characters are special already in isearch.  (I turned off
> one of them a couple of months ago.)

What about enabling isearch-mode-help only when the search string is empty?

I understand the need to exit the search with C-h when the search
string is not empty like in the case of `C-s some-function-name C-h f RET'
to call `describe-function' on the found function name.  But I don't see
the need to exit the search with C-h just after C-s as in `C-s C-h f'.

OTOH, I can imagine users (especially novices) after typing C-s and seeing
the prompt `I-search: ' wanting the help about what to do next.  It is
natural for them to type C-h in this case to see the Help buffer from
`isearch-mode-help'.

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

Re: Patch that adds help to isearch

[Richard M. Stallman]

    What about enabling isearch-mode-help only when the search string is empty?

We could consider that after the release.

Re: Patch that adds help to isearch

[Lennart Borgman]

Richard M. Stallman wrote:

>    What about enabling isearch-mode-help only when the search string is empty?
>
>We could consider that after the release.
>  
>
There was just somebody on EmacsWiki who wanted to bind 
isearch-repeat-forward to f3. It seems like the help is needed. See

    http://www.emacswiki.org/cgi-bin/wiki/SearchAtPoint

Re: Patch that adds help to isearch

[Juri Linkov]

>>    What about enabling isearch-mode-help only when the search string is empty?
>>
>>We could consider that after the release.
>>
> There was just somebody on EmacsWiki who wanted to bind
> isearch-repeat-forward to f3. It seems like the help is needed. See
>
>    http://www.emacswiki.org/cgi-bin/wiki/SearchAtPoint

I am aware of the development of the code on this page.  I don't think
the need for isearch-mode-help has any relation to this.  f3 is an attempt
to use the same key f3 that activated the search, in isearch mode too to
repeat the search  So you could use successive `f3 f3 f3 ...' to start
and repeat the search.

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