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

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

[Thierry Volpiatto]

The eww buffer is displayed in two windows whereas it should be
displayed only in other-window when used like this:

--8<---------------cut here---------------start------------->8---
(require 'bookmark)
(require 'eww)
(let ((bookmark-alist '(("La langue française"
			 (location . "https://www.lalanguefrancaise.com/")
			 (handler . eww-bookmark-jump)))))
  (bookmark-jump (car bookmark-alist) #'switch-to-buffer-other-window))
--8<---------------cut here---------------end--------------->8---

It is not a bug in `bookmark-jump` but in `eww-bookmark-jump` (the eww
bookmark handler) which uses wrongly the function `eww` which itself uses
`pop-to-buffer-same-window`.  Probably a lower level function should be
used (if one?).



In GNU Emacs 29.4 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw3d scroll bars) of 2024-12-26 built on IPad-S340
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Linux Mint 21.3

Configured using:
 'configure CFLAGS=-O8 --bindir=/usr/local/sbin/emacs-29.4 --with-cairo
 --with-x-toolkit=lucid --with-modules --without-tree-sitter
 --without-native-compilation'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS X11 XAW3D XDBE XIM XINPUT2 XPM LUCID ZLIB

Important settings:
  value of $LANG: fr_FR.UTF-8
  locale-coding-system: utf-8-unix

Major mode: 

Minor modes in effect:
  emms-mode-line-mode: t
  emms-playing-time-display-mode: t
  emms-playing-time-mode: t
  server-mode: t
  psession-mode: t
  psession-savehist-mode: t
  register-preview-mode: t
  global-git-gutter-mode: t
  display-time-mode: t
  winner-mode: t
  tv-save-place-mode: t
  helm-epa-mode: t
  helm-descbinds-mode: t
  helm-top-poll-mode: t
  helm-adaptive-mode: t
  helm-mode: t
  helm-minibuffer-history-mode: t
  helm-ff-icon-mode: t
  shell-dirtrack-mode: t
  helm-popup-tip-mode: t
  dired-async-mode: t
  minibuffer-depth-indicate-mode: t
  gcmh-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow emacsbug url-http url-gw url-cache url-auth gnus-async
gnus-bcklg gnus-ml disp-table nndraft nnmh nnfolder gnus-agent gnus-srvr
gnus-score score-mode nnvirtual nntp gnus-cache helm-emms helm-ring
tramp-sh cl-print helm-ls-hg vc-hg isl vc-rcs log-view pcvs-util cc-mode
cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars
cc-defs view smerge-mode diff helm-dabbrev cl-indent char-fold shortdoc
emacs-news-mode epa-file network-stream nsm mailalias qp epa-mail
face-remap gnus-cite smiley w3m-form w3m-symbol config-w3m w3m timezone
w3m-hist bookmark-w3m w3m-ems w3m-favicon w3m-image w3m-fb tab-line
w3m-proc w3m-util mm-archive mail-extr textsec uni-scripts idna-mapping
ucs-normalize uni-confusable textsec-check addressbook-bookmark
tv-mu4e-config advice gnus-and-mu4e mu4e-patch mu4e-contrib eshell
esh-cmd esh-ext esh-opt esh-proc esh-io esh-arg esh-module esh-groups
esh-util mu4e mu4e-org mu4e-notification notifications mu4e-main
smtpmail mu4e-view mu4e-mime-parts crm mu4e-headers mu4e-thread
mu4e-actions mu4e-compose mu4e-draft gnus-msg mu4e-search mu4e-lists
mu4e-bookmarks mu4e-mark mu4e-message flow-fill hl-line mu4e-contacts
mu4e-update mu4e-folders mu4e-context mu4e-query-items mu4e-server
mu4e-modeline mu4e-vars mu4e-helpers mu4e-config mu4e-window ido
mu4e-obsolete helm-skitour helm-command helm-elisp helm-eval edebug
debug backtrace helm-x-files helm-for-files helm-bookmark helm-info
bookmark image-file image-converter emms-config emms-idapi-browser
emms-idapi emms-idapi-musicbrainz emms-mpris emms-librefm-stream
emms-librefm-scrobbler emms-playlist-limit emms-i18n emms-history
emms-score emms-stream-info emms-metaplaylist-mode emms-bookmarks
emms-cue emms-mode-line-icon emms-browser sort emms-volume
emms-volume-sndioctl emms-volume-mixerctl emms-volume-pulse
emms-volume-amixer emms-playlist-sort emms-last-played emms-player-xine
emms-player-mpd tq emms-lyrics emms-url emms-streams emms-show-all
emms-tag-editor emms-tag-tracktag emms-mark emms-mode-line emms-cache
emms-info-native emms-info-native-spc emms-info-native-mp3
emms-info-native-ogg emms-info-native-opus emms-info-native-flac
emms-info-native-vorbis bindat emms-info-exiftool emms-info-tinytag
emms-info-metaflac emms-info-opusinfo emms-info-ogginfo
emms-info-mp3info emms-playlist-mode emms-player-vlc emms-player-mpv
emms-playing-time emms-info emms-later-do emms-player-mplayer
emms-player-simple emms-source-playlist emms-source-file locate
emms-setup emms emms-compat emms-auto helm-external helm-net
tramp-archive tramp-gvfs tramp-cache time-stamp zeroconf ffap
helm-ls-git vc-git diff-mode vc vc-dispatcher flymake-shellcheck
flymake-proc flymake project warnings sh-script smie treesit executable
make-mode org-indent org-element org-persist org-id org-refile avl-tree
generator oc-basic ol-eww eww url-queue thingatpt mm-url ol-rmail ol-mhe
ol-irc ol-info ol-gnus nnselect gnus-art mm-uu mml2015 mm-view mml-smime
smime gnutls dig gnus-sum shr pixel-fill kinsoku url-file svg dom
gnus-group gnus-undo gnus-start gnus-dbus dbus xml gnus-cloud nnimap
nnmail mail-source utf7 nnoo gnus-spec gnus-int gnus-range message
sendmail yank-media puny rfc822 mml mml-sec mm-decode mm-bodies
mm-encode mail-parse rfc2231 rfc2047 rfc2045 ietf-drums mailabbrev
gmm-utils mailheader gnus-win gnus nnheader gnus-util mail-utils range
mm-util mail-prsvr ol-docview doc-view jka-compr ol-bibtex bibtex
ol-bbdb ol-w3m ol-doi org-link-doi org-config ob-gnuplot org-crypt
org-protocol org ob ob-tangle ob-ref ob-lob ob-table ob-exp org-macro
org-src ob-comint org-pcomplete org-list org-footnote org-faces
org-entities noutline outline ob-emacs-lisp ob-core ob-eval org-cycle
org-table ol org-fold org-fold-core org-keys oc org-loaddefs find-func
org-version org-compat org-macs bug-reference cus-start naquadah-theme
solar cal-dst holidays holiday-loaddefs appt diary-lib diary-loaddefs
cal-menu calendar cal-loaddefs server bm cl-extra imenu tv-utils
psession frameset register-preview pcase git-gutter mule-util
dired-extension time winner describe-variable help-fns radix-tree
help-mode tv-save-place.el init-helm epa derived epg rfc6068 epg-config
helm-epa helm-descbinds cus-edit pp icons wid-edit helm-sys
helm-adaptive helm-mode helm-misc helm-files image-dired
image-dired-tags image-dired-external image-dired-util xdg image-mode
exif filenotify tramp tramp-loaddefs trampver tramp-integration files-x
tramp-compat rx shell pcomplete parse-time iso8601 time-date
helm-buffers all-the-icons all-the-icons-faces data-material
data-weathericons data-octicons data-fileicons data-faicons
data-alltheicons helm-occur helm-tags helm-locate helm-grep wgrep-helm
wgrep grep compile text-property-search comint ansi-osc ring helm-regexp
format-spec ansi-color helm-utils helm-help helm-types
helm-extensions-autoloads helm-autoloads helm helm-global-bindings
helm-easymenu edmacro kmacro helm-core helm-source helm-multi-match
helm-lib dired-async async dired-aux dired dired-loaddefs isl-autoloads
mb-depth avoid cus-load gcmh easy-mmode all-the-icons-autoloads
bash-completion-autoloads info ledger-mode-autoloads
markdown-mode-autoloads w3m-load w3m-autoloads package browse-url url
url-proxy url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util mailcap url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs password-cache json subr-x
map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs cl-lib rmc
iso-transl tooltip cconv eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo x-toolkit xinput2 x multi-tty
make-network-process emacs)

Memory information:
((conses 16 1612641 255300)
 (symbols 48 43903 5)
 (strings 32 321582 36530)
 (string-bytes 1 9242506)
 (vectors 16 125121)
 (vector-slots 8 2615479 161054)
 (floats 8 5249 2732)
 (intervals 56 129833 3766)
 (buffers 976 196))
<#secure method=pgpmime mode=sign>

-- 
Thierry

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

[Thierry Volpiatto]

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


I could fix the problem by modifying bookmark--jump-via:

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 0048330e790..a474bed652e 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -1261,28 +1261,29 @@ DISPLAY-FUNCTION is called with the current 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

-- 
Thierry

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

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

[Eli Zaretskii]

> 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):

> It is not a bug in `bookmark-jump` but in `eww-bookmark-jump` (the eww
> bookmark handler) which uses wrongly the function `eww` which itself uses
> `pop-to-buffer-same-window`.  Probably a lower level function should be
> used (if one?).

By contrast, the change you propose now will affect all the uses of
bookmarks, everywhere, which is riskier, given how many different
variants of bookmark usage are out there.

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

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

[Thierry Volpiatto]

[-- 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 --]

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

[Eli Zaretskii]

> From: Thierry Volpiatto <thievol@posteo.net>
> Cc: Thierry Volpiatto <thievol@posteo.net>,  75354@debbugs.gnu.org
> Date: Wed, 08 Jan 2025 13:52:32 +0000
> 
> > 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.

OK, thanks.  Let's leave this for a few days to give people time to
post comments if they have them.

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

[Thierry Volpiatto]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Thierry Volpiatto <thievol@posteo.net>
>> Cc: Thierry Volpiatto <thievol@posteo.net>,  75354@debbugs.gnu.org
>> Date: Wed, 08 Jan 2025 13:52:32 +0000
>> 
>> > 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.
>
> OK, thanks.  Let's leave this for a few days to give people time to
> post comments if they have them.

Ok, I will make changes to commit message (needs * lisp/bookmark.el (...):
bla) and also when-let => when-let* to fit with emacs-30+.

Thanks.

-- 
Thierry

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