bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Juri Linkov]

> Sorry about stirring in this pile again, but it looks like there is
> unfinished business with respect to `occur-highlight-regexp`
> introduced by this patch:
>
> +(defvar occur-highlight-regexp t
> +  "Regexp matching part of visited source lines to highlight temporarily.
> +Highlight entire line if t; don't highlight source lines if nil.")
>
> Are the t and nil cases really handled? As far as I can tell:
>
> - `occur-mode-goto-occurrence` and `occur-mode-display-occurrence`
>    both crash if `occur-highlight-regexp` isn't a string
> - `occur--highlight-occurrence` does not distinguish t from nil
> - `occur--highlight-occurrence` is only called from the two other (crashing) functions
>
> This was discovered when using an external package that uses
> occur-mode for their own purposes and don't actually have a regexp to
> match (only start and end markers).
>
> Since `occur-highlight-regexp` appears to serve an internal purpose only,
> perhaps we can use some other method to get at the text to highlight?

It seems `compilation-highlight-regexp` was supposed to duplicate the logic
of the existing variable `compilation-highlight-regexp` that is t by default.
But I see such conditions `(stringp highlight-regexp)` in `compilation-goto-locus`,
so maybe 'occur' needs to do the same.

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Mattias Engdegård]

16 juli 2021 kl. 00.10 skrev Juri Linkov <juri@linkov.net>:

> It seems `compilation-highlight-regexp` was supposed to duplicate the logic
> of the existing variable `compilation-highlight-regexp` that is t by default.

Yes, you are probably right. It's not obvious that using the same code is a good fit for Occur.
Specifically, compilation-mode can, in the best case, use the parsed location interval (starting and ending columns). When only a line number is available, it's not possible to do much better than highlighting the entire line. (The grep command uses what appears to be smelly hacks for more precise locations.)

In contrast, for Occur the exact position should always be available since Emacs made the search. Wouldn't it make sense to use it, instead of matching regexps again?

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Mattias Engdegård]

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

As a cheapo fix to prevent the (possibly misguided) external package from crashing in Emacs 28, what about this?


[-- Attachment #2: occur.diff --]
[-- Type: application/octet-stream, Size: 1357 bytes --]

diff --git a/lisp/replace.el b/lisp/replace.el
index ed81097e14..d39ebeca15 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1305,8 +1305,11 @@ occur-mode-goto-occurrence
         (regexp occur-highlight-regexp))
     (pop-to-buffer (marker-buffer pos))
     (goto-char pos)
-    (let ((end-mk (save-excursion (re-search-forward regexp nil t))))
-      (occur--highlight-occurrence pos end-mk))
+    (when regexp
+      (let ((end-mk (if (stringp regexp)
+                        (save-excursion (re-search-forward regexp nil t))
+                      (line-end-position))))
+        (occur--highlight-occurrence pos end-mk)))
     (when buffer (next-error-found buffer (current-buffer)))
     (run-hooks 'occur-mode-find-occurrence-hook)))
 
@@ -1386,8 +1389,11 @@ occur-mode-display-occurrence
     (save-selected-window
       (select-window window)
       (goto-char pos)
-      (let ((end-mk (save-excursion (re-search-forward regexp nil t))))
-        (occur--highlight-occurrence pos end-mk))
+      (when regexp
+        (let ((end-mk (if (stringp regexp)
+                          (save-excursion (re-search-forward regexp nil t))
+                        (line-end-position))))
+          (occur--highlight-occurrence pos end-mk)))
       (next-error-found buffer (current-buffer))
       (run-hooks 'occur-mode-find-occurrence-hook))))
 

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> As a cheapo fix to prevent the (possibly misguided) external package
> from crashing in Emacs 28, what about this?

Hm.  Well, the variable is defined to allow both nil/t in addition to a
string?

(defvar occur-highlight-regexp t
  "Regexp matching part of visited source lines to highlight temporarily.
Highlight entire line if t; don't highlight source lines if nil.")

So I think your patch looks correct.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Mattias Engdegård]

23 juli 2021 kl. 16.05 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> (defvar occur-highlight-regexp t
>  "Regexp matching part of visited source lines to highlight temporarily.
> Highlight entire line if t; don't highlight source lines if nil.")
> 
> So I think your patch looks correct.

Well yes, but that variable itself isn't really useful -- it is really just something transplanted from compilation-mode in order to achieve the same highlighting effect in Occur, but Occur shouldn't need it at all. So my patch is a bit rubbish; we could do better.

Currently, Occur buffers use `occur-target` properties to direct each line to the start of the first match on that line. We could use the property to indicating the exact extents (intervals) of matches, instead. For example, a buffer containing

 VENI VIDI VICI

with the Occur search regexp "VI.I", currently results in a line in *Occur* having the property `occur-target` with a marker to the start of 'VIDI' as value. Instead, we could make the value be ((m1 . m2) (m3 . m4)) where m1..m4 mark the beginning and end of 'VIDI' and 'VICI' respectively. Then occur-highlight-regexp could be done away entirely.

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Currently, Occur buffers use `occur-target` properties to direct each
> line to the start of the first match on that line. We could use the
> property to indicating the exact extents (intervals) of matches,
> instead. For example, a buffer containing
>
>  VENI VIDI VICI
>
> with the Occur search regexp "VI.I", currently results in a line in
> *Occur* having the property `occur-target` with a marker to the start
> of 'VIDI' as value. Instead, we could make the value be ((m1 . m2) (m3
> . m4)) where m1..m4 mark the beginning and end of 'VIDI' and 'VICI'
> respectively. Then occur-highlight-regexp could be done away entirely.

That does indeed sound like a better solution.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Mattias Engdegård]

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

24 juli 2021 kl. 13.46 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> That does indeed sound like a better solution.

All right, this might work. Patch!

The immediate visible benefit is that all matches on the same line are highlighted, not just the first one. It also fixes the compatibility problems mentioned above by removing occur-highlight-regexp entirely.

External packages that populate occur-mode buffers themselves should still work, since the old `occur-target` property format is still recognised. In those cases we just highlight from the first match to the end of the line.


[-- Attachment #2: 0001-Keep-track-of-match-extents-in-occur-mode-bug-39121.patch --]
[-- Type: application/octet-stream, Size: 16050 bytes --]

From 9035a88e1b62980f38362c938eba6b042d500686 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sat, 24 Jul 2021 16:32:11 +0200
Subject: [PATCH] Keep track of match extents in occur-mode (bug#39121)

Use the `occur-target` text property to keep track of the extents of
all matches on each line instead of just the start of the first match.
Doing so allows us to highlight all matches when jumping to a matching
line instead of just the first one, and it works in a more principled
way.  It also removes compatibility problems that were introduced with
occur-highlight-regexp.

For compatibility with code that populate their own occur-mode
buffers, we still accept `occur-target` properties with a single
marker as value.

* lisp/replace.el (occur-highlight-regexp, occur-highlight-overlay):
Remove.
(occur-highlight-overlays): New.
(occur--targets-start): New.
* lisp/replace.el (occur-after-change-function):
(occur-mode-find-occurrence): Replace with...
(occur-mode--find-occurrences): ...this function that returns the
whole `occur-target` property value.
(occur-mode-goto-occurrence, occur-mode-goto-occurrence-other-window)
(occur-goto-locus-delete-o, occur-mode-display-occurrence)
(occur-engine): Adjust to new property format.
(occur--highlight-occurrence): Replace with...
(occur--highlight-occurrences): ...this function that takes
the `occur-target` property value as argument.
(occur-1): Don't use `occur-highlight-regexp`.
* test/lisp/replace-tests.el (occur-highlight-occurrence):
Adapt to new property format.
---
 lisp/replace.el            | 177 +++++++++++++++++++------------------
 test/lisp/replace-tests.el |   2 +-
 2 files changed, 91 insertions(+), 88 deletions(-)

diff --git a/lisp/replace.el b/lisp/replace.el
index 7e30f1fc55..24befed241 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -792,12 +792,8 @@ regexp-history
 Maximum length of the history list is determined by the value
 of `history-length', which see.")
 
-(defvar occur-highlight-regexp t
-  "Regexp matching part of visited source lines to highlight temporarily.
-Highlight entire line if t; don't highlight source lines if nil.")
-
-(defvar occur-highlight-overlay nil
-  "Overlay used to temporarily highlight occur matches.")
+(defvar occur-highlight-overlays nil
+  "Overlays used to temporarily highlight occur matches.")
 
 (defvar occur-collect-regexp-history '("\\1")
   "History of regexp for occur's collect operation")
@@ -1357,18 +1353,27 @@ occur-cease-edit
     (occur-mode)
     (message "Switching to Occur mode.")))
 
+(defun occur--targets-start (targets)
+  "First marker of the `occur-target' property value TARGETS."
+  (if (consp targets)
+      (caar targets)
+    ;; Tolerate an `occur-target' value that is a single marker for
+    ;; compatibility.
+    targets))
+
 (defun occur-after-change-function (beg end length)
   (save-excursion
     (goto-char beg)
     (let* ((line-beg (line-beginning-position))
-	   (m (get-text-property line-beg 'occur-target))
+	   (targets (get-text-property line-beg 'occur-target))
+           (m (occur--targets-start targets))
 	   (buf (marker-buffer m))
 	   col)
       (when (and (get-text-property line-beg 'occur-prefix)
 		 (not (get-text-property end 'occur-prefix)))
 	(when (= length 0)
 	  ;; Apply occur-target property to inserted (e.g. yanked) text.
-	  (put-text-property beg end 'occur-target m)
+	  (put-text-property beg end 'occur-target targets)
 	  ;; Did we insert a newline?  Occur Edit mode can't create new
 	  ;; Occur entries; just discard everything after the newline.
 	  (save-excursion
@@ -1402,35 +1407,38 @@ occur-revert-function
   "Handle `revert-buffer' for Occur mode buffers."
   (apply #'occur-1 (append occur-revert-arguments (list (buffer-name)))))
 
-(defun occur-mode-find-occurrence ()
-  (let ((pos (get-text-property (point) 'occur-target)))
-    (unless pos
+(defun occur-mode--find-occurrences ()
+  ;; The `occur-target' property value is a list of (BEG . END) for each
+  ;; match on the line, or (for compatibility) a single marker to the start
+  ;; of the first match.
+  (let* ((targets (get-text-property (point) 'occur-target))
+         (start (occur--targets-start targets)))
+    (unless targets
       (error "No occurrence on this line"))
-    (unless (buffer-live-p (marker-buffer pos))
+    (unless (buffer-live-p (marker-buffer start))
       (error "Buffer for this occurrence was killed"))
-    pos))
+    targets))
 
 (defalias 'occur-mode-mouse-goto 'occur-mode-goto-occurrence)
 (defun occur-mode-goto-occurrence (&optional event)
   "Go to the occurrence specified by EVENT, a mouse click.
 If not invoked by a mouse click, go to occurrence on the current line."
   (interactive (list last-nonmenu-event))
-  (let ((buffer (when event (current-buffer)))
-        (pos
-         (if (null event)
-             ;; Actually `event-end' works correctly with a nil argument as
-             ;; well, so we could dispense with this test, but let's not
-             ;; rely on this undocumented behavior.
-             (occur-mode-find-occurrence)
-           (with-current-buffer (window-buffer (posn-window (event-end event)))
-             (save-excursion
-               (goto-char (posn-point (event-end event)))
-               (occur-mode-find-occurrence)))))
-        (regexp occur-highlight-regexp))
+  (let* ((buffer (when event (current-buffer)))
+         (targets
+          (if (null event)
+              ;; Actually `event-end' works correctly with a nil argument as
+              ;; well, so we could dispense with this test, but let's not
+              ;; rely on this undocumented behavior.
+              (occur-mode--find-occurrences)
+            (with-current-buffer (window-buffer (posn-window (event-end event)))
+              (save-excursion
+                (goto-char (posn-point (event-end event)))
+                (occur-mode--find-occurrences)))))
+         (pos (occur--targets-start targets)))
     (pop-to-buffer (marker-buffer pos))
     (goto-char pos)
-    (let ((end-mk (save-excursion (re-search-forward regexp nil t))))
-      (occur--highlight-occurrence pos end-mk))
+    (occur--highlight-occurrences targets)
     (when buffer (next-error-found buffer (current-buffer)))
     (run-hooks 'occur-mode-find-occurrence-hook)))
 
@@ -1438,15 +1446,15 @@ occur-mode-goto-occurrence-other-window
   "Go to the occurrence the current line describes, in another window."
   (interactive)
   (let ((buffer (current-buffer))
-        (pos (occur-mode-find-occurrence)))
+        (pos (occur--targets-start (occur-mode--find-occurrences))))
     (switch-to-buffer-other-window (marker-buffer pos))
     (goto-char pos)
     (next-error-found buffer (current-buffer))
     (run-hooks 'occur-mode-find-occurrence-hook)))
 
-;; Stolen from compile.el
 (defun occur-goto-locus-delete-o ()
-  (delete-overlay occur-highlight-overlay)
+  (mapc #'delete-overlay occur-highlight-overlays)
+  (setq occur-highlight-overlays nil)
   ;; Get rid of timer and hook that would try to do this again.
   (if (timerp next-error-highlight-timer)
       (cancel-timer next-error-highlight-timer))
@@ -1454,64 +1462,55 @@ occur-goto-locus-delete-o
                #'occur-goto-locus-delete-o))
 
 ;; Highlight the current visited occurrence.
-;; Adapted from `compilation-goto-locus'.
-(defun occur--highlight-occurrence (mk end-mk)
-  (let ((highlight-regexp occur-highlight-regexp))
-    (if (timerp next-error-highlight-timer)
-        (cancel-timer next-error-highlight-timer))
-    (unless occur-highlight-overlay
-      (setq occur-highlight-overlay
-	    (make-overlay (point-min) (point-min)))
-      (overlay-put occur-highlight-overlay 'face 'next-error))
-    (with-current-buffer (marker-buffer mk)
-      (save-excursion
-        (if end-mk (goto-char end-mk) (end-of-line))
-        (let ((end (point)))
-	  (if mk (goto-char mk) (beginning-of-line))
-	  (if (and (stringp highlight-regexp)
-		   (re-search-forward highlight-regexp end t))
-	      (progn
-	        (goto-char (match-beginning 0))
-	        (move-overlay occur-highlight-overlay
-			      (match-beginning 0) (match-end 0)
-			      (current-buffer)))
-	    (move-overlay occur-highlight-overlay
-			  (point) end (current-buffer)))
-	  (if (or (eq next-error-highlight t)
-		  (numberp next-error-highlight))
-	      ;; We want highlighting: delete overlay on next input.
-	      (add-hook 'pre-command-hook
-		        #'occur-goto-locus-delete-o)
-	    ;; We don't want highlighting: delete overlay now.
-	    (delete-overlay occur-highlight-overlay))
-	  ;; We want highlighting for a limited time:
-	  ;; set up a timer to delete it.
-	  (when (numberp next-error-highlight)
-	    (setq next-error-highlight-timer
-		  (run-at-time next-error-highlight nil
-			       'occur-goto-locus-delete-o))))))
-    (when (eq next-error-highlight 'fringe-arrow)
-      ;; We want a fringe arrow (instead of highlighting).
-      (setq next-error-overlay-arrow-position
-	    (copy-marker (line-beginning-position))))))
+(defun occur--highlight-occurrences (targets)
+  (let ((start-marker (occur--targets-start targets)))
+    (occur-goto-locus-delete-o)
+    (with-current-buffer (marker-buffer start-marker)
+      (when (or (eq next-error-highlight t)
+	        (numberp next-error-highlight))
+        (setq occur-highlight-overlays
+              (mapcar (lambda (target)
+                        (let ((o (make-overlay (car target) (cdr target))))
+                          (overlay-put o 'face 'next-error)
+                          o))
+                      (if (listp targets)
+                          targets
+                        ;; `occur-target' compatibility: when we only
+                        ;; have a single starting point, highlight the
+                        ;; rest of the line.
+                        (let ((end-pos (save-excursion
+                                         (goto-char start-marker)
+                                         (line-end-position))))
+                          (list (cons start-marker end-pos))))))
+        (add-hook 'pre-command-hook #'occur-goto-locus-delete-o)
+        (when (numberp next-error-highlight)
+          ;; We want highlighting for a limited time:
+          ;; set up a timer to delete it.
+	  (setq next-error-highlight-timer
+	        (run-at-time next-error-highlight nil
+			     'occur-goto-locus-delete-o))))
+
+      (when (eq next-error-highlight 'fringe-arrow)
+        ;; We want a fringe arrow (instead of highlighting).
+        (setq next-error-overlay-arrow-position
+	      (copy-marker (line-beginning-position)))))))
 
 (defun occur-mode-display-occurrence ()
   "Display in another window the occurrence the current line describes."
   (interactive)
-  (let ((buffer (current-buffer))
-        (pos (occur-mode-find-occurrence))
-        (regexp occur-highlight-regexp)
-        (next-error-highlight next-error-highlight-no-select)
-        (display-buffer-overriding-action
-         '(nil (inhibit-same-window . t)))
-	window)
+  (let* ((buffer (current-buffer))
+         (targets (occur-mode--find-occurrences))
+         (pos (occur--targets-start targets))
+         (next-error-highlight next-error-highlight-no-select)
+         (display-buffer-overriding-action
+          '(nil (inhibit-same-window . t)))
+	 window)
     (setq window (display-buffer (marker-buffer pos) t))
     ;; This is the way to set point in the proper window.
     (save-selected-window
       (select-window window)
       (goto-char pos)
-      (let ((end-mk (save-excursion (re-search-forward regexp nil t))))
-        (occur--highlight-occurrence pos end-mk))
+      (occur--highlight-occurrences targets)
       (next-error-found buffer (current-buffer))
       (run-hooks 'occur-mode-find-occurrence-hook))))
 
@@ -1868,7 +1867,6 @@ occur-1
 	    (buffer-undo-list t)
 	    (occur--final-pos nil))
 	(erase-buffer)
-        (setq-local occur-highlight-regexp regexp)
 	(let ((count
 	       (if (stringp nlines)
                    ;; Treat nlines as a regexp to collect.
@@ -1968,7 +1966,7 @@ occur-engine
 		       (origpt nil)
 		       (begpt nil)
 		       (endpt nil)
-		       (marker nil)
+                       markers            ; list of (BEG-MARKER . END-MARKER)
 		       (curstring "")
 		       (ret nil)
 	               ;; The following binding is for when case-fold-search
@@ -1994,8 +1992,7 @@ occur-engine
 		        (setq endpt (line-end-position)))
 		      ;; Sum line numbers up to the first match line.
 		      (setq curr-line (+ curr-line (count-lines origpt begpt)))
-		      (setq marker (make-marker))
-		      (set-marker marker matchbeg)
+                      (setq markers nil)
 		      (setq curstring (occur-engine-line begpt endpt keep-props))
 		      ;; Highlight the matches
 		      (let ((len (length curstring))
@@ -2017,6 +2014,11 @@ occur-engine
 			    (setq orig-line-shown-p t)))
 		        (while (and (< start len)
 				    (string-match regexp curstring start))
+                          (push (cons (set-marker (make-marker)
+                                                  (+ begpt (match-beginning 0)))
+                                      (set-marker (make-marker)
+                                                  (+ begpt (match-end 0))))
+                                markers)
 			  (setq matches (1+ matches))
 			  (add-text-properties
 			   (match-beginning 0) (match-end 0)
@@ -2029,6 +2031,7 @@ occur-engine
 			  ;; Avoid infloop (Bug#7593).
 			  (let ((end (match-end 0)))
 			    (setq start (if (= start end) (1+ start) end)))))
+                      (setq markers (nreverse markers))
 		      ;; Generate the string to insert for this match
 		      (let* ((match-prefix
 			      ;; Using 7 digits aligns tabs properly.
@@ -2042,7 +2045,7 @@ occur-engine
 				                     ;; (for Occur Edit mode).
 				                     front-sticky t
 						     rear-nonsticky t
-						     occur-target ,marker
+						     occur-target ,markers
 						     follow-link t
 				                     help-echo "mouse-2: go to this occurrence"))))
 			     (match-str
@@ -2050,7 +2053,7 @@ occur-engine
 			      ;; because that loses.  And don't put it
 			      ;; on context lines to reduce flicker.
 			      (propertize curstring
-					  'occur-target marker
+					  'occur-target markers
 					  'follow-link t
 					  'help-echo
 					  "mouse-2: go to this occurrence"))
@@ -2069,8 +2072,8 @@ occur-engine
                                 ;; get a contiguous highlight.
 			        (propertize (concat match-prefix match-str)
                                             'mouse-face 'highlight))
-			       ;; Add marker at eol, but no mouse props.
-			       (propertize "\n" 'occur-target marker)))
+			       ;; Add markers at eol, but no mouse props.
+			       (propertize "\n" 'occur-target markers)))
 			     (data
 			      (if (= nlines 0)
 				  ;; The simple display style
diff --git a/test/lisp/replace-tests.el b/test/lisp/replace-tests.el
index 417946c35f..7f62a417a0 100644
--- a/test/lisp/replace-tests.el
+++ b/test/lisp/replace-tests.el
@@ -589,7 +589,7 @@ occur-highlight-occurrence
       (replace-tests-with-highlighted-occurrence highlight-locus
         (occur-mode-display-occurrence)
         (with-current-buffer (marker-buffer
-                              (get-text-property (point) 'occur-target))
+                              (caar (get-text-property (point) 'occur-target)))
           (should (funcall check-overlays has-overlay)))))))
 
 (ert-deftest replace-regexp-bug45973 ()
-- 
2.21.1 (Apple Git-122.3)

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> All right, this might work. Patch!

Skimming the patch, it makes sense to me.  (But I didn't try it.)

> The immediate visible benefit is that all matches on the same line are
> highlighted, not just the first one. It also fixes the compatibility
> problems mentioned above by removing occur-highlight-regexp entirely.
>
> External packages that populate occur-mode buffers themselves should
> still work, since the old `occur-target` property format is still
> recognised. In those cases we just highlight from the first match to
> the end of the line.

Great; go ahead and push.

(And I see that `occur-highlight-regexp' was introduced after emacs-27,
so it's indeed OK to just remove it like your patch does.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 25 Jul 2021 08:41:14 +0200
> Cc: Juri Linkov <juri@linkov.net>, 39121@debbugs.gnu.org,
>  Tino Calancha <tino.calancha@gmail.com>
> 
> Mattias Engdegård <mattiase@acm.org> writes:
> 
> > All right, this might work. Patch!
> 
> Skimming the patch, it makes sense to me.  (But I didn't try it.)
> 
> > The immediate visible benefit is that all matches on the same line are
> > highlighted, not just the first one. It also fixes the compatibility
> > problems mentioned above by removing occur-highlight-regexp entirely.
> >
> > External packages that populate occur-mode buffers themselves should
> > still work, since the old `occur-target` property format is still
> > recognised. In those cases we just highlight from the first match to
> > the end of the line.
> 
> Great; go ahead and push.
> 
> (And I see that `occur-highlight-regexp' was introduced after emacs-27,
> so it's indeed OK to just remove it like your patch does.)

Should we say something about these changes in NEWS?

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Mattias Engdegård]

25 juli 2021 kl. 11.16 skrev Eli Zaretskii <eliz@gnu.org>:

> Should we say something about these changes in NEWS?

That's a good question. No incompatible changes were made, nor were any new facilities introduced that a user needs to know about. Did you have anything particular in mind?

Packages that attempt to piggy-back onto occur-mode do depend on undocumented implementation aspects, but after this change, they now work again as before. If we think that it is a good idea to use occur-mode in that way, we should provide a programming interface.

The previous change (abe7c22da966) made `next-error-highlight` and `next-error-highlight-no-select` effective in occur-mode. Perhaps that should make it to NEWS.

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Mattias Engdegård <mattiase@acm.org> writes:

> 25 juli 2021 kl. 11.16 skrev Eli Zaretskii <eliz@gnu.org>:
>
>> Should we say something about these changes in NEWS?
>
> That's a good question. No incompatible changes were made, nor were any new
> facilities introduced that a user needs to know about. Did you have anything
> particular in mind?

Just one minor incompatibility: the function occur-mode-find-occurrence
is still used in lisp/eshell/em-unix.el.

Thanks,

-- 
Basil

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Mattias Engdegård]

25 juli 2021 kl. 13.39 skrev Basil L. Contovounesios <contovob@tcd.ie>:

> Just one minor incompatibility: the function occur-mode-find-occurrence
> is still used in lisp/eshell/em-unix.el.

Right, and searching some more reveals that it's used by a few external packages as well (like fstar-mode). I've put it back and things seem to work again. Thank you for finding this!

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sun, 25 Jul 2021 12:55:47 +0200
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, juri@linkov.net, 39121@debbugs.gnu.org,
>         tino.calancha@gmail.com
> 
> 25 juli 2021 kl. 11.16 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > Should we say something about these changes in NEWS?
> 
> That's a good question. No incompatible changes were made, nor were any new facilities introduced that a user needs to know about. Did you have anything particular in mind?

That the occur-target text property's value can now be something new,
and that this new value form is actually the preferred one?

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Mattias Engdegård]

25 juli 2021 kl. 13.49 skrev Eli Zaretskii <eliz@gnu.org>:

> That the occur-target text property's value can now be something new,
> and that this new value form is actually the preferred one?

Maybe, but we never said anything about occur-target to begin with. It's like telling school-children that from now on they should use a different place for smoking weed.

I'm not necessarily against it either. There may be a slight advantage for some code that can make good use of the new format. However, we can keep supporting existing code more or less indefinitely.

It is also really not a good programming interface. I just fixed several bit-rot bugs in tex-mode.el (none related to my changes) and it turns out that the exact details of populating an occur-mode buffer are fiddly and easy to get wrong.

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sun, 25 Jul 2021 17:09:37 +0200
> Cc: larsi@gnus.org, juri@linkov.net, 39121@debbugs.gnu.org,
>         tino.calancha@gmail.com
> 
> 25 juli 2021 kl. 13.49 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > That the occur-target text property's value can now be something new,
> > and that this new value form is actually the preferred one?
> 
> Maybe, but we never said anything about occur-target to begin with.

The NEWS file doesn't necessarily describe only stuff documented
somewhere, it also describes changes that aren't documented anywhere
but the source code.  Suppose someone read the source of replace.el,
found out about this property, and uses it to do something, either
privately or for some 3rd-part package.  Put yourself in the shows of
that person and ask yourself whether you'd like to know that this kind
of change has been installed in Emacs.

> I'm not necessarily against it either. There may be a slight advantage for some code that can make good use of the new format. However, we can keep supporting existing code more or less indefinitely.

Since you introduced the new format, you probably thought it to be
better than the existing one, right?  Then telling others about that
would be a good service, IMO.

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Mattias Engdegård]

25 juli 2021 kl. 18.27 skrev Eli Zaretskii <eliz@gnu.org>:

> The NEWS file doesn't necessarily describe only stuff documented
> somewhere, it also describes changes that aren't documented anywhere
> but the source code.

Yes, but then it's always something that affects the user in some way, isn't it? Mentioning changed internals doesn't seem to be standard practice, but I could be wrong about that. Would you point out a few examples of where we described changed aspects of undocumented implementation details in NEWS? That would support your view and help me understand it better.

The question is also whether it should be documented at all. The fact that it never was before, as well as the general ad-hoc nature of the interface, are strong indicators that it probably shouldn't be.

As a case in point: until Lars and I fixed it, the use of occur-mode in tex-mode.el had been broken since at least Emacs 24, in equal parts for reasons of bit-rot (implementation details changed) and incorrect assumptions of interface invariants. And this is an Emacs core package.

>  Suppose someone read the source of replace.el,
> found out about this property, and uses it to do something, either
> privately or for some 3rd-part package.  Put yourself in the shows of
> that person and ask yourself whether you'd like to know that this kind
> of change has been installed in Emacs.

The `occur-target` property alone is far from sufficient for populating occur-mode buffers; it is one implementation detail of many. A little knowledge and all that.

It would have been different if we had changed the implementation incompatibly; in such case, I agree it would have been polite to issue a notice about it. But nothing should break as a result of the change we are talking about.

> Since you introduced the new format, you probably thought it to be
> better than the existing one, right?  Then telling others about that
> would be a good service, IMO.

The change was made exclusively for improving Occur itself, and the external packages that I have seen would generally draw little advantage from doing anything differently. Of course, I haven't seen them all, but having other people depending on implementation details of your software is a maintenance burden which either impedes progress.

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sun, 25 Jul 2021 20:54:47 +0200
> Cc: larsi@gnus.org, juri@linkov.net, 39121@debbugs.gnu.org,
>         tino.calancha@gmail.com
> 
> 25 juli 2021 kl. 18.27 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > The NEWS file doesn't necessarily describe only stuff documented
> > somewhere, it also describes changes that aren't documented anywhere
> > but the source code.
> 
> Yes, but then it's always something that affects the user in some way, isn't it?

"User" in this case includes Lisp programmers; there's the "Lisp
changes" section in NEWS for that reason.

> Mentioning changed internals doesn't seem to be standard practice, but I could be wrong about that.

Text properties are not internals, they are visible to any Lisp
program and to the user.

> Would you point out a few examples of where we described changed aspects of undocumented implementation details in NEWS? That would support your view and help me understand it better.

Sorry, no, I won't.  I think this aspect of the change should be in
NEWS, and I'm asking you to document it there.  I don't understand why
I'm required to go to such lengths to justify a simple request.  If
you are still not convinced, I will do it myself, because this endless
dispute about a couple of sentences in NEWS is more than I can afford.

> The question is also whether it should be documented at all.

I think it should, and have said so.

> > Since you introduced the new format, you probably thought it to be
> > better than the existing one, right?  Then telling others about that
> > would be a good service, IMO.
> 
> The change was made exclusively for improving Occur itself, and the external packages that I have seen would generally draw little advantage from doing anything differently. Of course, I haven't seen them all, but having other people depending on implementation details of your software is a maintenance burden which either impedes progress.

Please leave the final judgment about that to me.  I understand your
point and your doubts, but I still think we should document this
aspect of the change in NEWS.  I hope this is enough to convince you.

TIA

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Mattias Engdegård]

25 juli 2021 kl. 21.23 skrev Eli Zaretskii <eliz@gnu.org>:

> Text properties are not internals, they are visible to any Lisp
> program and to the user.

No, that can be said for just about anything in Lisp. That doesn't mean there are no internal implementation details.

> I think this aspect of the change should be in
> NEWS, and I'm asking you to document it there.

Certainly, I'll do it right away. Given the apparent lack of precedence it wasn't clear how best to describe it, but I suppose there is a first time for anything.

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Eli Zaretskii]

> Feedback-ID:mattiase@acm.or
> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sun, 25 Jul 2021 21:30:14 +0200
> Cc: larsi@gnus.org, juri@linkov.net, 39121@debbugs.gnu.org,
>         tino.calancha@gmail.com
> 
> 25 juli 2021 kl. 21.23 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > I think this aspect of the change should be in
> > NEWS, and I'm asking you to document it there.
> 
> Certainly, I'll do it right away.

Thank you.

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Mattias Engdegård]

25 juli 2021 kl. 08.41 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> Great; go ahead and push.

Thank you, done.

I also noticed that occur-edit-mode manhandled markers on the edited line so that even changing a single character outside a match would destroy all occur-target highlighting on that line. I pushed a fix for that, too.

That bug has apparently been with us from the beginning of occur-edit-mode but not noticed until now.

> (And I see that `occur-highlight-regexp' was introduced after emacs-27,
> so it's indeed OK to just remove it like your patch does.)

Yes, and I have tested the code against an external package that creates its own occur-mode buffers and it works as expected.

bug#39121: 27.0.60; occur: Add bindings for next-error-no-select

[Tino Calancha]

Severity: wishlist
X-Debbugs-Cc: Juri Linkov <juri@linkov.net>

I wish having `next-error-no-select', `previous-error-no-select' bound to `n'
and `p' in the occur mode, as we have in *grep* buffer.

AFAICS, we have all the infrastructure and it's just a matter of define
the bindings at `occur-mode-map'.

--8<-----------------------------cut here---------------start------------->8---
commit 72617bd49b151772d3c709bfa489d0a8f14bf408
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Mon Jan 13 21:37:39 2020 +0100

    occur: Add bindings for next-error-no-select
    
    Add bindings to navigate the matches without select their buffers.
    * lisp/replace.el (occur-mode-map): Bind n to next-error-no-select
    and p to previous-error-no-select.
    
    * etc/NEWS (Changes in Specialized Modes and Packages in Emacs 27.1):
    Annonce this change.

diff --git a/etc/NEWS b/etc/NEWS
index 031ddf5800..572dfbbcf0 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -688,6 +688,8 @@ same as the 'C-x C-+' and 'C-x C--' commands.
 \f
 * Changes in Specialized Modes and Packages in Emacs 27.1
 
+** New bindings in occur-mode, 'next-error-no-select' bound to 'n' and
+'previous-error-no-select' bound to 'p'.
 ---
 ** New HTML mode skeleton 'html-id-anchor'.
 This new command (which inserts an <a id="foo">_</a> skeleton) is
diff --git a/lisp/replace.el b/lisp/replace.el
index a0b050637e..3c1918a257 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -1106,6 +1106,8 @@ occur-mode-map
     (define-key map "\C-m" 'occur-mode-goto-occurrence)
     (define-key map "o" 'occur-mode-goto-occurrence-other-window)
     (define-key map "\C-o" 'occur-mode-display-occurrence)
+    (define-key map "n" 'next-error-no-select)
+    (define-key map "p" 'previous-error-no-select)
     (define-key map "\M-n" 'occur-next)
     (define-key map "\M-p" 'occur-prev)
     (define-key map "r" 'occur-rename-buffer)

--8<-----------------------------cut here---------------end--------------->8---

In GNU Emacs 27.0.60 (build 48, x86_64-pc-linux-gnu, GTK+ Version 3.24.5)
 of 2020-01-13 built on calancha-pc.dy.bbexcite.jp
Repository revision: d645628e3cf6ebe5eaea3b40100bd77b9c823f8b
Repository branch: emacs-27
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)