Discussion:
bug#32676: Feature request
(too old to reply)
Eli Zaretskii
2018-09-13 13:59:49 UTC
Permalink
[Please don't cross-post to emacs-devel.]
Date: Thu, 13 Sep 2018 00:10:09 -0700
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.
Robert Pluim
2018-09-13 14:14:23 UTC
Permalink
Post by Eli Zaretskii
[Please don't cross-post to emacs-devel.]
Date: Thu, 13 Sep 2018 00:10:09 -0700
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
Eli Zaretskii
2018-09-13 16:44:04 UTC
Permalink
Date: Thu, 13 Sep 2018 08:02:48 -0700
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?
Ernesto Alfonso
2018-09-13 18:18:13 UTC
Permalink
Post by Eli Zaretskii
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
Post by Eli Zaretskii
(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))))
Post by Eli Zaretskii
(message "hl-line-range-function caled. range is %s"
range)
Post by Eli Zaretskii
range)))))
See gif below where hl-line-function is not called after commands invoked
outside of the next-error buffer:


highlight-line.gif
<https://drive.google.com/file/d/0ByCqoLLtc4gqSS1pZTQ2WGZWdHI5Qml6MTd1MUFSQm45WjFF/view?usp=drivesdk>
Post by Eli Zaretskii
Basically, the difference is that hl-line uses post-command-hooks to
track the current line and put an overlay
Post by Eli Zaretskii
on it, whereas in this case highlighting only changes whenever
next-error-hook is invoked.
Post by Eli Zaretskii
Post by Eli Zaretskii
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.
Post by Eli Zaretskii
(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
Post by Eli Zaretskii
Post by Eli Zaretskii
Date: Thu, 13 Sep 2018 08:02:48 -0700
The problem is that there are two independent* markers, point, and a
marker at the beginning of the current
Post by Eli Zaretskii
error line in the next error buffer, for example
compilation-current-error, where the fringe arrow is displayed.
Post by Eli Zaretskii
In the same way that the user can move around the point in the
next-error buffer between calls to
Post by Eli Zaretskii
{next,previous}-error without affecting the location of the fringe
arrow, the user should also be able to move
Post by Eli Zaretskii
point around without affecting highlighting of the current error message
(for example, to kill part of an error
Post by Eli Zaretskii
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).
Post by Eli Zaretskii
Another problem with hl-line is what the original poster pointed out in
the screenshot below: hl-line only
Post by Eli Zaretskii
highlights on the current buffer's window, so if the user were to switch
to the source code buffer (or if he
Post by Eli Zaretskii
wasn't there in the first place, e.g. by having invokied next-error form
the source code buffer via a key
Post by Eli Zaretskii
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).
Post by Eli Zaretskii
Basically, the difference is that hl-line uses post-command-hooks to
track the current line and put an overlay
Post by Eli Zaretskii
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?
Juri Linkov
2018-09-15 23:05:15 UTC
Permalink
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?
Ernesto Alfonso
2018-09-16 00:37:12 UTC
Permalink
Post by Juri Linkov
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
Post by Juri Linkov
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
Post by Juri Linkov
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?
Juri Linkov
2018-09-16 23:27:23 UTC
Permalink
Post by Ernesto Alfonso
Post by Juri Linkov
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).
Post by Ernesto Alfonso
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.
Ernesto Alfonso
2018-09-18 08:51:04 UTC
Permalink
Post by Juri Linkov
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
Post by Juri Linkov
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.

https://groups.google.com/forum/#!topic/gnu.emacs.help/q1QucyB1Nzw

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

Ernesto
Post by Juri Linkov
Post by Ernesto Alfonso
Post by Juri Linkov
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
Post by Ernesto Alfonso
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).
Post by Ernesto Alfonso
I don't see a reference to compilation-highlight-regexp or
compilation-highlight-overlay in my patch or in this thread. Although I
did
Post by Ernesto Alfonso
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.
Stefan Kangas
2020-02-29 15:40:01 UTC
Permalink
Hi Ernesto,

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?

Thanks in advance.

Best regards,
Stefan Kangas
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
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.
Lars Ingebrigtsen
2020-08-10 12:38:47 UTC
Permalink
Post by Stefan Kangas
To accept your contribution to Emacs, we need to you to assign
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
Ernesto Alfonso
2020-08-10 14:00:56 UTC
Permalink
Sorry about the delay, I will get back to this soon.

Ernesto
Post by Lars Ingebrigtsen
Post by Stefan Kangas
To accept your contribution to Emacs, we need to you to assign
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
Ernesto Alfonso
2020-09-03 05:00:16 UTC
Permalink
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
Post by Ernesto Alfonso
Sorry about the delay, I will get back to this soon.
Ernesto
Post by Lars Ingebrigtsen
Post by Stefan Kangas
To accept your contribution to Emacs, we need to you to assign
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
Stefan Kangas
2020-09-03 10:40:30 UTC
Permalink
Hi Ernesto,

Thanks for following up on this.
Post by 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.
Someone else will have to answer this.
Post by Ernesto Alfonso
Also, I am not sure if this patch is still applicable.
In what way? From reading the discussion, it seems like the feature was
considered useful, but there were some additional comments before it was
ready. I copied in those comments below. Could you have a look at
them?

Thanks in advance.
Post by Ernesto Alfonso
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
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.)
Best regards,
Stefan Kangas

Loading...