bug#57876: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

bug#57876: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

[Augusto Stoffel]

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

Tags: patch

Currently, if point is say at the prompt of a comint buffer,
pulse-momentary-highlight-one-line doesn't show do anything – or, more
precisely, it pulses the zero-length region from prompt end to EOB.

Prot: I think you want to apply a similar change in pulsar.el.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-pulse-momentary-highlight-one-line-Act-on-visual-lin.patch --]
[-- Type: text/patch, Size: 1157 bytes --]

From e1241510fc9e0deddc5adb416aec5de4ca0300c8 Mon Sep 17 00:00:00 2001
From: Augusto Stoffel <arstoffel@gmail.com>
Date: Sat, 17 Sep 2022 10:17:34 +0200
Subject: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

* lisp/cedet/pulse.el (pulse-momentary-highlight-one-line):  Act on
entire visual line, ignoring fields etc.
---
 lisp/cedet/pulse.el | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/lisp/cedet/pulse.el b/lisp/cedet/pulse.el
index 9941f2a0cb..0564cf6d04 100644
--- a/lisp/cedet/pulse.el
+++ b/lisp/cedet/pulse.el
@@ -202,12 +202,8 @@ pulse-momentary-highlight-one-line
 Optional argument FACE specifies the face to do the highlighting."
   (save-excursion
     (goto-char (or point (point)))
-    (let ((start (line-beginning-position))
-          (end (save-excursion
-                 (end-of-line)
-                 (when (not (eobp))
-                   (forward-char 1))
-                 (point))))
+    (let ((start (progn (vertical-motion 0) (point)))
+          (end (progn (vertical-motion 1) (point))))
       (pulse-momentary-highlight-region start end face))))
 
 ;;;###autoload
-- 
2.37.3

bug#57876: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

[Eli Zaretskii]

> Cc: Protesilaos Stavrou <public@protesilaos.com>
> From: Augusto Stoffel <arstoffel@gmail.com>
> Date: Sat, 17 Sep 2022 10:23:01 +0200
> 
> Tags: patch
> 
> Currently, if point is say at the prompt of a comint buffer,
> pulse-momentary-highlight-one-line doesn't show do anything – or, more
> precisely, it pulses the zero-length region from prompt end to EOB.
> 
> Prot: I think you want to apply a similar change in pulsar.el.
> 
> 
> >From e1241510fc9e0deddc5adb416aec5de4ca0300c8 Mon Sep 17 00:00:00 2001
> From: Augusto Stoffel <arstoffel@gmail.com>
> Date: Sat, 17 Sep 2022 10:17:34 +0200
> Subject: [PATCH] pulse-momentary-highlight-one-line: Act on visual line
> 
> * lisp/cedet/pulse.el (pulse-momentary-highlight-one-line):  Act on
> entire visual line, ignoring fields etc.

Why don't you use beginning/end-of-visual-line?

bug#57876: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

[Augusto Stoffel]

On Sat, 17 Sep 2022 at 12:38, Eli Zaretskii wrote:

>> Cc: Protesilaos Stavrou <public@protesilaos.com>
>> From: Augusto Stoffel <arstoffel@gmail.com>
>> Date: Sat, 17 Sep 2022 10:23:01 +0200
>> 
>> Tags: patch
>> 
>> Currently, if point is say at the prompt of a comint buffer,
>> pulse-momentary-highlight-one-line doesn't show do anything – or, more
>> precisely, it pulses the zero-length region from prompt end to EOB.
>> 
>> Prot: I think you want to apply a similar change in pulsar.el.
>> 
>> 
>> >From e1241510fc9e0deddc5adb416aec5de4ca0300c8 Mon Sep 17 00:00:00 2001
>> From: Augusto Stoffel <arstoffel@gmail.com>
>> Date: Sat, 17 Sep 2022 10:17:34 +0200
>> Subject: [PATCH] pulse-momentary-highlight-one-line: Act on visual line
>> 
>> * lisp/cedet/pulse.el (pulse-momentary-highlight-one-line):  Act on
>> entire visual line, ignoring fields etc.
>
> Why don't you use beginning/end-of-visual-line?

Because it obeys fields, so if you try to highlight a prompt line in a
comint buffer, only part of the line would be highlighted.

The (only, AFAICT) purpose of pulse-momentary-highlight-one-line is to
locate the cursor, so you want to pulse a little rectangle spanning the
entire width of the window.

bug#57876: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: 57876@debbugs.gnu.org,  public@protesilaos.com
> Date: Sat, 17 Sep 2022 12:56:44 +0200
> 
> > Why don't you use beginning/end-of-visual-line?
> 
> Because it obeys fields, so if you try to highlight a prompt line in a
> comint buffer, only part of the line would be highlighted.

It should be possible to disable fields temporarily by binding
inhibit-point-motion-hooks, right?  I prefer doing that to reinventing
the wheel of moving to the edges of the visible line.

bug#57876: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

[Augusto Stoffel]

On Sat, 17 Sep 2022 at 14:20, Eli Zaretskii wrote:

>> From: Augusto Stoffel <arstoffel@gmail.com>
>> Cc: 57876@debbugs.gnu.org,  public@protesilaos.com
>> Date: Sat, 17 Sep 2022 12:56:44 +0200
>> 
>> > Why don't you use beginning/end-of-visual-line?
>> 
>> Because it obeys fields, so if you try to highlight a prompt line in a
>> comint buffer, only part of the line would be highlighted.
>
> It should be possible to disable fields temporarily by binding
> inhibit-point-motion-hooks, right?  I prefer doing that to reinventing
> the wheel of moving to the edges of the visible line.

This variable is allegedly obsolete since Emacs 25.  In any case, the
patch is really trivial, so how would this be making the code simpler or
more robust?

bug#57876: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

[Eli Zaretskii]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Cc: 57876@debbugs.gnu.org,  public@protesilaos.com
> Date: Sat, 17 Sep 2022 13:31:55 +0200
> 
> > It should be possible to disable fields temporarily by binding
> > inhibit-point-motion-hooks, right?  I prefer doing that to reinventing
> > the wheel of moving to the edges of the visible line.
> 
> This variable is allegedly obsolete since Emacs 25.

Sorry, I meant inhibit-field-text-motion.

> In any case, the patch is really trivial, so how would this be
> making the code simpler or more robust?

I just explained the reasons above: we have these utility functions
for a reason: so that Lisp programs won't need to reinvent how to do
what these utilities do every time.

bug#57876: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

[Protesilaos Stavrou]

> From: Augusto Stoffel <arstoffel@gmail.com>
> Date: Sat, 17 Sep 2022 10:23:01 +0200
>
> Tags: patch
>
> Currently, if point is say at the prompt of a comint buffer,
> pulse-momentary-highlight-one-line doesn't show do anything – or, more
> precisely, it pulses the zero-length region from prompt end to EOB.
>
> Prot: I think you want to apply a similar change in pulsar.el.

Hello Augusto,

Just to note that I am monitoring this thread and will act accordingly.

Thank you!

Prot

-- 
Protesilaos Stavrou
https://protesilaos.com

bug#57876: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

[Lars Ingebrigtsen]

Augusto Stoffel <arstoffel@gmail.com> writes:

> Currently, if point is say at the prompt of a comint buffer,
> pulse-momentary-highlight-one-line doesn't show do anything – or, more
> precisely, it pulses the zero-length region from prompt end to EOB.

Thanks; pushed to Emacs 29.

Eli Zaretskii <eliz@gnu.org> writes:

>> This variable is allegedly obsolete since Emacs 25.
>
> Sorry, I meant inhibit-field-text-motion.

That variable does not affect what beginning-of-visual-line does
w.r.t. fields, only invisible text.

bug#57876: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

[Eli Zaretskii]

> Cc: Protesilaos Stavrou <public@protesilaos.com>, 57876@debbugs.gnu.org
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 18 Sep 2022 13:02:11 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> This variable is allegedly obsolete since Emacs 25.
> >
> > Sorry, I meant inhibit-field-text-motion.
> 
> That variable does not affect what beginning-of-visual-line does
> w.r.t. fields, only invisible text.

That's not what the doc string of inhibit-field-text-motion does, so
it might be a bug in itself.  line-beginning-position, for example,
does ignore fields when that variable is non-nil.

bug#57876: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> >> This variable is allegedly obsolete since Emacs 25.
>> >
>> > Sorry, I meant inhibit-field-text-motion.
>> 
>> That variable does not affect what beginning-of-visual-line does
>> w.r.t. fields, only invisible text.
>
> That's not what the doc string of inhibit-field-text-motion does, so
> it might be a bug in itself.  line-beginning-position, for example,
> does ignore fields when that variable is non-nil.

Oh, sorry!  I was confusing myself by testing with

(let ((inhibit-point-motion-hooks t)) (beginning-of-visual-line))

which is the wrong thing, of course -- with inhibit-field-text-motion it
works fine.

So adjusting the patch to use that instead might be appropriate (but
doesn't make much difference).

bug#57876: [PATCH] pulse-momentary-highlight-one-line: Act on visual line

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: arstoffel@gmail.com,  public@protesilaos.com,  57876@debbugs.gnu.org
> Date: Sun, 18 Sep 2022 14:45:33 +0200
> 
> Oh, sorry!  I was confusing myself by testing with
> 
> (let ((inhibit-point-motion-hooks t)) (beginning-of-visual-line))
> 
> which is the wrong thing, of course -- with inhibit-field-text-motion it
> works fine.
> 
> So adjusting the patch to use that instead might be appropriate (but
> doesn't make much difference).

My point in suggesting that was that it is better to use utility
functions we have rather than the equivalent code, because the utility
functions presumably serve as a locus of all our wisdom related to
what they do.  For example, what does "beginning of visual line" mean
with bidirectional text?  Instead of having to decide this in each
case, we have a function that already does TRT.