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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 39121 in the body.
You can then email your comments to 39121 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.60; occur: Add bindings for next-error-no-select
Date: Mon, 13 Jan 2020 21:51:03 +0100

Severity: wishlist
X-Debbugs-Cc: Juri Linkov <juri <at> 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 <at> 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.
 
 * 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)

Message #10 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: Juri Linkov <juri <at> linkov.net>
Cc: 39121 <at> debbugs.gnu.org, 39122 <at> debbugs.gnu.org
Subject: Re: bug#39122: 27.0.60; occur: Add bindings for next-error-no-select
Date: Thu, 21 May 2020 23:05:15 +0200

Juri Linkov <juri <at> linkov.net> writes:

> merge 39121 39122
> thanks
>
>> 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.
> It's a good idea to make occur more consistent with grep/compile, thanks.

Hi Juri,
I have refined the patch so that we have visual feedback during the
navigation (i.e. highligh) as `grep' does.

--8<-----------------------------cut here---------------start------------->8---
commit 7d5917d0a2eda1782b9461951e40bfb837bc75ab
Author: Tino Calancha <tino.calancha <at> gmail.com>
Date:   Thu May 21 22:36:00 2020 +0200

    occur: Add bindings for next-error-no-select
    
    Make the navigation in the occur buffer closer
    to the navigation in the compilation buffer.
    
    Add bindings to navigate the occur matches (Bug#39121).
    Honor `next-error-highlight' and `next-error-highlight-no-select'
    when navigating the occurrences.
    
    * lisp/replace.el (occur-highlight-regexp, occur-highlight-overlay):
    New variables.
    (occur-1): Set `occur-highlight-regexp' to the searched regexp.
    (occur-goto-locus-delete-o, occur--highlight-occurrence): New defuns.
    (occur-mode-display-occurrence, occur-mode-goto-occurrence):
    Use `occur--highlight-occurrence'.
    (occur-mode-map): Bind n to `next-error-no-select'
    and p to `previous-error-no-select'
    
    * etc/NEWS (Changes in Sppecialized Modes and Packages in Emacs 28.1):
    Announce this change.
    
    * test/lisp/replace-tests.el (replace-tests-with-highlighted-occurrence):
    Add helper macro.
    (occur-highlight-occurrence): Add test.

diff --git a/etc/NEWS b/etc/NEWS
index 1bf1403cab..a273a06ef7 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -101,6 +101,9 @@ horizontal movements now stop at the edge of the board.
 
 * Changes in Specialized Modes and Packages in Emacs 28.1
 
+** New bindings in occur-mode, 'next-error-no-select' bound to 'n' and
+'previous-error-no-select' bound to 'p'.
+
 ** EIEIO: 'oset' and 'oset-default' are declared obsolete.
 
 ** New minor mode 'cl-font-lock-built-in-mode' for `lisp-mode'.
diff --git a/lisp/replace.el b/lisp/replace.el
index f3a71f87fe..69092c16f9 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -757,6 +757,13 @@ 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-collect-regexp-history '("\\1")
   "History of regexp for occur's collect operation")
 
@@ -1113,6 +1120,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)
@@ -1261,9 +1270,12 @@ occur-mode-goto-occurrence
            (with-current-buffer (window-buffer (posn-window (event-end event)))
              (save-excursion
                (goto-char (posn-point (event-end event)))
-               (occur-mode-find-occurrence))))))
+               (occur-mode-find-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 buffer (next-error-found buffer (current-buffer)))
     (run-hooks 'occur-mode-find-occurrence-hook)))
 
@@ -1277,17 +1289,74 @@ occur-mode-goto-occurrence-other-window
     (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)
+  ;; 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))
+  (remove-hook 'pre-command-hook
+               #'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-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)
     (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))
       (next-error-found buffer (current-buffer))
       (run-hooks 'occur-mode-find-occurrence-hook))))
 
@@ -1612,6 +1681,7 @@ occur-1
 	    (buffer-undo-list t)
 	    (occur--final-pos nil))
 	(erase-buffer)
+        (set (make-local-variable 'occur-highlight-regexp) regexp)
 	(let ((count
 	       (if (stringp nlines)
                    ;; Treat nlines as a regexp to collect.
diff --git a/test/lisp/replace-tests.el b/test/lisp/replace-tests.el
index f5cff92d54..aed14c3357 100644
--- a/test/lisp/replace-tests.el
+++ b/test/lisp/replace-tests.el
@@ -546,4 +546,46 @@ replace-tests--query-replace-undo
       ?q
       (string= expected (buffer-string))))))
 
+(defmacro replace-tests-with-highlighted-occurrence (highlight-locus &rest body)
+  "Helper macro to test the highlight of matches when navigating occur buffer.
+
+Eval BODY with `next-error-highlight' and `next-error-highlight-no-select'
+bound to HIGHLIGHT-LOCUS."
+  (declare (indent 1) (debug (form body)))
+  `(let ((regexp "foo")
+         (next-error-highlight ,highlight-locus)
+         (next-error-highlight-no-select ,highlight-locus)
+         (buffer (generate-new-buffer "test"))
+         (inhibit-message t))
+     (unwind-protect
+         ;; Local bind to disable the deletion of `occur-highlight-overlay'
+         (cl-letf (((symbol-function 'occur-goto-locus-delete-o) (lambda ())))
+           (with-current-buffer buffer (dotimes (_ 3) (insert regexp ?\n)))
+           (pop-to-buffer buffer)
+           (occur regexp)
+           (pop-to-buffer "*Occur*")
+           (occur-next)
+           ,@body)
+       (kill-buffer buffer)
+       (kill-buffer "*Occur*"))))
+
+(ert-deftest occur-highlight-occurrence ()
+  "Test for https://debbugs.gnu.org/39121 ."
+  (let ((alist '((nil . nil) (0.5 . t) (t . t) (fringe-arrow . nil)))
+        (check-overlays
+         (lambda (has-ov)
+           (eq has-ov (not (null (overlays-in (point-min) (point-max))))))))
+    (pcase-dolist (`(,highlight-locus . ,has-overlay) alist)
+      ;; Visiting occurrences
+      (replace-tests-with-highlighted-occurrence highlight-locus
+        (occur-mode-goto-occurrence)
+        (should (funcall check-overlays has-overlay)))
+      ;; Displaying occurrences
+      (replace-tests-with-highlighted-occurrence highlight-locus
+        (occur-mode-display-occurrence)
+        (with-current-buffer (marker-buffer
+                              (get-text-property (point) 'occur-target))
+          (should (funcall check-overlays has-overlay)))))))
+
+
 ;;; replace-tests.el ends here

--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 28.0.50 (build 12, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0)
 of 2020-05-21 built on calancha-pc.dy.bbexcite.jp
Repository revision: d714aa753b744c903d149a1f6c69262d958c313e
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)

Message #13 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Tino Calancha <tino.calancha <at> gmail.com>
Cc: 39121 <at> debbugs.gnu.org, 39122 <at> debbugs.gnu.org
Subject: Re: bug#39122: 27.0.60; occur: Add bindings for next-error-no-select
Date: Fri, 22 May 2020 01:33:53 +0300

> I have refined the patch so that we have visual feedback during the
> navigation (i.e. highligh) as `grep' does.

Using highlighting like in grep is a nice addition that makes
occur consistent with grep, thanks for this long-awaited feature.

Message #18 received at 39121-done <at> debbugs.gnu.org (full text, mbox):

From: Tino Calancha <tino.calancha <at> gmail.com>
To: 39121-done <at> debbugs.gnu.org
Subject: Re: bug#39121: bug#39122: 27.0.60; occur: Add bindings for
 next-error-no-select
Date: Sun, 31 May 2020 12:43:04 +0200

Juri Linkov <juri <at> linkov.net> writes:

>> I have refined the patch so that we have visual feedback during the
>> navigation (i.e. highligh) as `grep' does.
>
> Using highlighting like in grep is a nice addition that makes
> occur consistent with grep, thanks for this long-awaited feature.

Pushed into master branch as commit 'occur: Add bindings for next-error-no-select'
(abe7c22da96694ced1bc80ec7eb9eb8a662a568b)

Message #30 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 39121 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Fri, 16 Jul 2021 01:10:44 +0300

> 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.

Message #33 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 39121 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Fri, 16 Jul 2021 15:20:49 +0200

16 juli 2021 kl. 00.10 skrev Juri Linkov <juri <at> 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?

Message #36 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 39121 <at> debbugs.gnu.org, Tino Calancha <tino.calancha <at> gmail.com>
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Fri, 23 Jul 2021 15:32:43 +0200

[Message part 1 (text/plain, inline)]

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

[occur.diff (application/octet-stream, attachment)]

Message #39 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: Tino Calancha <tino.calancha <at> gmail.com>, 39121 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Fri, 23 Jul 2021 16:05:30 +0200

Mattias Engdegård <mattiase <at> 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

Message #42 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Tino Calancha <tino.calancha <at> gmail.com>, 39121 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Fri, 23 Jul 2021 19:16:02 +0200

23 juli 2021 kl. 16.05 skrev Lars Ingebrigtsen <larsi <at> 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.

Message #45 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: Tino Calancha <tino.calancha <at> gmail.com>, 39121 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sat, 24 Jul 2021 13:46:40 +0200

Mattias Engdegård <mattiase <at> 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

Message #48 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Tino Calancha <tino.calancha <at> gmail.com>, 39121 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sat, 24 Jul 2021 19:29:57 +0200

[Message part 1 (text/plain, inline)]

24 juli 2021 kl. 13.46 skrev Lars Ingebrigtsen <larsi <at> 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.

[0001-Keep-track-of-match-extents-in-occur-mode-bug-39121.patch (application/octet-stream, attachment)]

Message #51 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: Tino Calancha <tino.calancha <at> gmail.com>, 39121 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sun, 25 Jul 2021 08:41:14 +0200

Mattias Engdegård <mattiase <at> 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

Message #54 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: mattiase <at> acm.org, tino.calancha <at> gmail.com, 39121 <at> debbugs.gnu.org,
 juri <at> linkov.net
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sun, 25 Jul 2021 12:16:42 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Date: Sun, 25 Jul 2021 08:41:14 +0200
> Cc: Juri Linkov <juri <at> linkov.net>, 39121 <at> debbugs.gnu.org,
>  Tino Calancha <tino.calancha <at> gmail.com>
> 
> Mattias Engdegård <mattiase <at> 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?

Message #57 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Tino Calancha <tino.calancha <at> gmail.com>, 39121 <at> debbugs.gnu.org,
 Juri Linkov <juri <at> linkov.net>
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sun, 25 Jul 2021 12:06:32 +0200

25 juli 2021 kl. 08.41 skrev Lars Ingebrigtsen <larsi <at> 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.

Message #60 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, tino.calancha <at> gmail.com,
 39121 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sun, 25 Jul 2021 12:55:47 +0200

25 juli 2021 kl. 11.16 skrev Eli Zaretskii <eliz <at> 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.

Message #63 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: 39121 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 tino.calancha <at> gmail.com, Lars Ingebrigtsen <larsi <at> gnus.org>, juri <at> linkov.net
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sun, 25 Jul 2021 12:39:16 +0100

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

> 25 juli 2021 kl. 11.16 skrev Eli Zaretskii <eliz <at> 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

Message #66 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: larsi <at> gnus.org, tino.calancha <at> gmail.com, 39121 <at> debbugs.gnu.org,
 juri <at> linkov.net
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sun, 25 Jul 2021 14:49:07 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Sun, 25 Jul 2021 12:55:47 +0200
> Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, juri <at> linkov.net, 39121 <at> debbugs.gnu.org,
>         tino.calancha <at> gmail.com
> 
> 25 juli 2021 kl. 11.16 skrev Eli Zaretskii <eliz <at> 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?

Message #69 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: 39121 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 tino.calancha <at> gmail.com, Lars Ingebrigtsen <larsi <at> gnus.org>, juri <at> linkov.net
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sun, 25 Jul 2021 16:45:11 +0200

25 juli 2021 kl. 13.39 skrev Basil L. Contovounesios <contovob <at> 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!

Message #72 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, tino.calancha <at> gmail.com, 39121 <at> debbugs.gnu.org,
 juri <at> linkov.net
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sun, 25 Jul 2021 17:09:37 +0200

25 juli 2021 kl. 13.49 skrev Eli Zaretskii <eliz <at> 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.

Message #75 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: larsi <at> gnus.org, tino.calancha <at> gmail.com, 39121 <at> debbugs.gnu.org,
 juri <at> linkov.net
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sun, 25 Jul 2021 19:27:08 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Sun, 25 Jul 2021 17:09:37 +0200
> Cc: larsi <at> gnus.org, juri <at> linkov.net, 39121 <at> debbugs.gnu.org,
>         tino.calancha <at> gmail.com
> 
> 25 juli 2021 kl. 13.49 skrev Eli Zaretskii <eliz <at> 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.

Message #78 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, tino.calancha <at> gmail.com, 39121 <at> debbugs.gnu.org,
 juri <at> linkov.net
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sun, 25 Jul 2021 20:54:47 +0200

25 juli 2021 kl. 18.27 skrev Eli Zaretskii <eliz <at> 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.

Message #81 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: larsi <at> gnus.org, tino.calancha <at> gmail.com, 39121 <at> debbugs.gnu.org,
 juri <at> linkov.net
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sun, 25 Jul 2021 22:23:26 +0300

> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Sun, 25 Jul 2021 20:54:47 +0200
> Cc: larsi <at> gnus.org, juri <at> linkov.net, 39121 <at> debbugs.gnu.org,
>         tino.calancha <at> gmail.com
> 
> 25 juli 2021 kl. 18.27 skrev Eli Zaretskii <eliz <at> 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

Message #84 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Mattias Engdegård <mattiase <at> acm.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, tino.calancha <at> gmail.com, 39121 <at> debbugs.gnu.org,
 juri <at> linkov.net
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Sun, 25 Jul 2021 21:30:14 +0200

25 juli 2021 kl. 21.23 skrev Eli Zaretskii <eliz <at> 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.

Message #87 received at 39121 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mattias Engdegård <mattiase <at> acm.org>
Cc: larsi <at> gnus.org, tino.calancha <at> gmail.com, 39121 <at> debbugs.gnu.org,
 juri <at> linkov.net
Subject: Re: bug#39121: 27.0.60; occur: Add bindings for  next-error-no-select
Date: Mon, 26 Jul 2021 15:43:42 +0300

> Feedback-ID:mattiase <at> acm.or
> From: Mattias Engdegård <mattiase <at> acm.org>
> Date: Sun, 25 Jul 2021 21:30:14 +0200
> Cc: larsi <at> gnus.org, juri <at> linkov.net, 39121 <at> debbugs.gnu.org,
>         tino.calancha <at> gmail.com
> 
> 25 juli 2021 kl. 21.23 skrev Eli Zaretskii <eliz <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.