[PATCH] Fix two bugs in removing bookmark fringe marks (bug#70019)

[PATCH] Fix two bugs in removing bookmark fringe marks (bug#70019)

[Karl Fogel]

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

I'd appreciate some review on this patch.  I've no prior 
experience with fringe mark code (someone else added fringe mark 
support to bookmark.el).

This should fix bug#70019 and a separate-but-sort-of-related bug.

I'll wait at least a few days for comments/review before 
committing.

Best regards,
-Karl


[-- Attachment #2: Fix two bugs in removing bookmark fringe marks (bug#70019) --]
[-- Type: text/plain, Size: 2768 bytes --]

From a7c9618efdd423134990c22d62513eac50ab98e3 Mon Sep 17 00:00:00 2001
From: Karl Fogel <kfogel@red-bean.com>
Date: Thu, 18 Apr 2024 23:49:29 -0500
Subject: [PATCH] Fix two bugs in removing bookmark fringe marks

This fixes bug#70019 and a separate fringe-mark removal bug that
affected bookmarks in Info "dir" nodes.

* lisp/bookmark.el (bookmark--remove-fringe-mark): Fix bug#70019 by
temporarily widening in order to ensure we fetch the right overlays.
Also, don't tamper with the filename as received from the bookmark
object, since we will compare the received filename against one
generated dynamically by the same means (`bookmark-buffer-file-name')
to determine whether or not we're in the right buffer.
---
 lisp/bookmark.el | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index bf2357207d8..5d01fe24991 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -515,15 +515,30 @@ bookmark--remove-fringe-mark
         (non-essential t)
         overlays found temp)
     (when (and pos filename)
-      (setq filename (abbreviate-file-name (expand-file-name filename)))
       (dolist (buf (buffer-list))
         (with-current-buffer buf
           (when (equal filename
                        (ignore-errors (bookmark-buffer-file-name)))
             (setq overlays
                   (save-excursion
-                    (goto-char pos)
-                    (overlays-in (pos-bol) (1+ (pos-bol)))))
+                    (save-restriction
+                      ;; Suppose bookmark "foo" was earlier set at
+                      ;; location X in a file, but now the file is
+                      ;; narrowed such that X is outside the restriction.
+                      ;; Then the `goto-char' below would go to the
+                      ;; wrong place and thus the wrong overlays would
+                      ;; be fetched.  This is why we temporarily `widen'
+                      ;; before fetching.
+                      ;;
+                      ;; (This circumstance can easily arise when a
+                      ;; bookmark was set on Info node X but now the
+                      ;; "*info*" buffer is showing some other node Y,
+                      ;; with X and Y physically located in the same
+                      ;; file, as is often the case with Info nodes.
+                      ;; See bug #70019, for example.)
+                      (widen)
+                      (goto-char pos)
+                      (overlays-in (pos-bol) (1+ (pos-bol))))))
             (while (and (not found) (setq temp (pop overlays)))
               (when (eq 'bookmark (overlay-get temp 'category))
                 (delete-overlay (setq found temp))))))))))
-- 
2.43.0

Re: [PATCH] Fix two bugs in removing bookmark fringe marks (bug#70019)

[Dani Moncayo]

> This should fix bug#70019 and a separate-but-sort-of-related bug.
>
> I'll wait at least a few days for comments/review before
> committing.

I've applied the patch to my source tree and re-made my out-of-tree build.
But I still see the same problems reported in this bug ticket.
Maybe I've made some mistake... ¿Don't you see the problems with the
patch applied?

-- 
Dani Moncayo

Re: [PATCH] Fix two bugs in removing bookmark fringe marks (bug#70019)

[Karl Fogel]

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

On 19 Apr 2024, Dani Moncayo wrote:
>I've applied the patch to my source tree and re-made my 
>out-of-tree build.
>But I still see the same problems reported in this bug ticket.
>Maybe I've made some mistake... ¿Don't you see the problems with 
>the
>patch applied?

Nope, you didn't make any mistake.  I see the problem; please try 
this revised version, and thank you for testing.

Best regards,
-Karl


[-- Attachment #2: 0001-Fix-two-bugs-in-removing-bookmark-fringe-marks.patch --]
[-- Type: text/plain, Size: 3987 bytes --]

From 705eeb968c85cf8d865c893778cc0df6a6db6034 Mon Sep 17 00:00:00 2001
From: Karl Fogel <kfogel@red-bean.com>
Date: Thu, 18 Apr 2024 23:49:29 -0500
Subject: [PATCH] Fix two bugs in removing bookmark fringe marks

This fixes bug#70019 and a separate fringe-mark removal bug that
also affected bookmarks in certain Info nodes.

* lisp/bookmark.el (bookmark--remove-fringe-mark): Fix bug#70019 by
temporarily widening in order to ensure we fetch the right overlays.
Also, normalize both filenames before comparing, to avoid spurious
failure to match.
---
 lisp/bookmark.el | 47 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index bf2357207d8..06f8e24b518 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -515,18 +515,45 @@ bookmark--remove-fringe-mark
         (non-essential t)
         overlays found temp)
     (when (and pos filename)
-      (setq filename (abbreviate-file-name (expand-file-name filename)))
       (dolist (buf (buffer-list))
         (with-current-buffer buf
-          (when (equal filename
-                       (ignore-errors (bookmark-buffer-file-name)))
-            (setq overlays
-                  (save-excursion
-                    (goto-char pos)
-                    (overlays-in (pos-bol) (1+ (pos-bol)))))
-            (while (and (not found) (setq temp (pop overlays)))
-              (when (eq 'bookmark (overlay-get temp 'category))
-                (delete-overlay (setq found temp))))))))))
+          (let ((bkmk-fname (ignore-errors (bookmark-buffer-file-name))))
+            (when bkmk-fname
+              ;; Normalize both filenames before comparing, because the
+              ;; filename we receive from the bookmark wasn't
+              ;; necessarily generated by `bookmark-buffer-file-name'.
+              ;; For example, bookmarks set in Info nodes get a filename
+              ;; based on `Info-current-file', and under certain
+              ;; circumstances that can be an unexpanded path (e.g.,
+              ;; when the Info page was under your home directory).
+              (let ((this-fname-normalized (expand-file-name filename))
+                    (bkmk-fname-normalized (expand-file-name bkmk-fname)))
+                (when (equal this-fname-normalized bkmk-fname-normalized)
+                  (setq overlays
+                        (save-excursion
+                          (save-restriction
+                            ;; Suppose bookmark "foo" was earlier set at
+                            ;; location X in a file, but now the file is
+                            ;; narrowed such that X is outside the
+                            ;; restriction.  Then the `goto-char' below
+                            ;; would go to the wrong place and thus the
+                            ;; wrong overlays would be fetched.  This is
+                            ;; why we temporarily `widen' before
+                            ;; fetching.
+                            ;;
+                            ;; (This circumstance can easily arise when
+                            ;; a bookmark was set on Info node X but now
+                            ;; the "*info*" buffer is showing some other
+                            ;; node Y, with X and Y physically located
+                            ;; in the same file, as is often the case
+                            ;; with Info nodes.  See bug #70019, for
+                            ;; example.)
+                            (widen)
+                            (goto-char pos)
+                            (overlays-in (pos-bol) (1+ (pos-bol))))))
+                  (while (and (not found) (setq temp (pop overlays)))
+                    (when (eq 'bookmark (overlay-get temp 'category))
+                      (delete-overlay (setq found temp)))))))))))))
 
 (defun bookmark-maybe-sort-alist ()
   "Return `bookmark-alist' for display.
-- 
2.43.0

Re: [PATCH] Fix two bugs in removing bookmark fringe marks (bug#70019)

[Dani Moncayo]

On Sat, Apr 20, 2024 at 2:54 AM Karl Fogel <kfogel@red-bean.com> wrote:
> Nope, you didn't make any mistake.  I see the problem; please try
> this revised version, and thank you for testing.

OK.  With this second patch, I don't see the first problem [1]
anymore, but I keep seeing the second one [2].

-- 
Dani Moncayo

[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=70019#5
[2] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=70019#8

Re: [PATCH] Fix two bugs in removing bookmark fringe marks (bug#70019)

[Karl Fogel]

On 20 Apr 2024, Dani Moncayo wrote:
>On Sat, Apr 20, 2024 at 2:54 AM Karl Fogel <kfogel@red-bean.com> 
>wrote:
>> Nope, you didn't make any mistake.  I see the problem; please
>> try this revised version, and thank you for testing.
>
>OK.  With this second patch, I don't see the first problem [1]
>anymore, but I keep seeing the second one [2].

Ah, I'm sorry, I meant to discuss that second problem here.  For 
reference, here's your description of it:

> 1. Open some *info* manual and set a named bookmark there.
> E.g. [C-h r C-x r m f o o RET].
> 2. Kill the *info* buffer and open it again: [C-x k RET C-h r].
> 
> When I do that, I don't see the bookmark icon in the fringe 
> (wrong).
>
> But I _do_ see the icon if I _jump_ to the bookmark: [C-x r b f 
> o o RET]

This is the expected behavior, I think.

Let me explain why I believe that.  If you or someone can point 
out why this reasoning is wrong, I'd appreciate it.

In general, there's no mechanism for a bookmark fringe mark to be 
shown when one goes to a bookmarked location by some means *other* 
than through a bookmark function.

For example,

1. Find a file "SomeFile" in your home directory.

2. Create new bookmark "foo" on the first line.

   (Now you see a fringe mark.)

3. Kill the buffer entirely. 

4. Use [C-x C-f] to find "SomeFile" into a buffer again.

   (Now you don't see a fringe mark.)

5. Kill the buffer again.

6. Use [C-x r b] (`bookmark-jump') to visit bookmark "foo"

   (Now you see the fringe mark again.)

The reason the fringe mark appears in Step 6 is because the 
bookmark code had a chance to get involved (i.e., 
`bookmark--set-fringe-mark' got called somewhere along the way 
from `bookmark-jump').

In general, if you set a bookmark in a buffer, and you merely 
*leave* the buffer but don't kill the buffer (e.g., you do 
`bury-buffer' or something), then that fringe mark will still be 
there when you come back.  The fringe mark will also be placed if 
you get to the location via `bookmark-jump', and if you set a 
bookmark there with `bookmark-set'.

Also, if you retarget bookmark "foo" to point to a new location, 
then if there's some buffer currently displaying the old target of 
"foo", that old buffer's corresponding fringe mark for "foo" will 
get removed (bug #1 that I just fixed was about a special case of 
this behavior).

Now let's look at how this all works with Info nodes:

Often, two Info nodes are actually in the same file.  This was the 
case with the two that you used in your reproduction recipe: the 
"Distrib" node and the "Intro" node in the Emacs manual -- both 
are in emacs.info.gz, which is located at 
/usr/local/share/info/emacs.info.gz for me (but might be somewhere 
else for you).

When two Info nodes X and Y are in the same underlying file, then 
if you put a bookmark in node X, the fringe mark will stay there 
even if you go visit node Y and then come back to X (assuming you 
didn't do anything to kill the "*info*" buffer along the way). 
This is because the buffer has never been killed and the 
underlying file that it's visiting has not actually changed.

But imagine that your two nodes X and Y were in two different Info 
files.  If you set a bookmark in X and then go visit Y, and then 
come back to X (but not by using `bookmark-jump'), the fringe mark 
will be gone from from X.

So in your reproduction recipe quoted above, when you kill the 
"*info*" buffer, you're killing the buffer that's visiting that 
particular Info file.  When you visit that same Info node again 
via a non-bookmark route, there's no code that magically knows 
that there is a bookmark located here, and so no fringe mark is 
displayed.  On the other hand, when you use `bookmark-jump' to 
jump to the bookmark, then of course the bookmark code has a 
chance to put the fringe mark back.

By the way, I am not claiming that this behavior is ideal.  It 
would be wonderful if all of Emacs *did* magically have a way to 
know when a line associated with some bookmark is being displayed, 
so we could show a fringe mark there.

But I can't think of any reasonable way to implement that.  By 
"reasonable", I mean worth the complexity involved (I'm also not 
sure how to do it without an unacceptable performance penalty, but 
I haven't thought hard about it, because I'm pretty sure that if 
one *were* able to do it without a big performance hit, then the 
solution would be far more complex than a fringe mark is worth).

Best regards,
-Karl

Re: [PATCH] Fix two bugs in removing bookmark fringe marks (bug#70019)

[Dani Moncayo]

On Mon, Apr 22, 2024 at 5:51 AM Karl Fogel <kfogel@red-bean.com> wrote:
> [...]
>
> In general, there's no mechanism for a bookmark fringe mark to be
> shown when one goes to a bookmarked location by some means *other*
> than through a bookmark function.
>
> [...]

Hi Karl.

OK, Thanks for your work on this, and for your explanations, which I understand.

I intuitively expected the fringe icon to be shown at its current
location, whenever that location is visible, regardless of other
considerations.

We both agree that it would be a nice feature, but not at any price.

I'm OK with your fix, then.  Thanks.

-- 
Dani Moncayo

Re: bug#70019: [PATCH] Fix two bugs in removing bookmark fringe marks (bug#70019)

[Eli Zaretskii]

> Cc: 70019@debbugs.gnu.org, Emacs Developers <emacs-devel@gnu.org>
> From: Dani Moncayo <dmoncayo@gmail.com>
> Date: Mon, 22 Apr 2024 08:26:41 +0200
> 
> On Mon, Apr 22, 2024 at 5:51 AM Karl Fogel <kfogel@red-bean.com> wrote:
> > [...]
> >
> > In general, there's no mechanism for a bookmark fringe mark to be
> > shown when one goes to a bookmarked location by some means *other*
> > than through a bookmark function.
> >
> > [...]
> 
> Hi Karl.
> 
> OK, Thanks for your work on this, and for your explanations, which I understand.
> 
> I intuitively expected the fringe icon to be shown at its current
> location, whenever that location is visible, regardless of other
> considerations.
> 
> We both agree that it would be a nice feature, but not at any price.
> 
> I'm OK with your fix, then.  Thanks.

Thanks.  Karl, please install the patch on the master branch, and
close the bug when you do.

Re: bug#70019: [PATCH] Fix two bugs in removing bookmark fringe marks (bug#70019)

[Karl Fogel]

On 22 Apr 2024, Eli Zaretskii wrote:
>Thanks.  Karl, please install the patch on the master branch, and
>close the bug when you do.

Installed in commit 63765a74f15e, and this email should close the 
report.

Best regards,
-Karl