bug#75354: (29.4; eww buffer is not displayed correctly when used from bookmark-jump )

[Thierry Volpiatto <thievol@posteo.net>]

[-- Attachment #1.1: Type: text/plain, Size: 1686 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Thierry Volpiatto <thievol@posteo.net>
>> Date: Wed, 08 Jan 2025 07:40:30 +0000
>> 
>> I could fix the problem by modifying bookmark--jump-via:
>
> Thanks.
>
> This is contrary to what you originally wrote (with which I agree):

Yes, after deeper search I found that `bookmark--jump-via` is behaving
like this AFAIU:
 - It calls the handler which creates a new buffer related to bookmark.
 - It then displays the current-buffer (the one before jumping to bmk) in
 a window according to DISPLAY-FUNCTION and make the bookmark buffer current.

This approach is OK as long as the handler fn doesn't try do do one part
of the job (window handling) itself, which is not the case at least with
eww and w3m.  It is as well counter intuitive, DISPLAY-FUNCTION should
be called on the buffer generated by bookmark and not the contrary, so
this change makes the code simpler and easier to understand.

> By contrast, the change you propose now will affect all the uses of
> bookmarks, everywhere,

Yes, this is intended, in addition of fixing eww, it fixes w3m and also
the quit function of eww (actually broken).

> which is riskier, given how many different variants of bookmark usage
> are out there.

Tested here on many different kinds of bookmarks and work as expected
unlike the current code.

> Why did you change your mind about the preferred way of fixing this?

Because I couldn't find a way to fix this correctly by modifying only
the handler, thus the change would need to be done on each other
handlers which behave unexpectedly.

Here is the latest patch attached.

Thanks.

-- 
Thierry

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Handle-correctly-DISPLAY-FUNCTION-arg-in-bookmark-ju.patch --]
[-- Type: text/x-diff, Size: 3715 bytes --]

From b4d99152212744ab1d90c67641cd41576e6e13f3 Mon Sep 17 00:00:00 2001
From: Thierry Volpiatto <thievol@posteo.net>
Date: Wed, 8 Jan 2025 10:45:55 +0100
Subject: [PATCH] Handle correctly DISPLAY-FUNCTION arg in bookmark--jump-via

This fix bug #75354 where when jumping to a eww bmk to other window (or
frame) the buffer is displayed both in other window and current window.

Previously bookmark--jump-via was calling DISPLAY-FUNCTION on the
current-buffer from the new buffer created by bookmark handler
(e.g. eww) now DISPLAY-FUNCTION is called on the new buffer from the
current buffer (winconf is saved when calling handler).

In addition to make eww bookmarks displayed properly, this fix as well
w3m bookmarks and as a side effect fix the eww quit function which
restore properly winconf after quitting (previously leaving two windows
open).
---
 lisp/bookmark.el | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 0048330e790..248620fbda4 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -1256,33 +1256,34 @@ Useful for example to unhide text in `outline-mode'.")
 
 (defun bookmark--jump-via (bookmark-name-or-record display-function)
   "Handle BOOKMARK-NAME-OR-RECORD, then call DISPLAY-FUNCTION.
-DISPLAY-FUNCTION is called with the current buffer as argument.
+DISPLAY-FUNCTION is called with the new buffer as argument.
 
 After calling DISPLAY-FUNCTION, set window point to the point specified
 by BOOKMARK-NAME-OR-RECORD, if necessary, run `bookmark-after-jump-hook',
 and then show any annotations for this bookmark."
-  (bookmark-handle-bookmark bookmark-name-or-record)
-  ;; Store `point' now, because `display-function' might change it.
-  (let ((point (point)))
-    (save-current-buffer
-      (funcall display-function (current-buffer)))
-    (let ((win (get-buffer-window (current-buffer) 0)))
-      (if win (set-window-point win point))))
-  ;; FIXME: we used to only run bookmark-after-jump-hook in
-  ;; `bookmark-jump' itself, but in none of the other commands.
-  (when bookmark-fringe-mark
-    (let ((overlays (overlays-in (pos-bol) (1+ (pos-bol))))
-          temp found)
-      (while (and (not found) (setq temp (pop overlays)))
-        (when (eq 'bookmark (overlay-get temp 'category))
-          (setq found t)))
-      (unless found
-        (bookmark--set-fringe-mark))))
-  (run-hooks 'bookmark-after-jump-hook)
-  (if bookmark-automatically-show-annotations
+  (let (buf)
+    (save-window-excursion
+      (bookmark-handle-bookmark bookmark-name-or-record)
+      (setq buf (current-buffer)))
+    (let ((point (with-current-buffer buf (point))))
+      (funcall display-function buf)
+      (when-let ((win (get-buffer-window buf 0)))
+        (set-window-point win point)))
+    ;; FIXME: we used to only run bookmark-after-jump-hook in
+    ;; `bookmark-jump' itself, but in none of the other commands.
+    (when bookmark-fringe-mark
+      (let ((overlays (overlays-in (pos-bol) (1+ (pos-bol))))
+            temp found)
+        (while (and (not found) (setq temp (pop overlays)))
+          (when (eq 'bookmark (overlay-get temp 'category))
+            (setq found t)))
+        (unless found
+          (bookmark--set-fringe-mark))))
+    (run-hooks 'bookmark-after-jump-hook)
+    (when bookmark-automatically-show-annotations
       ;; if there is an annotation for this bookmark,
       ;; show it in a buffer.
-      (bookmark-show-annotation bookmark-name-or-record)))
+      (bookmark-show-annotation bookmark-name-or-record))))
 
 
 ;;;###autoload
-- 
2.34.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 686 bytes --]