bug#32676: [PATCH] Add option to highlight the 'next-error' error message

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

bug#32676: [PATCH] Add option to highlight the 'next-error' error message

Ernesto Alfonso
In addition to a fringe arrow, ‘next-error’ error may optionally
highlight the current error message in the ‘next-error’ buffer.
---
 doc/emacs/building.texi |  2 ++
 etc/NEWS                |  5 +++++
 lisp/simple.el          | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/doc/emacs/building.texi b/doc/emacs/building.texi
index 496c4275bc..e0d14c14d8 100644
--- a/doc/emacs/building.texi
+++ b/doc/emacs/building.texi
@@ -199,6 +199,8 @@ Compilation Mode
 @kindex M-g n
 @kindex C-x `
 @findex next-error
+@findex next-error-message
+@vindex next-error-message-highlight-p
 @vindex next-error-highlight
   To visit errors sequentially, type @w{@kbd{C-x `}}
 (@code{next-error}), or equivalently @kbd{M-g M-n} or @kbd{M-g n}.
diff --git a/etc/NEWS b/etc/NEWS
index ff65a5520d..a8d3400d8f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -607,6 +607,11 @@ error.
 It can be used to set any buffer as the next one to be used by
 'next-error' and 'previous-error'.
 
++++
+*** New customizable variable 'next-error-message-highlight-p'
+In addition to a fringe arrow, ‘next-error’ error may now optionally
+highlight the current error message in the ‘next-error’ buffer.
+
 ** nxml-mode
 
 ---
diff --git a/lisp/simple.el b/lisp/simple.el
index 0ccf2f1d22..c1298965cc 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -105,6 +105,23 @@ next-error-recenter
   :group 'next-error
   :version "23.1")
 
+(defcustom next-error-message-highlight-p nil
+  "If non-nil, highlight the current error message in the ‘next-error’ buffer"
+  :type 'boolean
+  :group 'next-error
+  :version "27.1")
+
+(defface next-error-message
+  '((t (:inherit highlight)))
+  "Face used to highlight the current error message in the ‘next-error’ buffer"
+  :group 'next-error
+  :version "27.1")
+
+(defvar next-error-message-highlight-overlay
+  nil
+  "Overlay highlighting the current error message in the ‘next-error’ buffer")
+(make-variable-buffer-local 'next-error-message-highlight-overlay)
+
 (defcustom next-error-hook nil
   "List of hook functions run by `next-error' after visiting source file."
   :type 'hook
@@ -421,6 +438,23 @@ next-error-follow-mode-post-command-hook
   (next-error-no-select 0))
       (error t))))
 
+(defun next-error-message-highlight ()
+  "Highlight the current error message in the ‘next-error’ buffer."
+  (when next-error-message-highlight-p
+    (with-current-buffer next-error-last-buffer
+      (when next-error-message-highlight-overlay
+        (delete-overlay next-error-message-highlight-overlay))
+      (save-excursion
+        (goto-char compilation-current-error)
+        (let ((ol (make-overlay (line-beginning-position) (line-end-position))))
+          ;; do not override region highlighting
+          (overlay-put ol 'priority -50)
+          (overlay-put ol 'face 'next-error-message)
+          (overlay-put ol 'window (get-buffer-window))
+          (setf next-error-message-highlight-overlay ol))))))
+
+(add-hook 'next-error-hook 'next-error-message-highlight)
+
 
 ;;;
 
--
2.11.0




Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Ernesto Alfonso
Dear maintainers,

I recently made feature request along with a patch and sent it to [hidden email] based on the contributor guidelends.

I wanted to provide a bit more context on the motivation behind this patch.

When scrolling through compilation errors, for some users the small fringe arrow indicator that appears next to the current error in the next-error buffer next to the error message can be hard to find quickly without without strain. I personally often find myself reading the wrong error message when fixing compilation errors.

This enhancement adds a customizable option to highlight the current error message in the next-error buffer in addition to the fringe arrow indicator. This makes for a visually more pleasing experience when locating the next-error current error message.

I include a demo below where I'm scrolling through several errors in a small c++ program.

For the implementation, I used the suggestion in Drew's answer on emacs stackexchange where another user raised the same concern about 2 years ago. 

The new behavior is off by default, and the font used to highlight the errors may also be customized.

Please let me know if the patch can be improved or what are the next steps in this review, and I look forward to this enhancement being part of the next emacs release.

demo.gif

Thanks,

Ernesto
Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Eli Zaretskii
[Please don't cross-post to emacs-devel.]

> From: Ernesto Alfonso <[hidden email]>
> Date: Thu, 13 Sep 2018 00:10:09 -0700
>
> I recently made feature request along with a patch and sent it to [hidden email] based on the
> contributor guidelends.

Thank you for your contribution.

I didn't yet have time to review your changes in detail, but one
question immediately popped up in my mind: why not use hl-line for
this?  I believe this was suggested in the stackexchange discussion,
but for some reason not followed up.



Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Robert Pluim
Eli Zaretskii <[hidden email]> writes:

> [Please don't cross-post to emacs-devel.]
>
>> From: Ernesto Alfonso <[hidden email]>
>> Date: Thu, 13 Sep 2018 00:10:09 -0700
>>
>> I recently made feature request along with a patch and sent it to [hidden email] based on the
>> contributor guidelends.
>
> Thank you for your contribution.
>
> I didn't yet have time to review your changes in detail, but one
> question immediately popped up in my mind: why not use hl-line for
> this?  I believe this was suggested in the stackexchange discussion,
> but for some reason not followed up.

Iʼve been using display-line-numbers + line-number-current-line face
for this, but hl-line works even better.

Robert



Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Ernesto Alfonso
Hi Eli,

I first tried using hl-line, and my patch did use hl-line as a reference.

The problem is that there are two independent* markers, point, and a marker at the beginning of the current error line in the next error buffer, for example compilation-current-error, where the fringe arrow is displayed.

In the same way that the user can move around the point in the next-error buffer between calls to {next,previous}-error without affecting the location of the fringe arrow, the user should also be able to move point around without affecting highlighting of the current error message (for example, to kill part of an error message in the compilation buffer), since this is really a visual enhancement to the fringe arrow.  

Basically, the difference is that hl-line uses post-command-hooks to track the current line and put an overlay on it, whereas in this case highlighting only changes whenever next-error-hook is invoked.

Below is an example of diverging point and compilation-current-error markers:


13-Sep-2018-07:41:28.png

*Markers can be independent until 'next-error is invoked, which moves point to the next error, but they can diverge between calls to next-error.

Thanks,

Ernesto

On Thu, Sep 13, 2018 at 7:14 AM Robert Pluim <[hidden email]> wrote:
Eli Zaretskii <[hidden email]> writes:

> [Please don't cross-post to emacs-devel.]
>
>> From: Ernesto Alfonso <[hidden email]>
>> Date: Thu, 13 Sep 2018 00:10:09 -0700
>>
>> I recently made feature request along with a patch and sent it to [hidden email] based on the
>> contributor guidelends.
>
> Thank you for your contribution.
>
> I didn't yet have time to review your changes in detail, but one
> question immediately popped up in my mind: why not use hl-line for
> this?  I believe this was suggested in the stackexchange discussion,
> but for some reason not followed up.

Iʼve been using display-line-numbers + line-number-current-line face
for this, but hl-line works even better.

Robert
Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Ernesto Alfonso
In reply to this post by Ernesto Alfonso
Replied all to the wrong thread -- removing emacs-devel

On Thu, Sep 13, 2018 at 9:10 AM Ernesto Alfonso <[hidden email]> wrote:
Another problem with hl-line is what the original poster pointed out in the screenshot below: hl-line only highlights on the current buffer's window, so if the user were to switch to the source code buffer (or if he wasn't there in the first place, e.g. by having invokied next-error form the source code buffer via a key binding) then highlighting of error messages is either lost or never happens.


13-Sep-2018-08:41:46.png


global hl line: no highlighting on next-error buffer if it is not current:

global-hl-line.gif

hl-line with next-buffer as current: highlighting not preserved after movement commands
hl-line-scrolling-on-next-buffer.gif

Thanks,

Ernesto

On Thu, Sep 13, 2018 at 12:10 AM Ernesto Alfonso <[hidden email]> wrote:
Dear maintainers,

I recently made feature request along with a patch and sent it to [hidden email] based on the contributor guidelends.

I wanted to provide a bit more context on the motivation behind this patch.

When scrolling through compilation errors, for some users the small fringe arrow indicator that appears next to the current error in the next-error buffer next to the error message can be hard to find quickly without without strain. I personally often find myself reading the wrong error message when fixing compilation errors.

This enhancement adds a customizable option to highlight the current error message in the next-error buffer in addition to the fringe arrow indicator. This makes for a visually more pleasing experience when locating the next-error current error message.

I include a demo below where I'm scrolling through several errors in a small c++ program.

For the implementation, I used the suggestion in Drew's answer on emacs stackexchange where another user raised the same concern about 2 years ago. 

The new behavior is off by default, and the font used to highlight the errors may also be customized.

Please let me know if the patch can be improved or what are the next steps in this review, and I look forward to this enhancement being part of the next emacs release.

demo.gif

Thanks,

Ernesto
Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Eli Zaretskii
In reply to this post by Ernesto Alfonso
> From: Ernesto Alfonso <[hidden email]>
> Date: Thu, 13 Sep 2018 08:02:48 -0700
> Cc: Eli Zaretskii <[hidden email]>, [hidden email]
>
> The problem is that there are two independent* markers, point, and a marker at the beginning of the current
> error line in the next error buffer, for example compilation-current-error, where the fringe arrow is displayed.
>
> In the same way that the user can move around the point in the next-error buffer between calls to
> {next,previous}-error without affecting the location of the fringe arrow, the user should also be able to move
> point around without affecting highlighting of the current error message (for example, to kill part of an error
> message in the compilation buffer), since this is really a visual enhancement to the fringe arrow.  

You should be able to fix this problem by setting
hl-line-range-function to a suitable function (which should be quite
simple, AFAIU).

> Another problem with hl-line is what the original poster pointed out in the screenshot below: hl-line only
> highlights on the current buffer's window, so if the user were to switch to the source code buffer (or if he
> wasn't there in the first place, e.g. by having invokied next-error form the source code buffer via a key
> binding) then highlighting of error messages is either lost or never happens.

This is only true for the global-hl-line-mode; the local mode's
highlight is "sticky" by default, and shows even in non-selected
windows.

Moreover, you can customize the global mode so that its highlight is
sticky as well (not that I see why would you want to in this case).

> Basically, the difference is that hl-line uses post-command-hooks to track the current line and put an overlay
> on it, whereas in this case highlighting only changes whenever next-error-hook is invoked.

Is this really important?  Those are just implementation details, no?



Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Ernesto Alfonso
> You should be able to fix this problem by setting
> hl-line-range-function to a suitable function (which should be quite
> simple, AFAIU).

Not really. I tried, setting hl-line-range-function to the next-error
buffer message line after turning on hl-line:

> (with-current-buffer next-error-last-buffer
>     (make-variable-buffer-local 'hl-line-range-function)
>     (setf hl-line-range-function
>           (lambda ()
>             (save-excursion
>               (goto-char compilation-current-error)
>               (let ((range
>                      (cons (line-beginning-position) (line-end-position))))
>                 (message "hl-line-range-function caled. range is %s" range)
>                 range)))))

See gif below where hl-line-function is not called after commands invoked outside of the next-error buffer:



> Basically, the difference is that hl-line uses post-command-hooks to track the current line and put an overlay
> on it, whereas in this case highlighting only changes whenever next-error-hook is invoked.
>>
>> Is this really important?  Those are just implementation details, no?

No, this is exactly the reason why hl-line-range-function doesn't work in the above example. These are
different concepts with different hooks involved that are invoked under different conditions.

post-command-hook means hook is invoked after movement commands, which should not affect err msg line
highlighting, it also means that it may not necessarily be invoked upon next-error. 

hl-line-mode hooks:
>   (if hl-line-mode
>       (progn
>         ;; In case `kill-all-local-variables' is called.
>         (add-hook 'change-major-mode-hook #'hl-line-unhighlight nil t)
>         (hl-line-highlight)
>         (setq hl-line-overlay-buffer (current-buffer))
(add-hook 'post-command-hook #'hl-line-highlight nil t)
>         (add-hook 'post-command-hook #'hl-line-maybe-unhighlight nil t))
>     (remove-hook 'post-command-hook #'hl-line-highlight t)
>     (hl-line-unhighlight)
>     (remove-hook 'change-major-mode-hook #'hl-line-unhighlight t)
>     (remove-hook 'post-command-hook #'hl-line-maybe-unhighlight t)))

Whereas for this enhancement, the only event that affects highlight region is next-error.

Additionally, hl-line and error message highlight and face should be independent:
the user may want current-line highlighting in addition to error message highlighting.


Ernesto
On Thu, Sep 13, 2018, 9:44 AM Eli Zaretskii <[hidden email]> wrote:
> From: Ernesto Alfonso <[hidden email]>
> Date: Thu, 13 Sep 2018 08:02:48 -0700
> Cc: Eli Zaretskii <[hidden email]>, [hidden email]
>
> The problem is that there are two independent* markers, point, and a marker at the beginning of the current
> error line in the next error buffer, for example compilation-current-error, where the fringe arrow is displayed.
>
> In the same way that the user can move around the point in the next-error buffer between calls to
> {next,previous}-error without affecting the location of the fringe arrow, the user should also be able to move
> point around without affecting highlighting of the current error message (for example, to kill part of an error
> message in the compilation buffer), since this is really a visual enhancement to the fringe arrow. 

You should be able to fix this problem by setting
hl-line-range-function to a suitable function (which should be quite
simple, AFAIU).

> Another problem with hl-line is what the original poster pointed out in the screenshot below: hl-line only
> highlights on the current buffer's window, so if the user were to switch to the source code buffer (or if he
> wasn't there in the first place, e.g. by having invokied next-error form the source code buffer via a key
> binding) then highlighting of error messages is either lost or never happens.

This is only true for the global-hl-line-mode; the local mode's
highlight is "sticky" by default, and shows even in non-selected
windows.

Moreover, you can customize the global mode so that its highlight is
sticky as well (not that I see why would you want to in this case).

> Basically, the difference is that hl-line uses post-command-hooks to track the current line and put an overlay
> on it, whereas in this case highlighting only changes whenever next-error-hook is invoked.

Is this really important?  Those are just implementation details, no?
Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Ernesto Alfonso
Attaching the last gif as an inline/attachment instead of an external link. This was the attempt to use hl-line-range-function. It did not work for me.

On Thu, Sep 13, 2018 at 11:18 AM Ernesto Alfonso <[hidden email]> wrote:
> You should be able to fix this problem by setting
> hl-line-range-function to a suitable function (which should be quite
> simple, AFAIU).

Not really. I tried, setting hl-line-range-function to the next-error
buffer message line after turning on hl-line:

> (with-current-buffer next-error-last-buffer
>     (make-variable-buffer-local 'hl-line-range-function)
>     (setf hl-line-range-function
>           (lambda ()
>             (save-excursion
>               (goto-char compilation-current-error)
>               (let ((range
>                      (cons (line-beginning-position) (line-end-position))))
>                 (message "hl-line-range-function caled. range is %s" range)
>                 range)))))

See gif below where hl-line-function is not called after commands invoked outside of the next-error buffer:



> Basically, the difference is that hl-line uses post-command-hooks to track the current line and put an overlay
> on it, whereas in this case highlighting only changes whenever next-error-hook is invoked.
>>
>> Is this really important?  Those are just implementation details, no?

No, this is exactly the reason why hl-line-range-function doesn't work in the above example. These are
different concepts with different hooks involved that are invoked under different conditions.

post-command-hook means hook is invoked after movement commands, which should not affect err msg line
highlighting, it also means that it may not necessarily be invoked upon next-error. 

hl-line-mode hooks:
>   (if hl-line-mode
>       (progn
>         ;; In case `kill-all-local-variables' is called.
>         (add-hook 'change-major-mode-hook #'hl-line-unhighlight nil t)
>         (hl-line-highlight)
>         (setq hl-line-overlay-buffer (current-buffer))
(add-hook 'post-command-hook #'hl-line-highlight nil t)
>         (add-hook 'post-command-hook #'hl-line-maybe-unhighlight nil t))
>     (remove-hook 'post-command-hook #'hl-line-highlight t)
>     (hl-line-unhighlight)
>     (remove-hook 'change-major-mode-hook #'hl-line-unhighlight t)
>     (remove-hook 'post-command-hook #'hl-line-maybe-unhighlight t)))

Whereas for this enhancement, the only event that affects highlight region is next-error.

Additionally, hl-line and error message highlight and face should be independent:
the user may want current-line highlighting in addition to error message highlighting.


Ernesto
On Thu, Sep 13, 2018, 9:44 AM Eli Zaretskii <[hidden email]> wrote:
> From: Ernesto Alfonso <[hidden email]>
> Date: Thu, 13 Sep 2018 08:02:48 -0700
> Cc: Eli Zaretskii <[hidden email]>, [hidden email]
>
> The problem is that there are two independent* markers, point, and a marker at the beginning of the current
> error line in the next error buffer, for example compilation-current-error, where the fringe arrow is displayed.
>
> In the same way that the user can move around the point in the next-error buffer between calls to
> {next,previous}-error without affecting the location of the fringe arrow, the user should also be able to move
> point around without affecting highlighting of the current error message (for example, to kill part of an error
> message in the compilation buffer), since this is really a visual enhancement to the fringe arrow. 

You should be able to fix this problem by setting
hl-line-range-function to a suitable function (which should be quite
simple, AFAIU).

> Another problem with hl-line is what the original poster pointed out in the screenshot below: hl-line only
> highlights on the current buffer's window, so if the user were to switch to the source code buffer (or if he
> wasn't there in the first place, e.g. by having invokied next-error form the source code buffer via a key
> binding) then highlighting of error messages is either lost or never happens.

This is only true for the global-hl-line-mode; the local mode's
highlight is "sticky" by default, and shows even in non-selected
windows.

Moreover, you can customize the global mode so that its highlight is
sticky as well (not that I see why would you want to in this case).

> Basically, the difference is that hl-line uses post-command-hooks to track the current line and put an overlay
> on it, whereas in this case highlighting only changes whenever next-error-hook is invoked.

Is this really important?  Those are just implementation details, no?

highlight-line.gif (976K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Juri Linkov-2
> Attaching the last gif as an inline/attachment instead of an external link.
> This was the attempt to use hl-line-range-function. It did not work for me.

Do I understand correctly that your proposed feature is like next-error-highlight,
but instead of highlighting the error locus, it highlights the error message?
Then I don't understand how it relates to compilation-highlight-regexp and
compilation-highlight-overlay?



Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Ernesto Alfonso

Do I understand correctly that your proposed feature is like next-error-highlight,
but instead of highlighting the error locus, it highlights the error message?
Yes, although I don't think it's important to support a timer to turn off highlighting like next-error-highlight does since the error locus highlighting might get in the user's way in source buffers, but not in the next-error buffer.

 Then I don't understand how it relates to compilation-highlight-regexp and
compilation-highlight-overlay?
 
I don't see a reference to compilation-highlight-regexp or compilation-highlight-overlay in my patch or in this thread. Although I did use compilation-current-error to find the mark at the error message location, which I think is not appropriate since that would be compilation-specific, and I think it should be (point) instead. Is this what you mean?

Ernesto



On Sat, Sep 15, 2018 at 4:49 PM Juri Linkov <[hidden email]> wrote:
> Attaching the last gif as an inline/attachment instead of an external link.
> This was the attempt to use hl-line-range-function. It did not work for me.

Do I understand correctly that your proposed feature is like next-error-highlight,
but instead of highlighting the error locus, it highlights the error message?
Then I don't understand how it relates to compilation-highlight-regexp and
compilation-highlight-overlay?
Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Juri Linkov-2
>> Do I understand correctly that your proposed feature is like
>> next-error-highlight,
>> but instead of highlighting the error locus, it highlights the error
>> message?
>
> Yes, although I don't think it's important to support a timer to turn off
> highlighting like next-error-highlight does since the error locus
> highlighting might get in the user's way in source buffers, but not in the
> next-error buffer.

I agree that a timer is not necessary in next-error-last-buffer.
I just wanted to emphasize that your new customization could be like
next-error-highlight defcustom (but without a timer option).

> I don't see a reference to compilation-highlight-regexp or
> compilation-highlight-overlay in my patch or in this thread. Although I did
> use compilation-current-error to find the mark at the error message
> location, which I think is not appropriate since that would be
> compilation-specific, and I think it should be (point) instead. Is this
> what you mean?

I guess you need to highlight exactly the same place from where
the error was visited.  It looks like (point) is always located
at this place because compilation-next-error-function sets
overlay-arrow-position to (point-marker), so it should be the
right place to highlight indeed.



Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Ernesto Alfonso

I agree that a timer is not necessary in next-error-last-buffer.
I just wanted to emphasize that your new customization could be like
next-error-highlight defcustom (but without a timer option).
Yes.

It looks like (point) is always located
at this place because compilation-next-error-function sets
overlay-arrow-position to (point-marker), so it should be the
right place to highlight indeed.
Yes. I tried replacing compilation-current-error with point and it works as expected. I am not sure how I would update the patch.

 
While searching for this thread, I came across another time qhwn a user made a query for the same feature I am proposing.


It provides another explanation of why hl-line doesn't work well in this case.

Ernesto
On Sun, Sep 16, 2018 at 4:38 PM Juri Linkov <[hidden email]> wrote:
>> Do I understand correctly that your proposed feature is like
>> next-error-highlight,
>> but instead of highlighting the error locus, it highlights the error
>> message?
>
> Yes, although I don't think it's important to support a timer to turn off
> highlighting like next-error-highlight does since the error locus
> highlighting might get in the user's way in source buffers, but not in the
> next-error buffer.

I agree that a timer is not necessary in next-error-last-buffer.
I just wanted to emphasize that your new customization could be like
next-error-highlight defcustom (but without a timer option).

> I don't see a reference to compilation-highlight-regexp or
> compilation-highlight-overlay in my patch or in this thread. Although I did
> use compilation-current-error to find the mark at the error message
> location, which I think is not appropriate since that would be
> compilation-specific, and I think it should be (point) instead. Is this
> what you mean?

I guess you need to highlight exactly the same place from where
the error was visited.  It looks like (point) is always located
at this place because compilation-next-error-function sets
overlay-arrow-position to (point-marker), so it should be the
right place to highlight indeed.
Reply | Threaded
Open this post in threaded view
|

bug#32676: Feature request

Ernesto Alfonso
I'd like to know if this patch is still being considered?

Ernesto

On Tue, Sep 18, 2018 at 1:51 AM Ernesto Alfonso <[hidden email]> wrote:

I agree that a timer is not necessary in next-error-last-buffer.
I just wanted to emphasize that your new customization could be like
next-error-highlight defcustom (but without a timer option).
Yes.

It looks like (point) is always located
at this place because compilation-next-error-function sets
overlay-arrow-position to (point-marker), so it should be the
right place to highlight indeed.
Yes. I tried replacing compilation-current-error with point and it works as expected. I am not sure how I would update the patch.

 
While searching for this thread, I came across another time qhwn a user made a query for the same feature I am proposing.


It provides another explanation of why hl-line doesn't work well in this case.

Ernesto
On Sun, Sep 16, 2018 at 4:38 PM Juri Linkov <[hidden email]> wrote:
>> Do I understand correctly that your proposed feature is like
>> next-error-highlight,
>> but instead of highlighting the error locus, it highlights the error
>> message?
>
> Yes, although I don't think it's important to support a timer to turn off
> highlighting like next-error-highlight does since the error locus
> highlighting might get in the user's way in source buffers, but not in the
> next-error buffer.

I agree that a timer is not necessary in next-error-last-buffer.
I just wanted to emphasize that your new customization could be like
next-error-highlight defcustom (but without a timer option).

> I don't see a reference to compilation-highlight-regexp or
> compilation-highlight-overlay in my patch or in this thread. Although I did
> use compilation-current-error to find the mark at the error message
> location, which I think is not appropriate since that would be
> compilation-specific, and I think it should be (point) instead. Is this
> what you mean?

I guess you need to highlight exactly the same place from where
the error was visited.  It looks like (point) is always located
at this place because compilation-next-error-function sets
overlay-arrow-position to (point-marker), so it should be the
right place to highlight indeed.
Reply | Threaded
Open this post in threaded view
|

bug#32676: [PATCH] Add option to highlight the 'next-error' error message

Juri Linkov-2
> I'd like to know if this patch is still being considered?

Why not?  Your patch provides a helpful feature.  I see only 2 problems
with its latest version:

1. compilation-current-error should be generalized not to be too
   compilation-specific;

2. next-error-hook should not be used for core features,
   you could call next-error-message-highlight directly
   from next-error-found.

PS: maybe a better name for defcustom would be next-error-message-highlight,
    not next-error-message-highlight-p, to be more future-proof,
    for the case when someone might want to add more choices later
    (e.g. fringe, timers, etc.)



Reply | Threaded
Open this post in threaded view
|

bug#32676: [PATCH] Add option to highlight the 'next-error' error message

Eli Zaretskii
> From: Juri Linkov <[hidden email]>
> Cc: Eli Zaretskii <[hidden email]>,  Robert Pluim <[hidden email]>,  [hidden email]
> Date: Mon, 08 Apr 2019 22:36:38 +0300
>
> > I'd like to know if this patch is still being considered?
>
> Why not?  Your patch provides a helpful feature.  I see only 2 problems
> with its latest version:
>
> 1. compilation-current-error should be generalized not to be too
>    compilation-specific;
>
> 2. next-error-hook should not be used for core features,
>    you could call next-error-message-highlight directly
>    from next-error-found.

3. A contribution this size needs legal paperwork for us to be able to
accept it.  Ernesto, would you like to start the paperwork now?

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#32676: [PATCH] Add option to highlight the 'next-error' error message

Lars Ingebrigtsen
Stefan Kangas <[hidden email]> writes:

> To accept your contribution to Emacs, we need to you to assign
> copyright to the FSF.  You can read more about the reasons why here:
> https://www.gnu.org/licenses/why-assign.html
>
> Before we can make any progress with this patch, I think we need to
> clarify that point.  Would you be willing to sign such paperwork?

This was half a year ago, and I can't see Ernesto in the copyright
assignment file.

Ernesto?

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



Reply | Threaded
Open this post in threaded view
|

bug#32676: [PATCH] Add option to highlight the 'next-error' error message

Ernesto Alfonso
I currently work for Google, and my understanding is that Google has a special agreement with the FSF. Please let me know if this is correct or if I still need to provide a copyright assignment.

Also, I am not sure if this patch is still applicable.

Thanks,

Ernesto

On Mon, Aug 10, 2020 at 10:00 AM Ernesto Alfonso <[hidden email]> wrote:
Sorry about the delay, I will get back to this soon.

Ernesto

On Mon, Aug 10, 2020 at 5:38 AM Lars Ingebrigtsen <[hidden email]> wrote:
Stefan Kangas <[hidden email]> writes:

> To accept your contribution to Emacs, we need to you to assign
> copyright to the FSF.  You can read more about the reasons why here:
> https://www.gnu.org/licenses/why-assign.html
>
> Before we can make any progress with this patch, I think we need to
> clarify that point.  Would you be willing to sign such paperwork?

This was half a year ago, and I can't see Ernesto in the copyright
assignment file.

Ernesto?

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no
Reply | Threaded
Open this post in threaded view
|

bug#32676: [PATCH] Add option to highlight the 'next-error' error message

Lars Ingebrigtsen
In reply to this post by Juri Linkov-2
Juri Linkov <[hidden email]> writes:

>> I'd like to know if this patch is still being considered?
>
> Why not?  Your patch provides a helpful feature.  I see only 2 problems
> with its latest version:

I've now applied the patch (after reworking slightly), and it seems to
work well, so I've pushed it to Emacs 28.

> 1. compilation-current-error should be generalized not to be too
>    compilation-specific;

Reading the code, it looks like that variable is always set, even in
grep buffers and the like?  So it's a bad name, but...

Is there a different variable that should be used instead?

> 2. next-error-hook should not be used for core features,
>    you could call next-error-message-highlight directly
>    from next-error-found.

Fixes.

> PS: maybe a better name for defcustom would be next-error-message-highlight,
>     not next-error-message-highlight-p, to be more future-proof,
>     for the case when someone might want to add more choices later
>     (e.g. fringe, timers, etc.)

And this.

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



Reply | Threaded
Open this post in threaded view
|

bug#32676: [PATCH] Add option to highlight the 'next-error' error message

Juri Linkov-2
> I've now applied the patch (after reworking slightly), and it seems to
> work well, so I've pushed it to Emacs 28.
>
>> 1. compilation-current-error should be generalized not to be too
>>    compilation-specific;
>
> Reading the code, it looks like that variable is always set, even in
> grep buffers and the like?  So it's a bad name, but...

Unsurprisingly, now using occur fails with the error

  next-error-found: Symbol’s value as variable is void: compilation-current-error

and when compile.el is loaded, occur fails with the error

  next-error-found: Wrong type argument: integer-or-marker-p, nil

This fails only when next-error-message-highlight is customized to t,
so there is no need to revert the patch, but it should be fixed ASAP.

> Is there a different variable that should be used instead?

I can't find another variable.  Maybe a new variable should be created,
with a name like next-error-current.

Then compilation-current-error could be declared as an obsolete alias.
Or maybe there is no need to remove the old variable, so they both
could be used: compilation-current-error in compilation-mode,
and next-error-current elsewhere.

PS: I see compilation-current-error is used also in
next-error-follow-mode-post-command-hook, and it works fine
when 'C-c C-f' (next-error-follow-minor-mode)
in enabled in occur buffers.



12