bug#10998: Allow movements in bookmark-bmenu-search.

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#10998: Allow movements in bookmark-bmenu-search.

Thierry Volpiatto-2
Hi,
there is no cursor in the prompt of bookmark-bmenu-search, and it is not
possible to move and edit it actually, this patch allow this.

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/bookmark.el b/lisp/bookmark.el
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -169,6 +169,11 @@
   :group 'bookmark
   :version "22.1")
 
+(defface bookmark-cursor
+  '((t (:foreground "green")))
+  "Face for cursor color in `bookmark-bmenu-search' prompt."
+  :group 'bookmark)
+
 
 ;;; No user-serviceable parts beyond this point.
 
@@ -2008,20 +2013,57 @@
 ;; Store keyboard input for incremental search.
 (defvar bookmark-search-pattern)
 
+(defun bookmark-set-cursor-in-prompt (str pos)
+  "Add a cursor in string STR at index position POS."
+  (setq pos (min index (1- (length tmp-list))))
+  (when (not (string= str ""))
+    (let* ((real-index (- (1- (length tmp-list)) pos))
+           (cur-str (substring str real-index (1+ real-index))))
+      (concat (substring str 0 real-index)
+              (propertize cur-str 'display
+                          (if (= index (length tmp-list))
+                              (concat
+                               (propertize "|" 'face 'bookmark-cursor)
+                               cur-str)
+                            (concat
+                             cur-str
+                             (propertize "|" 'face 'bookmark-cursor))))
+              (substring str (1+ real-index))))))
+
 (defun bookmark-read-search-input ()
   "Read each keyboard input and add it to `bookmark-search-pattern'."
   (let ((prompt       (propertize "Pattern: " 'face 'minibuffer-prompt))
         ;; (inhibit-quit t) ; inhibit-quit is evil.  Use it with extreme care!
-        (tmp-list     ()))
+        (tmp-list     ())
+        (index        0))
     (while
-        (let ((char (read-key (concat prompt bookmark-search-pattern))))
+        (let ((char (read-key (concat prompt (bookmark-set-cursor-in-prompt
+                                              bookmark-search-pattern index)))))
           (case char
             ((?\e ?\r) nil) ; RET or ESC break the search loop.
             (?\C-g (setq bookmark-quit-flag t) nil)
-            (?\d (pop tmp-list) t) ; Delete last char of pattern with DEL
+            (?\d (with-no-warnings ; Delete last char of pattern with DEL.
+                   (pop (nthcdr index tmp-list))) t)
+            ;; Movements in minibuffer.
+            (?\C-b                         ; backward-char.
+             (setq index (min (1+ index) (length tmp-list))) t)
+            (?\C-f                         ; forward-char.
+             (setq index (max (1- index) 0)) t)
+            (?\C-a                         ; move bol.
+             (setq index (length tmp-list)) t)
+            (?\C-e                         ; move eol.
+             (setq index 0) t)
+            (?\C-k
+             (kill-new (substring bookmark-search-pattern
+                                  (- (length tmp-list) index)))
+             (setq tmp-list (nthcdr index tmp-list)) (setq index 0) t)
+            (?\C-y
+             (let ((str (car kill-ring)))
+               (loop for char across str
+                     do (push char (nthcdr index tmp-list)))) t)
             (t
              (if (characterp char)
-                 (push char tmp-list)
+                 (push char (nthcdr index tmp-list))
                (setq unread-command-events
                      (nconc (mapcar 'identity
                                     (this-single-command-raw-keys))
--8<---------------cut here---------------end--------------->8---

--
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997




Reply | Threaded
Open this post in threaded view
|

bug#10998: Allow movements in bookmark-bmenu-search.

Stefan Monnier
> there is no cursor in the prompt of bookmark-bmenu-search, and it is not
> possible to move and edit it actually, this patch allow this.

Couldn't we fix that another way, using a real minibuffer, with an
after-change-function to update the set of matching entries?


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#10998: Allow movements in bookmark-bmenu-search.

Thierry Volpiatto-2
Stefan Monnier <[hidden email]> writes:

>> there is no cursor in the prompt of bookmark-bmenu-search, and it is not
>> possible to move and edit it actually, this patch allow this.
>
> Couldn't we fix that another way, using a real minibuffer, with an
> after-change-function to update the set of matching entries?
I don't know much about after-change-functions, but I guess yes it would
be possible.
However it will change all the logic of this code and need a complete
rewrite.

BTW the patch sent contain free-variables, it is fixed here.

--
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997



Reply | Threaded
Open this post in threaded view
|

bug#10998: Allow movements in bookmark-bmenu-search.

Stefan Monnier
>>> there is no cursor in the prompt of bookmark-bmenu-search, and it is not
>>> possible to move and edit it actually, this patch allow this.
>> Couldn't we fix that another way, using a real minibuffer, with an
>> after-change-function to update the set of matching entries?
> I don't know much about after-change-functions, but I guess yes it
> would be possible.  However it will change all the logic of this code
> and need a complete rewrite.

Indeed, but it will make all cursor motion commands work "as usual"
rather than hard-coding C-f, C-b etc...


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#10998: Allow movements in bookmark-bmenu-search.

Thierry Volpiatto-2
Hi Stefan,

Stefan Monnier <[hidden email]> writes:

>>>> there is no cursor in the prompt of bookmark-bmenu-search, and it is not
>>>> possible to move and edit it actually, this patch allow this.
>>> Couldn't we fix that another way, using a real minibuffer, with an
>>> after-change-function to update the set of matching entries?
>> I don't know much about after-change-functions, but I guess yes it
>> would be possible.  However it will change all the logic of this code
>> and need a complete rewrite.
>
> Indeed, but it will make all cursor motion commands work "as usual"
> rather than hard-coding C-f, C-b etc...
Agree, but I will not have the time to work on this, at least now.
I suggest you use the code I sent you (after 24.1 I guess) waiting
something better. (I have a better version here).
You can see how it works by using last (dev version) of ioccur.el.

Thanks.

--
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997



Reply | Threaded
Open this post in threaded view
|

bug#10998: Allow movements in bookmark-bmenu-search

Karl Fogel
In reply to this post by Thierry Volpiatto-2
The code has changed somewhat since the original patch -- it now uses
`pcase' instead of `case', for example (and thus `_' instead of `t'),
ever since [hidden email]-20120710115154-012cs2xbndtlpvh1.

Furthermore, in http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10998#11
Thierry said "the patch sent contain free-variables, it is fixed here"
but there was no new patch attached.

So, I've adjusted the original patch (see below) and done some light
testing, but I'm not confident enough in this to commit it yet.
Thierry, what were those free variables?  And can you test this to make
sure it behaves as your original patch did?

I agree with Stefan that this is not an ideal solution, by the way, and
have left a comment in the patch below to that effect.  It might still
be better than the status quo, though.

-Karl

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog 2012-10-01 03:44:59 +0000
+++ lisp/ChangeLog 2012-10-01 04:31:27 +0000
@@ -1,3 +1,11 @@
+2012-10-01  Karl Fogel  <[hidden email]>
+
+ * bookmark.el (bookmark-cursor, bookmark-set-cursor-in-prompt):
+ New face, new function.
+ (bookmark-read-search-input): Set the cursor and listen to more
+ command characters.
+ (Bug#10998)
+
 2012-10-01  Karl Fogel  <[hidden email]>
 
  * bookmark.el (bookmark-version-control): Give tags in the

=== modified file 'lisp/bookmark.el'
--- lisp/bookmark.el 2012-10-01 04:15:48 +0000
+++ lisp/bookmark.el 2012-10-01 04:34:25 +0000
@@ -168,6 +168,11 @@
   :group 'bookmark
   :version "22.1")
 
+(defface bookmark-cursor
+  '((t (:foreground "green")))
+  "Face for cursor color in `bookmark-bmenu-search' prompt."
+  :group 'bookmark)
+
 
 ;;; No user-serviceable parts beyond this point.
 
@@ -2013,20 +2018,62 @@
 ;; Store keyboard input for incremental search.
 (defvar bookmark-search-pattern)
 
+(defun bookmark-set-cursor-in-prompt (str pos)
+  "Add a cursor in string STR at index position POS."
+  (setq pos (min index (1- (length tmp-list))))
+  (when (not (string= str ""))
+    (let* ((real-index (- (1- (length tmp-list)) pos))
+           (cur-str (substring str real-index (1+ real-index))))
+      (concat (substring str 0 real-index)
+              (propertize cur-str 'display
+                          (if (= index (length tmp-list))
+                              (concat
+                               (propertize "|" 'face 'bookmark-cursor)
+                               cur-str)
+                            (concat
+                             cur-str
+                             (propertize "|" 'face 'bookmark-cursor))))
+              (substring str (1+ real-index))))))
+
 (defun bookmark-read-search-input ()
   "Read each keyboard input and add it to `bookmark-search-pattern'."
   (let ((prompt       (propertize "Pattern: " 'face 'minibuffer-prompt))
         ;; (inhibit-quit t) ; inhibit-quit is evil.  Use it with extreme care!
-        (tmp-list     ()))
+        (tmp-list     ())
+        (index        0))
     (while
-        (let ((char (read-key (concat prompt bookmark-search-pattern))))
+        (let ((char (read-key
+                     (concat prompt (bookmark-set-cursor-in-prompt
+                                     bookmark-search-pattern index)))))
+          ;; FIXME: This is kind of a kluge.  The right way to do this
+          ;; would be to use a real minibuffer, maybe with an
+          ;; after-change-function to update the set of matching
+          ;; entries each time.
           (pcase char
             ((or ?\e ?\r) nil) ; RET or ESC break the search loop.
             (?\C-g (setq bookmark-quit-flag t) nil)
-            (?\d (pop tmp-list) t) ; Delete last char of pattern with DEL
+            (?\d (with-no-warnings ; Delete last char of pattern with DEL.
+                   (pop (nthcdr index tmp-list))) t)
+            ;; Movements in minibuffer.
+            (?\C-b                         ; backward-char.
+             (setq index (min (1+ index) (length tmp-list))) t)
+            (?\C-f                         ; forward-char.
+             (setq index (max (1- index) 0)) t)
+            (?\C-a                         ; move bol.
+             (setq index (length tmp-list)) t)
+            (?\C-e                         ; move eol.
+             (setq index 0) t)
+            (?\C-k
+             (kill-new (substring bookmark-search-pattern
+                                  (- (length tmp-list) index)))
+             (setq tmp-list (nthcdr index tmp-list)) (setq index 0) t)
+            (?\C-y
+             (let ((str (car kill-ring)))
+               (loop for char across str
+                     do (push char (nthcdr index tmp-list)))) t)
             (_
              (if (characterp char)
-                 (push char tmp-list)
+                 (push char (nthcdr index tmp-list))
                (setq unread-command-events
                      (nconc (mapcar 'identity
                                     (this-single-command-raw-keys))




Reply | Threaded
Open this post in threaded view
|

bug#10998: Allow movements in bookmark-bmenu-search

Thierry Volpiatto-2
In reply to this post by Thierry Volpiatto-2
Karl Fogel <[hidden email]> writes:

> The code has changed somewhat since the original patch -- it now uses
> `pcase' instead of `case', for example (and thus `_' instead of `t'),
> ever since [hidden email]-20120710115154-012cs2xbndtlpvh1.
>
> Furthermore, in http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10998#11
> Thierry said "the patch sent contain free-variables, it is fixed here"
> but there was no new patch attached.
It is fixed "here" mean I fixed this on my own version of bookmark but
didn't send any patch because no body was interested by this change.
It is working with no problem since then.

> So, I've adjusted the original patch (see below) and done some light
> testing, but I'm not confident enough in this to commit it yet.
> Thierry, what were those free variables?
Don't remember that was more than one year ago.


> And can you test this to make sure it behaves as your original patch
> did?
I have not anymore this patch.
You will find bookmark-extensions.el at:
https://github.com/thierryvolpiatto/emacs-bmk-ext.git
Install it and try cursor movements to see if it behave like your patch.

Here the function:


#+BEGIN_SRC lisp
(defun bookmark-read-search-input ()
  "Read each keyboard input and add it to `bookmark-search-pattern'."
  (let ((prompt       (propertize "Pattern: " 'face 'minibuffer-prompt))
        (tmp-list     ())
        (index        0))
    (while
        (let ((char (read-key (concat prompt (bookmark-set-cursor-in-prompt
                                              bookmark-search-pattern
                                              index tmp-list)))))
          (case char
            ((?\e ?\r) nil) ; RET or ESC break the search loop.
            (?\C-g (setq bookmark-quit-flag t) nil)
            (?\d (with-no-warnings ; Delete last char of pattern with DEL.
                   (pop (nthcdr index tmp-list))) t)
            ;; Movements in minibuffer.
            (?\C-b                         ; backward-char.
             (setq index (min (1+ index) (length tmp-list))) t)
            (?\C-f                         ; forward-char.
             (setq index (max (1- index) 0)) t)
            (?\C-a                         ; move bol.
             (setq index (length tmp-list)) t)
            (?\C-e                         ; move eol.
             (setq index 0) t)
            (?\C-k
             (kill-new (substring bookmark-search-pattern
                                  (- (length tmp-list) index)))
             (setq tmp-list (nthcdr index tmp-list)) (setq index 0) t)
            (?\C-y
             (let ((str (car kill-ring)))
               (loop for char across str
                     do (push char (nthcdr index tmp-list)))) t)
            (t
             (if (characterp char)
                 (push char (nthcdr index tmp-list))
               (setq unread-command-events
                     (nconc (mapcar 'identity
                                    (this-single-command-raw-keys))
                            unread-command-events))
               nil))))
      (setq bookmark-search-pattern
            (apply 'string (reverse tmp-list))))))

#+END_SRC

Note: I don't maintain anymore bookmark-extensions.el, only small
bugfixes for my personal usage (e.g recently `bookmark-write-file')

> I agree with Stefan that this is not an ideal solution, by the way, and
> have left a comment in the patch below to that effect.
Keep in mind that this is not a real minibuffer.


--
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997




Reply | Threaded
Open this post in threaded view
|

bug#10998: Allow movements in bookmark-bmenu-search

Stefan Monnier
>> I agree with Stefan that this is not an ideal solution, by the way, and
>> have left a comment in the patch below to that effect.
> Keep in mind that this is not a real minibuffer.

AFAICT the fact that it's not a minibuffer is an implementation detail.
IOW it currently isn't a minibuffer, but it could/should be changed to
be one.
See example below.


        Stefan


(defun bookmark-bmenu-search ()
  "Incremental search of bookmarks, hiding the non-matches as we go."
  (interactive)
  (let ((bmk (bookmark-bmenu-bookmark))
        (timer nil))
    (unwind-protect
        (minibuffer-with-setup-hook
            (lambda ()
              (setq timer (run-with-idle-timer
                           bookmark-search-delay 'repeat
                           #'(lambda (buf)
                               (with-current-buffer buf
                                 (bookmark-bmenu-filter-alist-by-regexp
                                  (minibuffer-contents))))
                           (current-buffer))))
          (read-string "Pattern: ")
          (when timer (cancel-timer timer) (setq timer nil)))
      (when timer ;; Signalled an error or a `quit'.
        (cancel-timer timer)
        (bookmark-bmenu-list)
        (bookmark-bmenu-goto-bookmark bmk)))))



Reply | Threaded
Open this post in threaded view
|

bug#10998: Allow movements in bookmark-bmenu-search

Thierry Volpiatto-2
Stefan Monnier <[hidden email]> writes:

>>> I agree with Stefan that this is not an ideal solution, by the way, and
>>> have left a comment in the patch below to that effect.
>> Keep in mind that this is not a real minibuffer.
>
> AFAICT the fact that it's not a minibuffer is an implementation detail.
> IOW it currently isn't a minibuffer, but it could/should be changed to
> be one.
> See example below.
Very nice use of `minibuffer-with-setup-hook' cool.

--
  Thierry
Get my Gnupg key:
gpg --keyserver pgp.mit.edu --recv-keys 59F29997



Reply | Threaded
Open this post in threaded view
|

bug#10998: Allow movements in bookmark-bmenu-search

Stefan Monnier
In reply to this post by Stefan Monnier
> See example below.

Installed,


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#10998: Allow movements in bookmark-bmenu-search

Stefan Kangas
In reply to this post by Thierry Volpiatto-2
Stefan Monnier <[hidden email]> writes:
>> See example below.
>
> Installed,

I think that fixed this bug; we now have a minibuffer.

Can we close it?

Thanks,
Stefan Kangas



Reply | Threaded
Open this post in threaded view
|

bug#10998: Allow movements in bookmark-bmenu-search

Noam Postavsky
tags 10998 fixed
close 10998 24.3
quit

Stefan Kangas <[hidden email]> writes:
> Stefan Monnier <[hidden email]> writes:
>>> See example below.
>>
>> Installed,

Seems to be [1: d83ef97].

> I think that fixed this bug; we now have a minibuffer.
>
> Can we close it?

No news is generally good news, so let's assume yes.

[1: d83ef97]: 2012-10-01 22:47:12 -0400
  * lisp/bookmark.el (bookmark-search-pattern): Remove var. (bookmark-read-search-input): Remove function. (bookmark-bmenu-search): Reimplement using a minibuffer.
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=d83ef9762efd4efe833c0c9687b057d6b62cdcb7