bug#36550: Small bug fix in recentf.el

bug#36550: Small bug fix in recentf.el

[Linus Källberg]

Hello,

In recentf.el (GNU Emacs 26.1 (build 1, x86_64-w64-mingw32) of 
2018-05-30), I made the following small change on line 1187 (in the 
function recentf-open-files-item):

-           :format "%[%t\n%]"
+           :format "%[%t%]\n"

This fixes the highlighting when hovering the mouse over a file in the 
recent files dialog. Before, the file name, as well as the rest of the 
line and the first character on the next line, was highlighted. Now only 
the file name is highlighted, which looks much nicer and is probably 
what is intended.

Yours Sincerely,
Linus Källberg

bug#36550: Small bug fix in recentf.el

[Lars Ingebrigtsen]

Linus Källberg <linus.kallberg@outlook.com> writes:

> Hello,
>
> In recentf.el (GNU Emacs 26.1 (build 1, x86_64-w64-mingw32) of 
> 2018-05-30), I made the following small change on line 1187 (in the 
> function recentf-open-files-item):
>
> -           :format "%[%t\n%]"
> +           :format "%[%t%]\n"

(In the future, could you post complete patches instead of fragments?
That makes it easier to find the code in question.)

> This fixes the highlighting when hovering the mouse over a file in the 
> recent files dialog. Before, the file name, as well as the rest of the 
> line and the first character on the next line, was highlighted. Now only 
> the file name is highlighted, which looks much nicer and is probably 
> what is intended.

The commit message for that line seems to indicate that it's on purpose:

commit 5d24c60e3a3b07ccb31b886885ea117a058168be
Author: David Ponce <david@dponce.com>
Date:   Mon Apr 3 14:34:28 2006 +0000

    (recentf-open-files-item): Include newline in button
    field, so opening a file will work, when the point is at the end
    of the file name.  Allow, for example, to [i]search a file by
    extension and just push RET to open it.

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

bug#36550: Small bug fix in recentf.el

[Lars Ingebrigtsen]

(Please keep the debbugs address in the Cc list, otherwise the mail
won't reach the bug tracker.)

Linus Källberg <linus.kallberg@outlook.com> writes:

> Okay, makes sense. I just thought it looked a bit weird that the first 
> character on the next line is highlighted as well. At first I thought 
> something more serious was broken in Emacs, that's why I looked into it.

Oh, I didn't catch that the first character on the next line is
highlighted.  That does indeed seem like a bug.

> A middle ground would be to move the newline but put a single space 
> before "%]" (or set :button-suffix " "), to keep the behavior described 
> in the commit message. But it's no big deal in any event :-)

Could you propose a patch that avoids the highlight on the next line?

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

bug#36550: Small bug fix in recentf.el

[Linus Källberg]

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

Den 2019-07-09 kl. 15:04, skrev Lars Ingebrigtsen:
> Linus Källberg <linus.kallberg@outlook.com> writes:
> 
>> Okay, makes sense. I just thought it looked a bit weird that the first
>> character on the next line is highlighted as well. At first I thought
>> something more serious was broken in Emacs, that's why I looked into it.
> 
> Oh, I didn't catch that the first character on the next line is
> highlighted.  That does indeed seem like a bug.
> 
>> A middle ground would be to move the newline but put a single space
>> before "%]" (or set :button-suffix " "), to keep the behavior described
>> in the commit message. But it's no big deal in any event :-)
> 
> Could you propose a patch that avoids the highlight on the next line?

On second thought, I don't think the real problem is in recentf.el, but 
rather in the implementation of widgets and/or faces. It makes sense to 
keep recentf.el as it is.

I'm probably not qualified to fix the bug, at least in a timely manner, 
but I did some experimenting and came up with an MWE, which is attached 
to this e-mail. Hopefully it can be of some help.

Best regards,
Linus Källberg

[-- Attachment #2: widgets-mwe.el --]
[-- Type: text/plain, Size: 1568 bytes --]

(require 'widget)

(eval-when-compile
  (require 'wid-edit))

(defun widget-example ()
  "Create the widgets from the Widget manual."
  (interactive)
  (switch-to-buffer "*Widget Example*")
  (kill-all-local-variables)
  (let ((inhibit-read-only t))
    (erase-buffer))
  (remove-overlays)

  (widget-insert "Move the mouse over the links below to trigger the highlighting.\n\n")

  (widget-insert "This one looks normal:\n\n")
  (widget-create 'link "with brackets, w/o newline")

  (widget-insert "\n\nThis one looks okay, except that preferably, the whole first line\n"
                 "shouldn't be highlighted, only the text:\n\n")
  (widget-create 'link "with brackets, with newline\n")

  (widget-insert "\n\nThis one, which uses roughly the same parameters as are used in the\n"
                 "recentf dialog, looks really strange because the first character on the\n"
                 "next line is highlighted as well:\n\n")
  (widget-create 'link :button-suffix "" :button-prefix "" "w/o brackets, with newline\n")
  (widget-insert "A <- this character shouldn't be highlighted.")

  (widget-insert "\n\nThis one looks normal:\n\n")
  (widget-create 'link :button-suffix "" :button-prefix "" "w/o brackets, w/o newline")

  (widget-insert "\n\nThis one should look the same as the previous one, but it doesn't\n"
                 "because it is followed by EOF instead of a newline:\n\n")
  (widget-create 'link :button-suffix "" :button-prefix "" "w/o brackets, w/o newline")

  (widget-setup))

(widget-example)

bug#36550: Small bug fix in recentf.el

[Lars Ingebrigtsen]

Linus Källberg <linus.kallberg@outlook.com> writes:

> On second thought, I don't think the real problem is in recentf.el, but 
> rather in the implementation of widgets and/or faces. It makes sense to 
> keep recentf.el as it is.
>
> I'm probably not qualified to fix the bug, at least in a timely manner, 
> but I did some experimenting and came up with an MWE, which is attached 
> to this e-mail. Hopefully it can be of some help.

Yes, thanks for the examples -- widget.el is clearly placing the mouse
highlights wrong here; the most important being the "highlight the first
character on the next line", but the EOF thing was also pretty curious,
and is probably more an Emacs quirk than a widget.el bug...

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

bug#36550: mouse-face overlay calculation error

[Lars Ingebrigtsen]

Linus Källberg <linus.kallberg@outlook.com> writes:

> On second thought, I don't think the real problem is in recentf.el, but 
> rather in the implementation of widgets and/or faces. It makes sense to 
> keep recentf.el as it is.

There's something even more fundamentally wrong going on here, I think.

Here's a test case:

(progn
  (let ((point (point)))
    (insert "foo\n")
    (let ((o (make-overlay point (point))))
      (overlay-put o 'mouse-face 'highlight)
      (insert "bar"))))

This should make a mouse face that's displayed the entire "foo" line,
but it extends to the first character of the next line.

If you make it one character shorter, then the entire line isn't
highlighted.

And!  If you say `face' instead of `mouse-face', then everything is
highlighted correctly (i.e., just the entire "foo" line, and not the "b"
on the next line).

So is there some basic fault in the code that calculates the length of
the mouse highlighting?  I don't really know where to start looking...

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

bug#36550: mouse-face overlay calculation error

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sat, 13 Jul 2019 02:31:15 +0200
> Cc: "36550@debbugs.gnu.org" <36550@debbugs.gnu.org>
> 
> (progn
>   (let ((point (point)))
>     (insert "foo\n")
>     (let ((o (make-overlay point (point))))
>       (overlay-put o 'mouse-face 'highlight)
>       (insert "bar"))))
> 
> This should make a mouse face that's displayed the entire "foo" line,
> but it extends to the first character of the next line.
> 
> If you make it one character shorter, then the entire line isn't
> highlighted.
> 
> And!  If you say `face' instead of `mouse-face', then everything is
> highlighted correctly (i.e., just the entire "foo" line, and not the "b"
> on the next line).

Mouse-face isn't supposed to cover newlines, I think.  Why do you need
that?

The "one character shorter" variant does what it's expected to do,
because mouse-face is not extended to EOL as with other faces.
Mouse-face is for showing the parts of text that are mouse-sensitive,
so it makes no sense to highlight portions of display that have no
text.

> So is there some basic fault in the code that calculates the length of
> the mouse highlighting?  I don't really know where to start looking...

It's in the display code, and is quite complicated due to
bidirectional text use case.  See mouse_face_from_buffer_pos and its
subroutine rows_from_pos_range.

bug#36550: mouse-face overlay calculation error

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Mouse-face isn't supposed to cover newlines, I think.  Why do you need
> that?

Because Widget wants to have the mouse-face extend to the end of the
line, I think...

> The "one character shorter" variant does what it's expected to do,
> because mouse-face is not extended to EOL as with other faces.
> Mouse-face is for showing the parts of text that are mouse-sensitive,
> so it makes no sense to highlight portions of display that have no
> text.

OK, if this is how mouse-face is supposed to work, then the fix in
wid-edit.el should be pretty trivial -- I'll just have it not put the
overlay on the newline?

>> So is there some basic fault in the code that calculates the length of
>> the mouse highlighting?  I don't really know where to start looking...
>
> It's in the display code, and is quite complicated due to
> bidirectional text use case.  See mouse_face_from_buffer_pos and its
> subroutine rows_from_pos_range.

Oh, wow; that's a daunting function indeed...

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

bug#36550: mouse-face overlay calculation error

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 36550@debbugs.gnu.org,  linus.kallberg@outlook.com
> Date: Sat, 13 Jul 2019 15:10:05 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Mouse-face isn't supposed to cover newlines, I think.  Why do you need
> > that?
> 
> Because Widget wants to have the mouse-face extend to the end of the
> line, I think...

I don't understand why Widget would want to do that.  As I explained,
it makes no sense to highlight parts of display that have no text with
a face that shows mouse-sensitive text.  When the next line's
characters are also sensitive, then yes, we do highlight all the way
to the newline.

> > The "one character shorter" variant does what it's expected to do,
> > because mouse-face is not extended to EOL as with other faces.
> > Mouse-face is for showing the parts of text that are mouse-sensitive,
> > so it makes no sense to highlight portions of display that have no
> > text.
> 
> OK, if this is how mouse-face is supposed to work, then the fix in
> wid-edit.el should be pretty trivial -- I'll just have it not put the
> overlay on the newline?

Yes, I think so.

> > It's in the display code, and is quite complicated due to
> > bidirectional text use case.  See mouse_face_from_buffer_pos and its
> > subroutine rows_from_pos_range.
> 
> Oh, wow; that's a daunting function indeed...

Yes.  The problem it tries to solve is to highlight correctly when
buffer positions do not increment monotonously with screen
coordinates.  Unsurprisingly, at the time it took me some time to get
that code right.

bug#36550: mouse-face overlay calculation error

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't understand why Widget would want to do that.  As I explained,
> it makes no sense to highlight parts of display that have no text with
> a face that shows mouse-sensitive text.  When the next line's
> characters are also sensitive, then yes, we do highlight all the way
> to the newline.

This is the commit that introduced the problem:

commit 5d24c60e3a3b07ccb31b886885ea117a058168be
Author: David Ponce <david@dponce.com>
Date:   Mon Apr 3 14:34:28 2006 +0000

    (recentf-open-files-item): Include newline in button
    field, so opening a file will work, when the point is at the end
    of the file name.  Allow, for example, to [i]search a file by
    extension and just push RET to open it.

So it really wants the widget to have the newline inside the widget for
usability reasons.

I can special-case this in wid-edit.el -- if the final character in the
region is a newline, then all the other overlays extend all the way, but
mouse-face stops just short of the newline.  But it's more than a bit
hacky...

>> Oh, wow; that's a daunting function indeed...
>
> Yes.  The problem it tries to solve is to highlight correctly when
> buffer positions do not increment monotonously with screen
> coordinates.  Unsurprisingly, at the time it took me some time to get
> that code right.

I see.

Well, it's not 100% right, since it highlights the first character on
the next line if you have mouse-face on the preceding newline, which has
to be a bug even if we're not supposed to put mouse-face on the newline.
:-)

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

bug#36550: mouse-face overlay calculation error

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 36550@debbugs.gnu.org,  linus.kallberg@outlook.com
> Date: Sat, 13 Jul 2019 15:50:41 +0200
> 
> commit 5d24c60e3a3b07ccb31b886885ea117a058168be
> Author: David Ponce <david@dponce.com>
> Date:   Mon Apr 3 14:34:28 2006 +0000
> 
>     (recentf-open-files-item): Include newline in button
>     field, so opening a file will work, when the point is at the end
>     of the file name.  Allow, for example, to [i]search a file by
>     extension and just push RET to open it.
> 
> So it really wants the widget to have the newline inside the widget for
> usability reasons.

I still don't understand why.  Surely, "the end of the file name" is
before the newline, right?  And what point has to do with that, since
mouse-face is about the mouse pointer, not about point?  What am I
missing here?

> Well, it's not 100% right, since it highlights the first character on
> the next line if you have mouse-face on the preceding newline, which has
> to be a bug even if we're not supposed to put mouse-face on the newline.
> :-)

No, it's 100% right, because it implements a certain set of
requirements, and those mandate that when the end point is beyond the
last character on a line, that end point is in the next line.

bug#36550: mouse-face overlay calculation error

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: 36550@debbugs.gnu.org,  linus.kallberg@outlook.com
>> Date: Sat, 13 Jul 2019 15:50:41 +0200
>> 
>> commit 5d24c60e3a3b07ccb31b886885ea117a058168be
>> Author: David Ponce <david@dponce.com>
>> Date:   Mon Apr 3 14:34:28 2006 +0000
>> 
>>     (recentf-open-files-item): Include newline in button
>>     field, so opening a file will work, when the point is at the end
>>     of the file name.  Allow, for example, to [i]search a file by
>>     extension and just push RET to open it.
>> 
>> So it really wants the widget to have the newline inside the widget for
>> usability reasons.
>
> I still don't understand why.  Surely, "the end of the file name" is
> before the newline, right?

I am not sure; I don't use recentf...

> And what point has to do with that, since mouse-face is about the
> mouse pointer, not about point?  What am I missing here?

The widget consists of text in the buffer and a bunch of overlays
(including keymap overlays), and the mouse-face overlay is one of them.
My guess is that the committer wanted the keymap to be on the newline so
that it's in effect when typing?

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

bug#36550: mouse-face overlay calculation error

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 36550@debbugs.gnu.org,  linus.kallberg@outlook.com
> Date: Sat, 13 Jul 2019 16:25:35 +0200
> 
> > And what point has to do with that, since mouse-face is about the
> > mouse pointer, not about point?  What am I missing here?
> 
> The widget consists of text in the buffer and a bunch of overlays
> (including keymap overlays), and the mouse-face overlay is one of them.
> My guess is that the committer wanted the keymap to be on the newline so
> that it's in effect when typing?

OK, but then they simply should use a separate overlay for the
mouse-face, I think, one that doesn't cover the newline.  Does that
solve the problem?

bug#36550: mouse-face overlay calculation error

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> OK, but then they simply should use a separate overlay for the
> mouse-face, I think, one that doesn't cover the newline.  Does that
> solve the problem?

Yes, I can hack wid-edit.el as previously suggested.

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

bug#36550: mouse-face overlay calculation error

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> OK, but then they simply should use a separate overlay for the
>> mouse-face, I think, one that doesn't cover the newline.  Does that
>> solve the problem?
>
> Yes, I can hack wid-edit.el as previously suggested.

OK, I took another look at the wid-edit.el code which reminded me that
it's not that simple, because it does a lot of manipulation of the
overlays in the most tedious way.

So I'll just leave this for somebody else to fix one rainy day...

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

bug#36550: mouse-face overlay calculation error

[Linus Källberg]

Den 2019-07-13 kl. 17:08, skrev Lars Ingebrigtsen:
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>> OK, but then they simply should use a separate overlay for the
>>> mouse-face, I think, one that doesn't cover the newline.  Does that
>>> solve the problem?
>>
>> Yes, I can hack wid-edit.el as previously suggested.
> 
> OK, I took another look at the wid-edit.el code which reminded me that
> it's not that simple, because it does a lot of manipulation of the
> overlays in the most tedious way.
> 
> So I'll just leave this for somebody else to fix one rainy day...

I might have misunderstood the discussion, but just to clarify my 
opinion, if the overlay ends with a newline character, the mouse-face 
should *not* extend to the right edge of the window (and definitely not 
to the first character on the next line). I would expect it to end after 
the last character before the newline, or possibly one character further 
to the right (as if there was an imaginary space character inserted 
there). As was said earlier, it doesn't make sense to highlight parts of 
a line that doesn't contain clickable text.

In recentf, the newline after the file name is included in each link so 
that if point is positioned right after the last character -- which 
happens, e.g., if one i-searches for a file extension -- one can simply 
press enter to open the file (as said in the commit message). However, 
due to the "bug" in question (if it is indeed a bug), when hovering the 
mouse over a link, the whole line (up to the right edge of the window) 
and the first character on the next line is highlighted. I have been 
using Emacs, and the recentf package, for only five years, but I always 
thought that this looked ugly and somewhat "buggy". The other day I was 
a bit cranky and decided to look into it :-)

As I said earlier, one way to fix the issue in recentf is simply to move 
the newline outside of the link, but put a space character after each 
file name. This way, the mouse-over highlights look okay, and one can 
still i-search for a file extension and then simply press enter. Here is 
a patch that does this:

diff --git "a/lisp/recentf.el" "b/lisp/recentf.el"
index 4112b44e48..5d832a9d37 100644
--- "a/lisp/recentf.el"
+++ "b/lisp/recentf.el"
@@ -1179,9 +1179,9 @@ IGNORE other arguments."
      ;; Represent a single file with a link widget
      `(link :tag ,(car menu-element)
             :button-prefix ""
-           :button-suffix ""
+           :button-suffix " "
             :button-face default
-           :format "%[%t\n%]"
+           :format "%[%t%]\n"
             :help-echo ,(concat "Open " (cdr menu-element))
             :action recentf-open-files-action
             ;; Override the (problematic) follow-link property of the

Best regards,
Linus Källberg

bug#36550: mouse-face overlay calculation error

[Eli Zaretskii]

> From: Linus Källberg <linus.kallberg@outlook.com>
> CC: "36550@debbugs.gnu.org" <36550@debbugs.gnu.org>
> Date: Sat, 13 Jul 2019 15:38:05 +0000
> 
> I might have misunderstood the discussion, but just to clarify my 
> opinion, if the overlay ends with a newline character, the mouse-face 
> should *not* extend to the right edge of the window (and definitely not 
> to the first character on the next line).

But that's not what mouse-face means and does.  It highlights
mouse-sensitive text, and thus it should NOT include a newline,
because a newline does not have a glyph in the buffer text.  That's
why you see the 1st character on the next line highlighted: it's the
next character after the newline, and since the newline is absent, it
gets highlighted instead, because when character positions are not
monotonous with screen coordinates, we have no alternative but
highlight that next character.

> I would expect it to end after the last character before the newline

Can't do that, because the newline is also covered by the face.

> or possibly one character further to the right (as if there was an
> imaginary space character inserted there).

Can't do that, because that imaginary character doesn't exist, and
therefore we cannot possible access its buffer position.

> In recentf, the newline after the file name is included in each link so 
> that if point is positioned right after the last character -- which 
> happens, e.g., if one i-searches for a file extension -- one can simply 
> press enter to open the file (as said in the commit message).

But pressing Enter doesn't require mouse-face, does it?  It requires
an overlay with a suitable keymap property, right?

> However, due to the "bug" in question (if it is indeed a bug), when
> hovering the mouse over a link, the whole line (up to the right edge
> of the window) and the first character on the next line is
> highlighted. I have been using Emacs, and the recentf package, for
> only five years, but I always thought that this looked ugly and
> somewhat "buggy". The other day I was a bit cranky and decided to
> look into it :-)

Indeed, the mouse-face should end before the newline.

> As I said earlier, one way to fix the issue in recentf is simply to move 
> the newline outside of the link, but put a space character after each 
> file name. This way, the mouse-over highlights look okay, and one can 
> still i-search for a file extension and then simply press enter. Here is 
> a patch that does this:

If this fixes the issue, it's fine with me.  However, I think we
should have a comment there explaining why we add this space
character.

Thanks.

bug#36550: mouse-face overlay calculation error

[Linus Källberg]

Den 2019-07-13 kl. 17:49, skrev Eli Zaretskii:
>> From: Linus Källberg <linus.kallberg@outlook.com>
>> CC: "36550@debbugs.gnu.org" <36550@debbugs.gnu.org>
>> Date: Sat, 13 Jul 2019 15:38:05 +0000
>>
>> I might have misunderstood the discussion, but just to clarify my
>> opinion, if the overlay ends with a newline character, the mouse-face
>> should *not* extend to the right edge of the window (and definitely not
>> to the first character on the next line).
> 
> But that's not what mouse-face means and does.  It highlights
> mouse-sensitive text, and thus it should NOT include a newline,
> because a newline does not have a glyph in the buffer text.  That's
> why you see the 1st character on the next line highlighted: it's the
> next character after the newline, and since the newline is absent, it
> gets highlighted instead, because when character positions are not
> monotonous with screen coordinates, we have no alternative but
> highlight that next character.

Are you saying that the mouse-face property should not be used on 
overlays that contain newline characters, period, or simply those that 
have a newline character as their *last* character? I must say, it does 
seem like a bug that the appearance of characters not even included in 
the overlay (i.e., the first character on the next line) is changed when 
hovering the mouse over it.

I understand that a newline character cannot be clicked or highlighted 
if it has no glyph in the text, but why can't it then simply not be 
highlighted? Why must the first character on the next line be 
highlighted instead? No doubt it is difficult to change this behavior 
due to the complicated logic involved, but is it really intended, in the 
sense that something else would break if it was "fixed"?

>> or possibly one character further to the right (as if there was an
>> imaginary space character inserted there).
> 
> Can't do that, because that imaginary character doesn't exist, and
> therefore we cannot possible access its buffer position.

But doesn't it already do that? I use an Emacs theme that underlines 
highlighted text, and when an overlay contains a newline character 
(anywhere, not necessarily at the end), there is always one extra 
"imaginary" character underlined after the last character before the 
line break.

In this example, there is one extra underlined character after "foo":

(progn
   (load-theme 'wombat t)
   (let ((point (point)))
     (insert "foo\nbar")
     (let ((o (make-overlay point (point))))
       (overlay-put o 'mouse-face 'highlight))))

>> In recentf, the newline after the file name is included in each link so
>> that if point is positioned right after the last character -- which
>> happens, e.g., if one i-searches for a file extension -- one can simply
>> press enter to open the file (as said in the commit message).
> 
> But pressing Enter doesn't require mouse-face, does it?  It requires
> an overlay with a suitable keymap property, right?

Exactly, I guess the button widget simply uses the same overlay for 
everything.

>> As I said earlier, one way to fix the issue in recentf is simply to move
>> the newline outside of the link, but put a space character after each
>> file name. This way, the mouse-over highlights look okay, and one can
>> still i-search for a file extension and then simply press enter. Here is
>> a patch that does this:
> 
> If this fixes the issue, it's fine with me.  However, I think we
> should have a comment there explaining why we add this space
> character.

Yes, a comment should be added. However, I would prefer fixing the real 
problem, which, if not in the display code, might be in the 
implementation of the button widget.

Best regards,
Linus Källberg

bug#36550: mouse-face overlay calculation error

[Eli Zaretskii]

> From: Linus Källberg <linus.kallberg@outlook.com>
> CC: "larsi@gnus.org" <larsi@gnus.org>, "36550@debbugs.gnu.org"
> 	<36550@debbugs.gnu.org>
> Date: Sat, 13 Jul 2019 19:49:07 +0000
> 
> > But that's not what mouse-face means and does.  It highlights
> > mouse-sensitive text, and thus it should NOT include a newline,
> > because a newline does not have a glyph in the buffer text.  That's
> > why you see the 1st character on the next line highlighted: it's the
> > next character after the newline, and since the newline is absent, it
> > gets highlighted instead, because when character positions are not
> > monotonous with screen coordinates, we have no alternative but
> > highlight that next character.
> 
> Are you saying that the mouse-face property should not be used on 
> overlays that contain newline characters, period, or simply those that 
> have a newline character as their *last* character?

The latter.

> I understand that a newline character cannot be clicked or highlighted 
> if it has no glyph in the text, but why can't it then simply not be 
> highlighted?

Because the algorithm to find what is the last glyph to be highlighted
is very complicated (since it cannot rely on buffer positions changing
monotonously with screen coordinates) and in this case it yields a
result that looks wrong.  If someone wants to find how to change the
algorithm so that it also covers this niche use case, feel free.

> is it really intended, in the sense that something else would break
> if it was "fixed"?

It depends on the fix.  I cannot tell until I see a proposal for such
a fix.  The existing code relies on some properties of glyphs as they
are laid out for display, and one of those properties is that the
screen line's end position is the position of the first character on
the next line.  That's why you see what you see in this case.

> >> or possibly one character further to the right (as if there was an
> >> imaginary space character inserted there).
> > 
> > Can't do that, because that imaginary character doesn't exist, and
> > therefore we cannot possible access its buffer position.
> 
> But doesn't it already do that? I use an Emacs theme that underlines 
> highlighted text, and when an overlay contains a newline character 
> (anywhere, not necessarily at the end), there is always one extra 
> "imaginary" character underlined after the last character before the 
> line break.

This is something specific to mouse-face, because unlike other faces,
it works with characters on display, not in the buffer.

> In this example, there is one extra underlined character after "foo":
> 
> (progn
>    (load-theme 'wombat t)
>    (let ((point (point)))
>      (insert "foo\nbar")
>      (let ((o (make-overlay point (point))))
>        (overlay-put o 'mouse-face 'highlight))))

Yes, as expected.

> >> In recentf, the newline after the file name is included in each link so
> >> that if point is positioned right after the last character -- which
> >> happens, e.g., if one i-searches for a file extension -- one can simply
> >> press enter to open the file (as said in the commit message).
> > 
> > But pressing Enter doesn't require mouse-face, does it?  It requires
> > an overlay with a suitable keymap property, right?
> 
> Exactly, I guess the button widget simply uses the same overlay for 
> everything.

Yes, I suggested to use a separate overlay for that.

> > If this fixes the issue, it's fine with me.  However, I think we
> > should have a comment there explaining why we add this space
> > character.
> 
> Yes, a comment should be added. However, I would prefer fixing the real 
> problem, which, if not in the display code, might be in the 
> implementation of the button widget.

Fine with me, if someone wants to work on that.