bug#73172: [PATCH] Move to start of current header in diff-{file, hunk}-prev

bug#73172: [PATCH] Move to start of current header in diff-{file, hunk}-prev

[Spencer Baugh via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Tags: patch


(The following change is split across two patches; the first one, "move
easy-mmode", fixes an unrelated FIXME, which makes the diff in the
second patch simpler)

If point was after a file or hunk header, the diff-file-prev and
diff-hunk-prev commands would move to the start of that header.
But if point was *within* the header, they would not move, and
would report "No previous file" or "No previous hunk".  This
differs from the behavior of most other movement commands,
e.g. backward-sexp or backward-sentence.

This commit fixes diff-file-prev and diff-hunk-prev, as well as
other easy-mmode-define-navigation BASE-prev commands.  Now
these commands move to the start of the containing "thing" just
like other movement commands.

* lisp/emacs-lisp/easy-mmode.el (easy-mmode--prev): Move to
start of current match first.

Also discussed here:
https://lists.gnu.org/archive/html/help-gnu-emacs/2024-08/msg00367.html

In GNU Emacs 29.2.50 (build 17, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2024-09-06 built on
 igm-qws-u22796a
Repository revision: e6d04c06a7eb6ce932b52a346368d02b7a811a00
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.10 (Green Obsidian)

Configured using:
 'configure --with-x-toolkit=lucid --without-gpm --without-gconf
 --without-selinux --without-imagemagick --with-modules --with-gif=no
 --with-cairo --with-rsvg --without-compress-install
 --with-native-compilation=aot --with-tree-sitter
 PKG_CONFIG_PATH=/usr/local/home/garnish/libtree-sitter/0.22.6-1/lib/pkgconfig/'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Move-easy-mmode-define-navigation-logic-to-helper-fu.patch --]
[-- Type: text/x-patch, Size: 6106 bytes --]

From 93f50388bda9f986a5aa8e51378793031cfdce05 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Tue, 10 Sep 2024 13:46:18 -0400
Subject: [PATCH] Move easy-mmode-define-navigation logic to helper functions

The functions defined by easy-mmode-define-navigation are useful
even if the easy-mmode-define-navigation macro is not used.
Let's take a step towards exposing them by moving them out as
helpers.

This also makes the macro much easier to modify and work on.

* lisp/emacs-lisp/easy-mmode.el (easy-mmode--prev)
(easy-mmode--next): Add.
(easy-mmode-define-navigation): Use easy-mmode--prev and
easy-mmode--next.
---
 lisp/emacs-lisp/easy-mmode.el | 86 ++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 37 deletions(-)

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index a140027839e..d3dcab899d6 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -763,6 +763,48 @@ easy-mmode-defsyntax
 ;;; easy-mmode-define-navigation
 ;;;
 
+(defun easy-mmode--prev (re name count &optional endfun narrowfun)
+  "Go to the previous COUNT'th occurence of RE.
+
+If none, error with NAME.
+
+ENDFUN and NARROWFUN are treated like in `easy-mmode-define-navigation'."
+  (unless count (setq count 1))
+  (if (< count 0) (easy-mmode--next re name (- count) endfun narrowfun)
+    (let ((re-narrow (and narrowfun (prog1 (buffer-narrowed-p) (widen)))))
+      (unless (re-search-backward re nil t count)
+        (user-error "No previous %s" name))
+      (when re-narrow (funcall narrowfun)))))
+
+(defun easy-mmode--next (re name count &optional endfun narrowfun)
+  "Go to the next COUNT'th occurence of RE.
+
+If none, error with NAME.
+
+ENDFUN and NARROWFUN are treated like in `easy-mmode-define-navigation'."
+  (unless count (setq count 1))
+  (if (< count 0) (easy-mmode--prev re name (- count) endfun narrowfun)
+    (if (looking-at re) (setq count (1+ count)))
+    (let ((re-narrow (and narrowfun (prog1 (buffer-narrowed-p) (widen)))))
+      (if (not (re-search-forward re nil t count))
+          (if (looking-at re)
+              (goto-char (or (if endfun (funcall endfun)) (point-max)))
+            (user-error "No next %s" name))
+        (goto-char (match-beginning 0))
+        (when (and (eq (current-buffer) (window-buffer))
+                   (called-interactively-p 'interactive))
+          (let ((endpt (or (save-excursion
+                             (if endfun (funcall endfun)
+                               (re-search-forward re nil t 2)))
+                           (point-max))))
+            (unless (pos-visible-in-window-p endpt nil t)
+              (let ((ws (window-start)))
+                (recenter '(0))
+                (if (< (window-start) ws)
+                    ;; recenter scrolled in the wrong direction!
+                    (set-window-start nil ws)))))))
+      (when re-narrow (funcall narrowfun)))))
+
 (defmacro easy-mmode-define-navigation (base re &optional name endfun narrowfun
                                              &rest body)
   "Define BASE-next and BASE-prev to navigate in the buffer.
@@ -780,53 +822,23 @@ easy-mmode-define-navigation
   (let* ((base-name (symbol-name base))
 	 (prev-sym (intern (concat base-name "-prev")))
 	 (next-sym (intern (concat base-name "-next")))
-         (when-narrowed
-          (lambda (body)
-            (if (null narrowfun) body
-              `(let ((was-narrowed (prog1 (buffer-narrowed-p) (widen))))
-                 ,body
-                 (when was-narrowed (funcall #',narrowfun)))))))
+         (endfun (when endfun `#',endfun))
+         (narrowfun (when narrowfun `#',narrowfun)))
     (unless name (setq name base-name))
-    ;; FIXME: Move most of those functions's bodies to helper functions!
     `(progn
        (defun ,next-sym (&optional count)
 	 ,(format "Go to the next COUNT'th %s.
 Interactively, COUNT is the prefix numeric argument, and defaults to 1." name)
 	 (interactive "p")
-	 (unless count (setq count 1))
-	 (if (< count 0) (,prev-sym (- count))
-	   (if (looking-at ,re) (setq count (1+ count)))
-           ,(funcall when-narrowed
-             `(if (not (re-search-forward ,re nil t count))
-                  (if (looking-at ,re)
-                      (goto-char (or ,(if endfun `(funcall #',endfun)) (point-max)))
-                    (user-error "No next %s" ,name))
-                (goto-char (match-beginning 0))
-                (when (and (eq (current-buffer) (window-buffer))
-                           (called-interactively-p 'interactive))
-                  (let ((endpt (or (save-excursion
-                                     ,(if endfun `(funcall #',endfun)
-                                        `(re-search-forward ,re nil t 2)))
-                                   (point-max))))
-                    (unless (pos-visible-in-window-p endpt nil t)
-                      (let ((ws (window-start)))
-                        (recenter '(0))
-                        (if (< (window-start) ws)
-                            ;; recenter scrolled in the wrong direction!
-                            (set-window-start nil ws))))))))
-           ,@body))
+         (easy-mmode--next ,re ,name count ,endfun ,narrowfun)
+         ,@body)
        (put ',next-sym 'definition-name ',base)
        (defun ,prev-sym (&optional count)
 	 ,(format "Go to the previous COUNT'th %s.
-Interactively, COUNT is the prefix numeric argument, and defaults to 1."
-                  (or name base-name))
+Interactively, COUNT is the prefix numeric argument, and defaults to 1." name)
 	 (interactive "p")
-	 (unless count (setq count 1))
-	 (if (< count 0) (,next-sym (- count))
-           ,(funcall when-narrowed
-             `(unless (re-search-backward ,re nil t count)
-                (user-error "No previous %s" ,name)))
-           ,@body))
+         (easy-mmode--prev ,re ,name count ,endfun ,narrowfun)
+         ,@body)
        (put ',prev-sym 'definition-name ',base))))
 
 ;; When deleting these two, also delete them from loaddefs-gen.el.
-- 
2.39.3


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Move-to-start-of-current-header-in-diff-file-hunk-pr.patch --]
[-- Type: text/patch, Size: 2131 bytes --]

From 3bf96598caae3746001fdd5d4f4a4d5a14bdf717 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Tue, 10 Sep 2024 14:18:39 -0400
Subject: [PATCH] Move to start of current header in diff-{file,hunk}-prev

If point was after a file or hunk header, the diff-file-prev and
diff-hunk-prev commands would move to the start of that header.
But if point was *within* the header, they would not move, and
would report "No previous file" or "No previous hunk".  This
differs from the behavior of most other movement commands,
e.g. backward-sexp or backward-sentence.

This commit fixes diff-file-prev and diff-hunk-prev, as well as
other easy-mmode-define-navigation BASE-prev commands.  Now
these commands move to the start of the containing "thing" just
like other movement commands.

* lisp/emacs-lisp/easy-mmode.el (easy-mmode--prev): Move to
start of current match first.
---
 lisp/emacs-lisp/easy-mmode.el | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index d3dcab899d6..7a94d832273 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -772,6 +772,17 @@ easy-mmode--prev
   (unless count (setq count 1))
   (if (< count 0) (easy-mmode--next re name (- count) endfun narrowfun)
     (let ((re-narrow (and narrowfun (prog1 (buffer-narrowed-p) (widen)))))
+      ;; If point is inside a match for RE, move to its beginning like
+      ;; `backward-sexp' and other movement commands.
+      (when (and (not (zerop count))
+                 (save-excursion
+                   ;; Make sure we're out of the current match if any.
+                   (goto-char (if (re-search-backward re nil t 1)
+                                  (match-end 0) (point-min)))
+                   (re-search-forward re nil t 1))
+                 (< (match-beginning 0) (point) (match-end 0)))
+        (goto-char (match-beginning 0))
+        (setq count (1- count)))
       (unless (re-search-backward re nil t count)
         (user-error "No previous %s" name))
       (when re-narrow (funcall narrowfun)))))
-- 
2.39.3

bug#73172: [PATCH] Move to start of current header in diff-{file, hunk}-prev

[Dmitry Gutov]

Hi Spencer and Stefan,

On 10/09/2024 21:40, Spencer Baugh via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:

> (The following change is split across two patches; the first one, "move
> easy-mmode", fixes an unrelated FIXME, which makes the diff in the
> second patch simpler)
> 
> If point was after a file or hunk header, the diff-file-prev and
> diff-hunk-prev commands would move to the start of that header.
> But if point was *within* the header, they would not move, and
> would report "No previous file" or "No previous hunk".  This
> differs from the behavior of most other movement commands,
> e.g. backward-sexp or backward-sentence.
> 
> This commit fixes diff-file-prev and diff-hunk-prev, as well as
> other easy-mmode-define-navigation BASE-prev commands.  Now
> these commands move to the start of the containing "thing" just
> like other movement commands.
> 
> * lisp/emacs-lisp/easy-mmode.el (easy-mmode--prev): Move to
> start of current match first.
> 
> Also discussed here:
> https://lists.gnu.org/archive/html/help-gnu-emacs/2024-08/msg00367.html

Patch#1 seems unequivocally a good thing (easier code iteration) and 
patch#2 seems good on balance.

It does introduce some backward incompatibility in rare cases where I 
have probably internalized the current behavior already -- for example 
in the vc-print-root-log output pressing 'p' while on the first line 
somewhere between the initial '*' and the end of the date dings with "No 
previous log message", and how will move to bol.

But it might be more consistent anyway, given that the there is no ding 
already if you start out inside the summary text.

There aren't many in-tree callers of easy-mmode-define-navigation 
(diff-mode, log-view, smerge and, uh, cvs-status), and few 3rd party 
callers, so this seems low-impact. diff-hl-mode is not affected either 
because it only uses diff-hunk-next, not diff-hunk-prev.