bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

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

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Ramesh Nedunchezian

28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'.

Commentary in `diff-hl' says

    ;; `diff-hl-mode' highlights uncommitted changes on the side of the
    ;; window (using the fringe, by default), allows you to jump between
    ;; the hunks and revert them selectively.

    ;; Provided commands:
    ;;
    ;; `diff-hl-diff-goto-hunk'  C-x v =
    ;; `diff-hl-revert-hunk'     C-x v n
    ;; `diff-hl-previous-hunk'   C-x v [
    ;; `diff-hl-next-hunk'       C-x v ]
    ;;
    ;; The mode takes advantage of `smartrep' if it is installed.

FWIW, `smartrep' is not part of GNU Emacs or GNU ELPA but available
elsewhere.  (I install it via MELPA)

Now, that `repeat-mode' is part of GNU Emacs, I think this dependency
on `smartrep' can be removed.

FWIW, this is what I have in my `.emacs'.

    (progn
      (defvar diff-hl--repeat-map
        (let
            ((map
              (make-sparse-keymap)))
          map))
      (message "Installed %s" 'diff-hl--repeat-map)
      (cl-loop for
               (cmd . key)
               in
               '((diff-hl-diff-goto-hunk . "=")
                 (diff-hl-revert-hunk . "n")
                 (diff-hl-previous-hunk . "[")
                 (diff-hl-next-hunk . "]"))
               do
               (define-key diff-hl--repeat-map key cmd)
               (put cmd 'repeat-map 'diff-hl--repeat-map)))

----------------

In GNU Emacs 28.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version 3.24.24, cairo version 1.16.0)
 of 2021-03-20 built on debian
Repository revision: 31544bc908d35bff513450bc4bea1d0283a7ddb0
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12010000
System Description: Debian GNU/Linux bullseye/sid



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Juri Linkov-2
>>        (cl-loop for
>>       (cmd . key)
>>       in
>>       '((diff-hl-diff-goto-hunk . "=")
>> (diff-hl-revert-hunk . "n")
>> (diff-hl-previous-hunk . "[")
>> (diff-hl-next-hunk . "]"))
>>       do
>>       (define-key diff-hl--repeat-map key cmd)
>>       (put cmd 'repeat-map 'diff-hl--repeat-map)))
>
> Try this alternative too, seems shorter:
>
> (map-keymap
>  (lambda (_key cmd)
>    (put cmd 'repeat-map 'diff-hl-command-map))
>  diff-hl-command-map)
>
> But either version (together with enabling repeat-mode) seems to break
> diff-hl-revert-hunk: as soon as it reaches the the y-or-n prompt, and I try
> to answer 'n', it enters the repeat loop again, instead of sending the
> result to the command.

Could you provide a minimal test case?  I tried with:

#+begin_src emacs-lisp
(defun hl-test ()
  (interactive)
  (message "OK")
  (y-or-n-p "Yes? "))

(defvar hl-repeat-map
  (let ((map (make-sparse-keymap)))
    (define-key map (kbd "r") 'hl-test)
    map)
  "Keymap to repeat hl-test.")
(put 'hl-test 'repeat-map 'hl-repeat-map)
#+end_src

Then typing `M-x hl-test RET', and `y r y r y r ...' keeps the loop.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Dmitry Gutov
Hi Juri,

On 05.04.2021 23:43, Juri Linkov wrote:

> Could you provide a minimal test case?  I tried with:
>
> #+begin_src emacs-lisp
> (defun hl-test ()
>    (interactive)
>    (message "OK")
>    (y-or-n-p "Yes? "))
>
> (defvar hl-repeat-map
>    (let ((map (make-sparse-keymap)))
>      (define-key map (kbd "r") 'hl-test)
>      map)
>    "Keymap to repeat hl-test.")
> (put 'hl-test 'repeat-map 'hl-repeat-map)
> #+end_src
>
> Then typing `M-x hl-test RET', and `y r y r y r ...' keeps the loop.

Here you go:

(defun hl-test ()
   (interactive)
   (message "OK")
   (message "result: %s"
            (y-or-n-p "Yes? ")))

(defvar hl-repeat-map
   (let ((map (make-sparse-keymap)))
     (define-key map (kbd "n") 'hl-test) ; Note the changed key binding.
     map)
   "Keymap to repeat hl-test.")

(put 'hl-test 'repeat-map 'hl-repeat-map)

To try it:

1. M-x hl-test.
2. Press 'n' a few times.

Expected behavior:

It alternates between the prompt "Yes? " and message "result: nil".

Actual behavior:

It enters some sort of recursive state, only showing the prompt. I have
to press 'y' a bunch of times to get out of it.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Juri Linkov-2
> (defun hl-test ()
>   (interactive)
>   (message "OK")
>   (message "result: %s"
>            (y-or-n-p "Yes? ")))
>
> (defvar hl-repeat-map
>   (let ((map (make-sparse-keymap)))
>     (define-key map (kbd "n") 'hl-test) ; Note the changed key binding.
>     map)
>   "Keymap to repeat hl-test.")
>
> (put 'hl-test 'repeat-map 'hl-repeat-map)
>
> To try it:
>
> 1. M-x hl-test.
> 2. Press 'n' a few times.
>
> Expected behavior:
>
> It alternates between the prompt "Yes? " and message "result: nil".
>
> Actual behavior:
>
> It enters some sort of recursive state, only showing the prompt. I have to
> press 'y' a bunch of times to get out of it.

Thanks for the detailed test case.  Now it's fixed in 580c4c6510.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Dmitry Gutov
On 08.04.2021 21:57, Juri Linkov wrote:
> Thanks for the detailed test case.  Now it's fixed in 580c4c6510.

Thanks! repeat-mode is looking good now (*).

I've added integration for it to diff-hl master, and with that we can
close this issue.

Thanks all.

(*) Though the first impression, in comparison, was that it is too
chatty. The hints are definitely helpful for discovery at first, though.

Maybe something like this would be an improvement? Experimental code
warning.

diff --git a/lisp/repeat.el b/lisp/repeat.el
index b3c58f2f81..e704e4da56 100644
--- a/lisp/repeat.el
+++ b/lisp/repeat.el
@@ -404,7 +404,7 @@ repeat-post-hook
                                         (key-description repeat-exit-key))
                               ""))))
                  (if (current-message)
-                    (message "%s [%s]" (current-message) mess)
+                    (message #("%s [%s]" 3 7 (face deemphasized))
(current-message) mess)
                    (message mess))))

              ;; Adding an exit key



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Ramesh Nedunchezian


On 10/04/21 7:10 am, Dmitry Gutov wrote:

> (*) Though the first impression, in comparison, was that it is too chatty. The hints are definitely helpful for discovery at first, though.
>
> Maybe something like this would be an improvement? Experimental code warning.
>
> diff --git a/lisp/repeat.el b/lisp/repeat.el
> index b3c58f2f81..e704e4da56 100644
> --- a/lisp/repeat.el
> +++ b/lisp/repeat.el
> @@ -404,7 +404,7 @@ repeat-post-hook
>                                         (key-description repeat-exit-key))
>                               ""))))
>                  (if (current-message)
> -                    (message "%s [%s]" (current-message) mess)
> +                    (message #("%s [%s]" 3 7 (face deemphasized)) (current-message) mess)
>                    (message mess))))
>
>              ;; Adding an exit key

I created a repeat map for rectangle commands, and the echo area
becomes much more chattier. See
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=47688.

It seems to me that repeat-mode is essentially a poor man's hydra.
May be the the symbol that holds the repeat map can specify a
`:help'-er property.

This `:help'-er can either

(a) give a fancy help string which the `repet-mode' can display in a
    pop-up

or

(b) the helper itself can prepare the string and arrange for providing
    a pop-up.

What I am saying is let the repeat map provide it's own `:help'-er
which the `repeat-mode' can hook in to.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Juri Linkov-2
> It seems to me that repeat-mode is essentially a poor man's hydra.

It is.  It provides only the basic functionality.
Don't expect many fancy things from repeat-mode.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Dmitry Gutov
In reply to this post by Dmitry Gutov
On 11.04.2021 01:58, Juri Linkov wrote:
> I can't find the face named 'deemphasized'.

Sorry.

Apparently, it's a face that only the tango-plus theme defines (unwisely).

It simply looks like less contrasting text. Try 'shadow' instead, it's
about the same.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Ramesh Nedunchezian
In reply to this post by Juri Linkov-2

On 11/04/21 4:29 am, Juri Linkov wrote:
>> It seems to me that repeat-mode is essentially a poor man's hydra.
>
> It is.  It provides only the basic functionality.
> Don't expect many fancy things from repeat-mode.

The code below is pulled out from the existing code i.e., it doesn't
add any additional layer of complexity or does anything fanciful.  But
it does add a bit of extensibility.

This is what I was suggesting:

    (defvar repeat-mode-helper
      (defun repeat-mode-helper (map)
        (let (keys)
          (message "Coming here")
          (map-keymap (lambda (key _) (push key keys)) map)
          (let ((mess (format-message
                       "Repeat with %s%s"
                       (mapconcat (lambda (key)
                                    (key-description (vector key)))
                                  keys ", ")
                       (if repeat-exit-key
                           (format ", or exit with %s"
                                   (key-description repeat-exit-key))
                         ""))))
            (if (current-message)
                (message "%s [%s]" (current-message) mess)
              (message mess))))))


And in `repeat-post-hook', do something like

    ;; Messaging
    (unless prefix-arg
      (funcall (or (get rep-sym 'help) repeat-mode-helper) rep-map))


For those who don't want hints, they can use

    (setq repeat-mode-helper #'ignore)

For those who want hints, but do /not/ want the hints hogging the echo
area, they could have their own custom helper like the one above,
after replacing

        (message mess)

with

        (tooltip-show mess)

--------------------------------

I would also appreciate if you could assess adding the exit key as a
property to the repeat mode symbol.





Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Juri Linkov-2
> This is what I was suggesting:
>
>     (defvar repeat-mode-helper
>       (defun repeat-mode-helper (map)
>[...]
>
> For those who don't want hints, they can use
>
>     (setq repeat-mode-helper #'ignore)

Thanks, this is a good idea.  Maybe the name is not the best one
since the word "helper" often means a place that provides some
utility functions.

Using the Emacs terminology, a better name would be 'repeat-mode-echo'.
I'll add such option soon.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Juri Linkov-2
In reply to this post by Ramesh Nedunchezian
> For those who don't want hints, they can use
>
>     (setq repeat-mode-helper #'ignore)
>
> For those who want hints, but do /not/ want the hints hogging the echo
> area, they could have their own custom helper like the one above,
> after replacing
>
> (message mess)
>
> with
>
> (tooltip-show mess)

This is implemented now, so you can customize it to disable messaging,
to do truncation of long messages, or to use tooltip-show, etc.

> I would also appreciate if you could assess adding the exit key as a
> property to the repeat mode symbol.

The exit key specific to some keymap can be easily added to that keymap.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Juri Linkov-2
In reply to this post by Dmitry Gutov
>> I can't find the face named 'deemphasized'.
>
> Sorry.
>
> Apparently, it's a face that only the tango-plus theme defines (unwisely).
>
> It simply looks like less contrasting text. Try 'shadow' instead, it's
> about the same.

I'm not sure about deemphasizing by default.  But now it's easy to
customize this, and add any emphazing.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Dmitry Gutov
On 12.04.2021 19:17, Juri Linkov wrote:

>>> I can't find the face named 'deemphasized'.
>>
>> Sorry.
>>
>> Apparently, it's a face that only the tango-plus theme defines (unwisely).
>>
>> It simply looks like less contrasting text. Try 'shadow' instead, it's
>> about the same.
>
> I'm not sure about deemphasizing by default.  But now it's easy to
> customize this, and add any emphazing.

Thanks.

Consider adding a repeat-mode-echo option which follows what Smartrep
does: instead of adding to the echo area, it just highlights the changed
state in the mode line (basically a long mode lighter saying
"========SMARTREP=======") and also colors the mode-line green.

IME that works well as an indicator that we're in a "special" state.

Though perhaps we could use a smaller text (like "[REPEAT]") and only
color the text inside the brackets, rather than the whole mode-line.
Could use some experimentation.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Kévin Le Gouguec
Dmitry Gutov <[hidden email]> writes:

> Consider adding a repeat-mode-echo option which follows what Smartrep
> does: instead of adding to the echo area, it just highlights the
> changed state in the mode line (basically a long mode lighter saying
> "========SMARTREP=======") and also colors the mode-line green.
>
> IME that works well as an indicator that we're in a "special" state.
>
> Though perhaps we could use a smaller text (like "[REPEAT]") and only
> color the text inside the brackets, rather than the whole
> mode-line. Could use some experimentation.

An entry in mode-line-modes, à la compilation-in-progress, would be
lovely IMO.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Juri Linkov-2
In reply to this post by Ramesh Nedunchezian
>> Consider adding a repeat-mode-echo option which follows what Smartrep
>> does: instead of adding to the echo area, it just highlights the
>> changed state in the mode line (basically a long mode lighter saying
>> "========SMARTREP=======") and also colors the mode-line green.
>>
>> IME that works well as an indicator that we're in a "special" state.
>>
>> Though perhaps we could use a smaller text (like "[REPEAT]") and only
>> color the text inside the brackets, rather than the whole
>> mode-line. Could use some experimentation.
>
> An entry in mode-line-modes, à la compilation-in-progress, would be
> lovely IMO.

compilation-in-progress is a good example of an unobtrusive indicator
in the mode line.  Now added to repeat-mode.

PS: like your message-subject-re-regexp turned the subject into
"Re: peat lambda" on emacs-devel, it removed the bug number
from the subject here :)



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Juri Linkov-2
> Thanks!  Was this bit intentional?
>
>> ;;;###autoload
>> (put 'mode-line-defining-kbd-macro 'risky-local-variable t)

Thanks for noticing this, I stole this from bindings.el,
but now that repeat-echo-mode-line-string is highlighted
correctly in the mode-line, I wonder if such property is needed
for repeat-echo-mode-line-string at all?  And why it was needed
for mode-line-defining-kbd-macro to keep its text properties?

After removing this line from bindings.el:
  (put 'mode-line-defining-kbd-macro 'risky-local-variable t)
mode-line-defining-kbd-macro loses its text property
face=font-lock-warning-face, but repeat-echo-mode-line-string keeps it.

This is explained in (info "(elisp) Properties in Mode"),
but I don't understand why text properties are preserved for
repeat-echo-mode-line-string without ‘risky-local-variable’ property?

> Also, AFAICT the indication (whether in the mode-line or in the echo
> area) only shows up once I start repeating a key (e.g. after C-x o o),
> not when I first hit a repeatable key (e.g. after C-x o).

Strange, I see it immediately after C-x o.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Juri Linkov-2
>> Also, AFAICT the indication (whether in the mode-line or in the echo
>> area) only shows up once I start repeating a key (e.g. after C-x o o),
>> not when I first hit a repeatable key (e.g. after C-x o).
>
> Strange, I see it immediately after C-x o.

Could you please try to move 'force-mode-line-update' out of condition,
so it updates the mode-line on activating the repeat map as well:

#+begin_src emacs-lisp
(defun repeat-echo-mode-line (map)
  "Display the repeat indicator in the mode line."
  (if map
      (unless (assq 'repeat-in-progress mode-line-modes)
        (add-to-list 'mode-line-modes (list 'repeat-in-progress
                                            repeat-echo-mode-line-string))))
  (force-mode-line-update t))
#+end_src

Maybe your mode-line is not updated immediately?



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Juri Linkov-2
In reply to this post by Juri Linkov-2
>>> Also, AFAICT the indication (whether in the mode-line or in the echo
>>> area) only shows up once I start repeating a key (e.g. after C-x o o),
>>> not when I first hit a repeatable key (e.g. after C-x o).
>>
>> Strange, I see it immediately after C-x o.
>
> 🤦 My apologies, I should have tried with emacs -Q before jumping to
> conclusions; I actually had left C-x o bound to my own command…  I hope
> I didn't waste too much of your time on this.

No problem.  I just wondered why the mode-line is automatically updated
when repeat-in-progress in mode-line-modes is non-nil, but not updated
when it becomes nil.



Reply | Threaded
Open this post in threaded view
|

bug#47566: 28.0.50; diff-hl should use `repeat-mode' ... and not `smartrep'

Dmitry Gutov
In reply to this post by Juri Linkov-2
On 14.04.2021 21:10, Juri Linkov wrote:
> compilation-in-progress is a good example of an unobtrusive indicator
> in the mode line.  Now added to repeat-mode.

Looks nice, thank you.