GNU bug report logs - #52856
29.0.50; Problematic handling of webkit xwidget bookmarks

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 52856 in the body.
You can then email your comments to 52856 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; Problematic handling of webkit xwidget bookmarks
Date: Tue, 28 Dec 2021 20:19:25 +0100

[Message part 1 (text/plain, inline)]

If you make a bookmark to a webkit xwidget (i.e. to a URL displayed by
the xwidget) and then use `bookmark-jump-other-window' or
`bookmark-jump-other-frame' to jump to the bookmarked URL, the xwidget
is displayed in both the original window and the other window or other
frame.  Also, if you try to jump to the bookmark in an Emacs session
before loading xwidget.el, it fails with an unknown symbol error.  The
patch below fixes these issues for me.

2021-12-28  Stephen Berman  <stephen.berman <at> gmx.net>

	Fix handling of webkit xwidget bookmarks

	Make jumping to a bookmarked webkit xwidget in another window or
	another frame show the xwidget only in that window or frame.  Also
	make jumping to the bookmarked xwidget possible before explicitly
	loading xwidget.el (bug#xxxx).

	* lisp/xwidget.el (xwidget-webkit-bookmark-jump-handler): New
	autoloaded function.
	(xwidget-webkit-bookmark-make-record): Use it.

[Message part 2 (text/x-patch, inline)]

diff --git a/lisp/xwidget.el b/lisp/xwidget.el
index ce9839ebd3..eed35b7312 100644
--- a/lisp/xwidget.el
+++ b/lisp/xwidget.el
@@ -540,15 +540,26 @@ xwidget-webkit-bookmark-jump-new-session
   :type 'boolean)

 (defun xwidget-webkit-bookmark-make-record ()
-  "Create bookmark record in webkit xwidget.
-See `xwidget-webkit-bookmark-jump-new-session' for whether this
-should create a new session or not."
+  "Create a bookmark record for a webkit xwidget."
   (nconc (bookmark-make-record-default t t)
          `((page . ,(xwidget-webkit-uri (xwidget-webkit-current-session)))
-           (handler  . (lambda (bmk)
-                         (xwidget-webkit-browse-url
-                          (bookmark-prop-get bmk 'page)
-                          xwidget-webkit-bookmark-jump-new-session))))))
+           (handler . xwidget-webkit-bookmark-jump-handler))))
+
+;;;###autoload
+(defun xwidget-webkit-bookmark-jump-handler (bmk)
+  "Function for jumping to the webkit xwidget bookmarked by BMK.
+If `xwidget-webkit-bookmark-jump-new-session' is non-nil, create
+a new xwidget-webkit session, otherwise use an existing session."
+  (let* ((curbuf (current-buffer))
+	 (url (bookmark-prop-get bmk 'page))
+	 (xwbuf (if xwidget-webkit-bookmark-jump-new-session
+		    (with-temp-buffer
+		      (xwidget-webkit-new-session url)
+		      (current-buffer))
+		  (xwidget-webkit-goto-url url)
+		  (current-buffer))))
+    (switch-to-buffer curbuf)
+    (set-buffer xwbuf)))

 ;;; xwidget webkit session

[Message part 3 (text/plain, inline)]

In GNU Emacs 29.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version 3.24.29, cairo version 1.17.4)
 of 2021-12-23 built on strobelfs
Repository revision: 2fa7feca336dd16c57ffef072e0f0da6fffe4c5f
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Linux From Scratch 10.2-rc1

Configured using:
 'configure --with-xwidgets 'CFLAGS=-Og -g3'
 PKG_CONFIG_PATH=/usr/local/lib/pkgconfig:/opt/qt5/lib/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LCMS2 LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP
SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XPM
XWIDGETS GTK3 ZLIB

Message #10 received at 52856 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 52856 <at> debbugs.gnu.org
Subject: Re: bug#52856: 29.0.50; Problematic handling of webkit xwidget
 bookmarks
Date: Wed, 29 Dec 2021 15:26:11 +0800

Stephen Berman <stephen.berman <at> gmx.net> writes:

> If you make a bookmark to a webkit xwidget (i.e. to a URL displayed by
> the xwidget) and then use `bookmark-jump-other-window' or
> `bookmark-jump-other-frame' to jump to the bookmarked URL, the xwidget
> is displayed in both the original window and the other window or other
> frame.

Does "original window" mean the window that was selected before you ran
`bookmark-jump-other-window' (or frame)?

If so, you should customize `xwidget-webkit-bookmark-jump-new-session'
to a non-nil value instead.

Message #13 received at 52856 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 52856 <at> debbugs.gnu.org
Subject: Re: bug#52856: 29.0.50; Problematic handling of webkit xwidget
 bookmarks
Date: Wed, 29 Dec 2021 11:50:26 +0100

On Wed, 29 Dec 2021 15:26:11 +0800 Po Lu <luangruo <at> yahoo.com> wrote:

> Stephen Berman <stephen.berman <at> gmx.net> writes:
>
>> If you make a bookmark to a webkit xwidget (i.e. to a URL displayed by
>> the xwidget) and then use `bookmark-jump-other-window' or
>> `bookmark-jump-other-frame' to jump to the bookmarked URL, the xwidget
>> is displayed in both the original window and the other window or other
>> frame.
>
> Does "original window" mean the window that was selected before you ran
> `bookmark-jump-other-window' (or frame)?

Yes.

> If so, you should customize `xwidget-webkit-bookmark-jump-new-session'
> to a non-nil value instead.

I did, but that just ensures that jumping to the bookmarked xwidget
creates a new xwidget session -- it does not prevent the xwidget from
being displayed both in the other window/frame and in the originally
selected window.  My patch ensures that the bookmarked xwidget is
displayed only in the other window/frame, which is consistent with the
behavior of `bookmark-jump-other-window' and `bookmark-jump-other-frame'
with other types of bookmarks (e.g. to PDFs in the pdf-tools package),
regardless of whether a new xwidget session is created.

However, my bug report was mistaken in reporting that jumping to a
bookmarked xwidget before loading xwidget.el raises an error -- with
the existing code it succeeds, because there the bookmark uses
`xwidget-webkit-browse-url', which is autoloaded.  I must have tested
that after applying my patch but before adding the autoload cookie
(which is needed with my patch).  Sorry for the confusion.

Steve Berman

Message #16 received at 52856 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 52856 <at> debbugs.gnu.org
Subject: Re: bug#52856: 29.0.50; Problematic handling of webkit xwidget
 bookmarks
Date: Wed, 29 Dec 2021 19:25:52 +0800

Stephen Berman <stephen.berman <at> gmx.net> writes:

> I did, but that just ensures that jumping to the bookmarked xwidget
> creates a new xwidget session -- it does not prevent the xwidget from
> being displayed both in the other window/frame and in the originally
> selected window.  My patch ensures that the bookmarked xwidget is
> displayed only in the other window/frame, which is consistent with the
> behavior of `bookmark-jump-other-window' and `bookmark-jump-other-frame'
> with other types of bookmarks (e.g. to PDFs in the pdf-tools package),
> regardless of whether a new xwidget session is created.

Okay, thanks -- some comments below:

> +		    (with-temp-buffer
> +		      (xwidget-webkit-new-session url)
> +		      (current-buffer))

Why with-temp-buffer?

xwidget-webkit-new-session creates a new buffer and switches to it,
which is not appropriate if you want to obtain a buffer containing the
new xwidget.  I suggest that you write a new function that creates the
buffer, displays the xwidget, and returns the buffer, preferably also
updating xwidget-webkit-new-session to use it as well.

> +    (switch-to-buffer curbuf)
> +    (set-buffer xwbuf)))

See above.

Message #19 received at 52856 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 52856 <at> debbugs.gnu.org
Subject: Re: bug#52856: 29.0.50; Problematic handling of webkit xwidget
 bookmarks
Date: Thu, 30 Dec 2021 11:52:20 +0100

[Message part 1 (text/plain, inline)]

On Wed, 29 Dec 2021 19:25:52 +0800 Po Lu <luangruo <at> yahoo.com> wrote:

> Stephen Berman <stephen.berman <at> gmx.net> writes:
>
>> I did, but that just ensures that jumping to the bookmarked xwidget
>> creates a new xwidget session -- it does not prevent the xwidget from
>> being displayed both in the other window/frame and in the originally
>> selected window.  My patch ensures that the bookmarked xwidget is
>> displayed only in the other window/frame, which is consistent with the
>> behavior of `bookmark-jump-other-window' and `bookmark-jump-other-frame'
>> with other types of bookmarks (e.g. to PDFs in the pdf-tools package),
>> regardless of whether a new xwidget session is created.
>
> Okay, thanks -- some comments below:
>
>> +		    (with-temp-buffer
>> +		      (xwidget-webkit-new-session url)
>> +		      (current-buffer))
>
> Why with-temp-buffer?

Oh, that's a faux pas, the residue of an earlier version; `progn' would
have sufficed, but the new version below does it differently anyway.

> xwidget-webkit-new-session creates a new buffer and switches to it,
> which is not appropriate if you want to obtain a buffer containing the
> new xwidget.  I suggest that you write a new function that creates the
> buffer, displays the xwidget, and returns the buffer, preferably also
> updating xwidget-webkit-new-session to use it as well.
>
>> +    (switch-to-buffer curbuf)
>> +    (set-buffer xwbuf)))
>
> See above.

Thanks for the suggestion, see the patch below.  Since the previous
bookmark handler was the only user of `xwidget-webkit-browse-url''s
`new-session' parameter, I removed that, and took the opportunity to
make some other cleanups to that function, see the commit message.
While testing the refactored `xwidget-webkit-new-session' I found that
the xwidget-webkit-clone-* commands were broken because
`xwidget-webkit-current-url' returned the message instead of the URL, so
I fixed that.  And I removed from the doc string of
`xwidget-webkit-bookmark-jump-new-session' the suggestion to customize
`xwidget-webkit-last-session-buffer', since, for one thing, it's not a
defcustom, but more importantly, changing it has no effect, since it
gets set by `xwidget-webkit--create-new-session-buffer' (previously by
`xwidget-webkit-new-session'), overriding any user change to its value.


2021-12-30  Stephen Berman  <stephen.berman <at> gmx.net>

	Fix handling of webkit xwidget bookmarks

	Make jumping to a bookmarked webkit xwidget in another window or
	another frame show the xwidget only in that window or frame.
	Adjust xwidget.el code accordingly.  Make xwidget-webkit-clone-*
	commands work. (Bug#52856)

	* lisp/xwidget.el (xwidget-webkit-browse-url): Remove
	`new-session' parameter, since the bookmark handler, which was its
	only user, no longer calls this function.  Remove superfluous
	`browse-url' require from interactive spec and discard the unused
	flag returned by `browse-url-interactive-arg'.  Incorporate the
	code of the now removed `xwidget-webkit-goto-url', since this
	function was its only caller.
	(xwidget-webkit-goto-url): Remove.
	(xwidget-webkit-bookmark-jump-new-session): Adjust doc string,
	rephrasing and removing a customization suggestion that cannot
	take effect.
	(xwidget-webkit-bookmark-jump-handler): New autoloaded function.
	(xwidget-webkit-bookmark-make-record): Use it.
	(xwidget-webkit--create-new-session-buffer): New function
	extracted from `xwidget-webkit-new-session'.
	(xwidget-webkit-new-session): Use it.
	(xwidget-webkit-current-url): Return the URL instead of the
	message displaying it, since the xwidget-webkit-clone-* commands
	need it.

[Message part 2 (text/x-patch, inline)]

diff --git a/lisp/xwidget.el b/lisp/xwidget.el
index ce9839ebd3..42b5e08055 100644
--- a/lisp/xwidget.el
+++ b/lisp/xwidget.el
@@ -116,22 +116,21 @@ xwidget-webkit-cookie-file
   :version "29.1")

 ;;;###autoload
-(defun xwidget-webkit-browse-url (url &optional new-session)
-  "Ask xwidget-webkit to browse URL.
-NEW-SESSION specifies whether to create a new xwidget-webkit session.
-Interactively, URL defaults to the string looking like a url around point."
-  (interactive (progn
-                 (require 'browse-url)
-                 (browse-url-interactive-arg "xwidget-webkit URL: ")))
+(defun xwidget-webkit-browse-url (url)
+  "Prompt for a URL and display it in a webkit xwidget.
+The URL defaults to the string looking like a url around point."
+  (interactive (list (car (browse-url-interactive-arg "xwidget-webkit URL: "))))
   (or (featurep 'xwidget-internal)
       (user-error "Your Emacs was not compiled with xwidgets support"))
   (when (stringp url)
     ;; If it's a "naked url", just try adding https: to it.
     (unless (string-match "\\`[A-Za-z]+:" url)
       (setq url (concat "https://" url)))
-    (if new-session
-        (xwidget-webkit-new-session url)
-      (xwidget-webkit-goto-url url))))
+    (if (xwidget-webkit-current-session)
+        (progn
+          (xwidget-webkit-goto-uri (xwidget-webkit-current-session) url)
+          (switch-to-buffer (xwidget-buffer (xwidget-webkit-current-session))))
+      (xwidget-webkit-new-session url))))

 (defun xwidget-webkit-clone-and-split-below ()
   "Clone current URL into a new widget place in new window below.
@@ -531,24 +530,31 @@ xwidget-webkit-save-as-file
 ;;; Bookmarks integration

 (defcustom xwidget-webkit-bookmark-jump-new-session nil
-  "Control bookmark jump to use new session or not.
-If non-nil, use a new xwidget webkit session after bookmark jump.
-Otherwise, it will use `xwidget-webkit-last-session'.
-When you set this variable to nil, consider further customization with
-`xwidget-webkit-last-session-buffer'."
+  "Whether to jump to a bookmarked URL in a new xwidget webkit session.
+If non-nil, create a new xwidget webkit session, otherwise use
+the value of `xwidget-webkit-last-session'."
   :version "28.1"
   :type 'boolean)

 (defun xwidget-webkit-bookmark-make-record ()
-  "Create bookmark record in webkit xwidget.
-See `xwidget-webkit-bookmark-jump-new-session' for whether this
-should create a new session or not."
+  "Create a bookmark record for a webkit xwidget."
   (nconc (bookmark-make-record-default t t)
          `((page . ,(xwidget-webkit-uri (xwidget-webkit-current-session)))
-           (handler  . (lambda (bmk)
-                         (xwidget-webkit-browse-url
-                          (bookmark-prop-get bmk 'page)
-                          xwidget-webkit-bookmark-jump-new-session))))))
+           (handler . xwidget-webkit-bookmark-jump-handler))))
+
+;;;###autoload
+(defun xwidget-webkit-bookmark-jump-handler (bmk)
+  "Function for jumping to the webkit xwidget bookmarked by BMK.
+If `xwidget-webkit-bookmark-jump-new-session' is non-nil, create
+a new xwidget-webkit session, otherwise use an existing session."
+  (let* ((url (bookmark-prop-get bmk 'page))
+	 (xwbuf (if (or xwidget-webkit-bookmark-jump-new-session
+                        (not (xwidget-webkit-current-session)))
+	            (xwidget-webkit--create-new-session-buffer url)
+                  (xwidget-buffer (xwidget-webkit-current-session)))))
+    (with-current-buffer xwbuf
+      (xwidget-webkit-goto-uri (xwidget-webkit-current-session) url))
+    (set-buffer xwbuf)))

 ;;; xwidget webkit session

@@ -796,37 +802,44 @@ xwidget-webkit-adjust-size-in-frame
   (add-to-list 'window-size-change-functions
                'xwidget-webkit-adjust-size-in-frame))

-(defun xwidget-webkit-new-session (url &optional callback)
-  "Create a new webkit session buffer with URL."
+(defun xwidget-webkit--create-new-session-buffer (url &optional callback)
+  "Create a new webkit session buffer to display URL in an xwidget.
+Optional function CALLBACK specifies the callback for webkit xwidgets;
+see `xwidget-webkit-callback'."
   (let* ((bufname
-          ;; Generate a temp-name based on current buffer name. it
-          ;; will be renamed by `xwidget-webkit-callback' in the
-          ;; future. This approach can limit flicker of buffer-name in
-          ;; mode-line.
+          ;; Generate a temp-name based on current buffer name.  The
+          ;; buffer will subsequently be renamed by
+          ;; `xwidget-webkit-callback'.  This approach can avoid
+          ;; flicker of buffer-name in mode-line.
           (generate-new-buffer-name (buffer-name)))
          (callback (or callback #'xwidget-webkit-callback))
          (current-session (xwidget-webkit-current-session))
          xw)
-    (setq xwidget-webkit-last-session-buffer (switch-to-buffer
-                                              (get-buffer-create bufname)))
+    (setq xwidget-webkit-last-session-buffer (get-buffer-create bufname))
     ;; The xwidget id is stored in a text property, so we need to have
     ;; at least character in this buffer.
     ;; Insert invisible url, good default for next `g' to browse url.
-    (let ((start (point)))
-      (insert url)
-      (put-text-property start (+ start (length url)) 'invisible t)
-      (setq xw (xwidget-insert
-                start 'webkit bufname
-                (xwidget-window-inside-pixel-width (selected-window))
-                (xwidget-window-inside-pixel-height (selected-window))
-                nil current-session)))
-    (when xwidget-webkit-cookie-file
-      (xwidget-webkit-set-cookie-storage-file
-       xw (expand-file-name xwidget-webkit-cookie-file)))
-    (xwidget-put xw 'callback callback)
-    (xwidget-put xw 'display-callback #'xwidget-webkit-display-callback)
-    (xwidget-webkit-mode)
-    (xwidget-webkit-goto-uri (xwidget-webkit-last-session) url)))
+    (with-current-buffer xwidget-webkit-last-session-buffer
+      (let ((start (point)))
+        (insert url)
+        (put-text-property start (+ start (length url)) 'invisible t)
+        (setq xw (xwidget-insert
+                  start 'webkit bufname
+                  (xwidget-window-inside-pixel-width (selected-window))
+                  (xwidget-window-inside-pixel-height (selected-window))
+                  nil current-session)))
+      (when xwidget-webkit-cookie-file
+        (xwidget-webkit-set-cookie-storage-file
+         xw (expand-file-name xwidget-webkit-cookie-file)))
+      (xwidget-put xw 'callback callback)
+      (xwidget-put xw 'display-callback #'xwidget-webkit-display-callback)
+      (xwidget-webkit-mode))
+    xwidget-webkit-last-session-buffer))
+
+(defun xwidget-webkit-new-session (url)
+  "Display URL in a new webkit xwidget."
+  (switch-to-buffer (xwidget-webkit--create-new-session-buffer url))
+  (xwidget-webkit-goto-uri (xwidget-webkit-last-session) url))

 (defun xwidget-webkit-import-widget (xwidget)
   "Create a new webkit session buffer from XWIDGET, an existing xwidget.
@@ -868,14 +881,6 @@ xwidget-webkit-display-callback

 (define-key special-event-map [xwidget-display-event] 'xwidget-webkit-display-event)

-(defun xwidget-webkit-goto-url (url)
-  "Goto URL with xwidget webkit."
-  (if (xwidget-webkit-current-session)
-      (progn
-        (xwidget-webkit-goto-uri (xwidget-webkit-current-session) url)
-        (switch-to-buffer (xwidget-buffer (xwidget-webkit-current-session))))
-    (xwidget-webkit-new-session url)))
-
 (defun xwidget-webkit-back ()
   "Go back to previous URL in xwidget webkit buffer."
   (interactive nil xwidget-webkit-mode)
@@ -892,10 +897,12 @@ xwidget-webkit-reload
   (xwidget-webkit-goto-history (xwidget-webkit-current-session) 0))

 (defun xwidget-webkit-current-url ()
-  "Display the current xwidget webkit URL and place it on the `kill-ring'."
+  "Return current xwidget webkit URL and display it in a message.
+Also place it on the `kill-ring'."
   (interactive nil xwidget-webkit-mode)
   (let ((url (xwidget-webkit-uri (xwidget-webkit-current-session))))
-    (message "URL: %s" (kill-new (or url "")))))
+    (message "URL: %s" (kill-new (or url "")))
+    url))

 (defun xwidget-webkit-browse-history ()
   "Display a buffer containing the history of page loads."

Message #22 received at 52856 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 52856 <at> debbugs.gnu.org
Subject: Re: bug#52856: 29.0.50; Problematic handling of webkit xwidget
 bookmarks
Date: Thu, 30 Dec 2021 19:18:57 +0800

Stephen Berman <stephen.berman <at> gmx.net> writes:

> Thanks for the suggestion, see the patch below.  Since the previous
> bookmark handler was the only user of `xwidget-webkit-browse-url''s
> `new-session' parameter, I removed that, and took the opportunity to
> make some other cleanups to that function, see the commit message.
> While testing the refactored `xwidget-webkit-new-session' I found that
> the xwidget-webkit-clone-* commands were broken because
> `xwidget-webkit-current-url' returned the message instead of the URL, so
> I fixed that.  And I removed from the doc string of
> `xwidget-webkit-bookmark-jump-new-session' the suggestion to customize
> `xwidget-webkit-last-session-buffer', since, for one thing, it's not a
> defcustom, but more importantly, changing it has no effect, since it
> gets set by `xwidget-webkit--create-new-session-buffer' (previously by
> `xwidget-webkit-new-session'), overriding any user change to its
> value.

Thanks, but there are a few problems with this change:

> 2021-12-30  Stephen Berman  <stephen.berman <at> gmx.net>
>
> 	Fix handling of webkit xwidget bookmarks
>
> 	Make jumping to a bookmarked webkit xwidget in another window or
> 	another frame show the xwidget only in that window or frame.
> 	Adjust xwidget.el code accordingly.  Make xwidget-webkit-clone-*
> 	commands work. (Bug#52856)
>
> 	* lisp/xwidget.el (xwidget-webkit-browse-url): Remove
> 	`new-session' parameter, since the bookmark handler, which was its
> 	only user, no longer calls this function.  Remove superfluous
> 	`browse-url' require from interactive spec and discard the unused
> 	flag returned by `browse-url-interactive-arg'.  Incorporate the
> 	code of the now removed `xwidget-webkit-goto-url', since this
> 	function was its only caller.

While that might be the case in-tree, external packages and users might
be using that parameter -- it should stay.

> 	(xwidget-webkit-goto-url): Remove.

The same goes for this.  I think it's certain that some external code
will be using this function, as the name is rather tempting.

I also have some further comments below:

> +        (progn
> +          (xwidget-webkit-goto-uri (xwidget-webkit-current-session) url)
> +          (switch-to-buffer (xwidget-buffer (xwidget-webkit-current-session))))

Since you already define a replacement for `xwidget-webkit-new-session'
below that also behaves like this, is there any reason you can't use
that here instead?

> +(defun xwidget-webkit-bookmark-jump-handler (bmk)
> +  "Function for jumping to the webkit xwidget bookmarked by BMK.
> +If `xwidget-webkit-bookmark-jump-new-session' is non-nil, create
> +a new xwidget-webkit session, otherwise use an existing session."

I think it would be clearer to say "Jump to the page described by the
bookmark record BMK."

But I'm not good at writing documentation, so I won't insist.

Also, `bmk' should be named `bookmark', or something else to that
effect.

> +  "Return current xwidget webkit URL and display it in a message.
> +Also place it on the `kill-ring'."

I don't understand why this user command has to be called
programmatically, and why it should return the URL.  Can't you use
`xwidget-webkit-uri' instead?

And thanks for the work you've been doing.  I really appreciate it.

Message #25 received at 52856 <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 52856 <at> debbugs.gnu.org
Subject: Re: bug#52856: 29.0.50; Problematic handling of webkit xwidget
 bookmarks
Date: Thu, 30 Dec 2021 16:00:09 +0100

[Message part 1 (text/plain, inline)]

On Thu, 30 Dec 2021 19:18:57 +0800 Po Lu <luangruo <at> yahoo.com> wrote:

>> 2021-12-30  Stephen Berman  <stephen.berman <at> gmx.net>
>>
>> 	Fix handling of webkit xwidget bookmarks
>>
>> 	Make jumping to a bookmarked webkit xwidget in another window or
>> 	another frame show the xwidget only in that window or frame.
>> 	Adjust xwidget.el code accordingly.  Make xwidget-webkit-clone-*
>> 	commands work. (Bug#52856)
>>
>> 	* lisp/xwidget.el (xwidget-webkit-browse-url): Remove
>> 	`new-session' parameter, since the bookmark handler, which was its
>> 	only user, no longer calls this function.  Remove superfluous
>> 	`browse-url' require from interactive spec and discard the unused
>> 	flag returned by `browse-url-interactive-arg'.  Incorporate the
>> 	code of the now removed `xwidget-webkit-goto-url', since this
>> 	function was its only caller.
>
> While that might be the case in-tree, external packages and users might
> be using that parameter -- it should stay.

Ok.  (The superfluous `browse-url' require can be removed by future code
cleanup.)

>> 	(xwidget-webkit-goto-url): Remove.
>
> The same goes for this.  I think it's certain that some external code
> will be using this function, as the name is rather tempting.

Ok.

> I also have some further comments below:
>
>> +        (progn
>> +          (xwidget-webkit-goto-uri (xwidget-webkit-current-session) url)
>> +          (switch-to-buffer (xwidget-buffer (xwidget-webkit-current-session))))
>
> Since you already define a replacement for `xwidget-webkit-new-session'
> below that also behaves like this, is there any reason you can't use
> that here instead?

This is the code from the not-to-be-removed `xwidget-webkit-goto-url'
for reusing the current xwidget-webkit session instead of creating a new
session.  This is the only occurrence of this sexp, so encapsulating it
in a function seems like overkill.

>> +(defun xwidget-webkit-bookmark-jump-handler (bmk)
>> +  "Function for jumping to the webkit xwidget bookmarked by BMK.
>> +If `xwidget-webkit-bookmark-jump-new-session' is non-nil, create
>> +a new xwidget-webkit session, otherwise use an existing session."
>
> I think it would be clearer to say "Jump to the page described by the
> bookmark record BMK."
>
> But I'm not good at writing documentation, so I won't insist.
>
> Also, `bmk' should be named `bookmark', or something else to that
> effect.

How's this: "Jump to the web page bookmarked by the bookmark record
BOOKMARK."?

>> +  "Return current xwidget webkit URL and display it in a message.
>> +Also place it on the `kill-ring'."
>
> I don't understand why this user command has to be called
> programmatically, and why it should return the URL.  Can't you use
> `xwidget-webkit-uri' instead?

Sure.

> And thanks for the work you've been doing.  I really appreciate it.

Thanks.  Here's the latest version:


2021-12-30  Stephen Berman  <stephen.berman <at> gmx.net>

	Fix handling of webkit xwidget bookmarks

	Make jumping to a bookmarked webkit xwidget in another window or
	another frame show the xwidget only in that window or frame and
	refactor new session code used by the bookmark code (bug#52856).
	In addition, make xwidget-webkit-clone-* commands work.

	* lisp/xwidget.el (xwidget-webkit-clone-and-split-below)
	(xwidget-webkit-clone-and-split-right): Unbreak these functions by
	passing just the URL to them, not the message displaying it.
	(xwidget-webkit-bookmark-jump-new-session): Adjust doc string,
	rephrasing and removing a customization suggestion that cannot
	take effect.
	(xwidget-webkit-bookmark-jump-handler): New autoloaded function.
	(xwidget-webkit-bookmark-make-record): Use it.
	(xwidget-webkit--create-new-session-buffer): New function
	extracted from `xwidget-webkit-new-session'.
	(xwidget-webkit-new-session): Use it.

[Message part 2 (text/x-patch, inline)]

diff --git a/lisp/xwidget.el b/lisp/xwidget.el
index ce9839ebd3..1f92966492 100644
--- a/lisp/xwidget.el
+++ b/lisp/xwidget.el
@@ -138,7 +138,7 @@ xwidget-webkit-clone-and-split-below
 Get the URL of current session, then browse to the URL
 in `split-window-below' with a new xwidget webkit session."
   (interactive nil xwidget-webkit-mode)
-  (let ((url (xwidget-webkit-current-url)))
+  (let ((url (xwidget-webkit-uri (xwidget-webkit-current-session))))
     (with-selected-window (split-window-below)
       (xwidget-webkit-new-session url))))

@@ -147,7 +147,7 @@ xwidget-webkit-clone-and-split-right
 Get the URL of current session, then browse to the URL
 in `split-window-right' with a new xwidget webkit session."
   (interactive nil xwidget-webkit-mode)
-  (let ((url (xwidget-webkit-current-url)))
+  (let ((url (xwidget-webkit-uri (xwidget-webkit-current-session))))
     (with-selected-window (split-window-right)
       (xwidget-webkit-new-session url))))

@@ -531,24 +531,31 @@ xwidget-webkit-save-as-file
 ;;; Bookmarks integration

 (defcustom xwidget-webkit-bookmark-jump-new-session nil
-  "Control bookmark jump to use new session or not.
-If non-nil, use a new xwidget webkit session after bookmark jump.
-Otherwise, it will use `xwidget-webkit-last-session'.
-When you set this variable to nil, consider further customization with
-`xwidget-webkit-last-session-buffer'."
+  "Whether to jump to a bookmarked URL in a new xwidget webkit session.
+If non-nil, create a new xwidget webkit session, otherwise use
+the value of `xwidget-webkit-last-session'."
   :version "28.1"
   :type 'boolean)

 (defun xwidget-webkit-bookmark-make-record ()
-  "Create bookmark record in webkit xwidget.
-See `xwidget-webkit-bookmark-jump-new-session' for whether this
-should create a new session or not."
+  "Create a bookmark record for a webkit xwidget."
   (nconc (bookmark-make-record-default t t)
          `((page . ,(xwidget-webkit-uri (xwidget-webkit-current-session)))
-           (handler  . (lambda (bmk)
-                         (xwidget-webkit-browse-url
-                          (bookmark-prop-get bmk 'page)
-                          xwidget-webkit-bookmark-jump-new-session))))))
+           (handler . xwidget-webkit-bookmark-jump-handler))))
+
+;;;###autoload
+(defun xwidget-webkit-bookmark-jump-handler (bookmark)
+  "Jump to the web page bookmarked by the bookmark record BOOKMARK.
+If `xwidget-webkit-bookmark-jump-new-session' is non-nil, create
+a new xwidget-webkit session, otherwise use an existing session."
+  (let* ((url (bookmark-prop-get bookmark 'page))
+	 (xwbuf (if (or xwidget-webkit-bookmark-jump-new-session
+                        (not (xwidget-webkit-current-session)))
+	            (xwidget-webkit--create-new-session-buffer url)
+                  (xwidget-buffer (xwidget-webkit-current-session)))))
+    (with-current-buffer xwbuf
+      (xwidget-webkit-goto-uri (xwidget-webkit-current-session) url))
+    (set-buffer xwbuf)))

 ;;; xwidget webkit session

@@ -796,37 +803,44 @@ xwidget-webkit-adjust-size-in-frame
   (add-to-list 'window-size-change-functions
                'xwidget-webkit-adjust-size-in-frame))

-(defun xwidget-webkit-new-session (url &optional callback)
-  "Create a new webkit session buffer with URL."
+(defun xwidget-webkit--create-new-session-buffer (url &optional callback)
+  "Create a new webkit session buffer to display URL in an xwidget.
+Optional function CALLBACK specifies the callback for webkit xwidgets;
+see `xwidget-webkit-callback'."
   (let* ((bufname
-          ;; Generate a temp-name based on current buffer name. it
-          ;; will be renamed by `xwidget-webkit-callback' in the
-          ;; future. This approach can limit flicker of buffer-name in
-          ;; mode-line.
+          ;; Generate a temp-name based on current buffer name.  The
+          ;; buffer will subsequently be renamed by
+          ;; `xwidget-webkit-callback'.  This approach can avoid
+          ;; flicker of buffer-name in mode-line.
           (generate-new-buffer-name (buffer-name)))
          (callback (or callback #'xwidget-webkit-callback))
          (current-session (xwidget-webkit-current-session))
          xw)
-    (setq xwidget-webkit-last-session-buffer (switch-to-buffer
-                                              (get-buffer-create bufname)))
+    (setq xwidget-webkit-last-session-buffer (get-buffer-create bufname))
     ;; The xwidget id is stored in a text property, so we need to have
     ;; at least character in this buffer.
     ;; Insert invisible url, good default for next `g' to browse url.
-    (let ((start (point)))
-      (insert url)
-      (put-text-property start (+ start (length url)) 'invisible t)
-      (setq xw (xwidget-insert
-                start 'webkit bufname
-                (xwidget-window-inside-pixel-width (selected-window))
-                (xwidget-window-inside-pixel-height (selected-window))
-                nil current-session)))
-    (when xwidget-webkit-cookie-file
-      (xwidget-webkit-set-cookie-storage-file
-       xw (expand-file-name xwidget-webkit-cookie-file)))
-    (xwidget-put xw 'callback callback)
-    (xwidget-put xw 'display-callback #'xwidget-webkit-display-callback)
-    (xwidget-webkit-mode)
-    (xwidget-webkit-goto-uri (xwidget-webkit-last-session) url)))
+    (with-current-buffer xwidget-webkit-last-session-buffer
+      (let ((start (point)))
+        (insert url)
+        (put-text-property start (+ start (length url)) 'invisible t)
+        (setq xw (xwidget-insert
+                  start 'webkit bufname
+                  (xwidget-window-inside-pixel-width (selected-window))
+                  (xwidget-window-inside-pixel-height (selected-window))
+                  nil current-session)))
+      (when xwidget-webkit-cookie-file
+        (xwidget-webkit-set-cookie-storage-file
+         xw (expand-file-name xwidget-webkit-cookie-file)))
+      (xwidget-put xw 'callback callback)
+      (xwidget-put xw 'display-callback #'xwidget-webkit-display-callback)
+      (xwidget-webkit-mode))
+    xwidget-webkit-last-session-buffer))
+
+(defun xwidget-webkit-new-session (url)
+  "Display URL in a new webkit xwidget."
+  (switch-to-buffer (xwidget-webkit--create-new-session-buffer url))
+  (xwidget-webkit-goto-uri (xwidget-webkit-last-session) url))

 (defun xwidget-webkit-import-widget (xwidget)
   "Create a new webkit session buffer from XWIDGET, an existing xwidget.

Message #28 received at 52856 <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 52856 <at> debbugs.gnu.org
Subject: Re: bug#52856: 29.0.50; Problematic handling of webkit xwidget
 bookmarks
Date: Fri, 31 Dec 2021 08:56:58 +0800

Stephen Berman <stephen.berman <at> gmx.net> writes:

> How's this: "Jump to the web page bookmarked by the bookmark record
> BOOKMARK."?

Sure.

> Thanks.  Here's the latest version:

Thanks, LGTM.  Have you signed the copyright papers yet?

> 2021-12-30  Stephen Berman  <stephen.berman <at> gmx.net>
>
> 	Fix handling of webkit xwidget bookmarks
>
> 	Make jumping to a bookmarked webkit xwidget in another window or
> 	another frame show the xwidget only in that window or frame and
> 	refactor new session code used by the bookmark code (bug#52856).
> 	In addition, make xwidget-webkit-clone-* commands work.
>
> 	* lisp/xwidget.el (xwidget-webkit-clone-and-split-below)
> 	(xwidget-webkit-clone-and-split-right): Unbreak these functions by
> 	passing just the URL to them, not the message displaying it.
> 	(xwidget-webkit-bookmark-jump-new-session): Adjust doc string,
> 	rephrasing and removing a customization suggestion that cannot
> 	take effect.
> 	(xwidget-webkit-bookmark-jump-handler): New autoloaded function.
> 	(xwidget-webkit-bookmark-make-record): Use it.
> 	(xwidget-webkit--create-new-session-buffer): New function
> 	extracted from `xwidget-webkit-new-session'.
> 	(xwidget-webkit-new-session): Use it.
>
> diff --git a/lisp/xwidget.el b/lisp/xwidget.el
> index ce9839ebd3..1f92966492 100644
> --- a/lisp/xwidget.el
> +++ b/lisp/xwidget.el
> @@ -138,7 +138,7 @@ xwidget-webkit-clone-and-split-below
>  Get the URL of current session, then browse to the URL
>  in `split-window-below' with a new xwidget webkit session."
>    (interactive nil xwidget-webkit-mode)
> -  (let ((url (xwidget-webkit-current-url)))
> +  (let ((url (xwidget-webkit-uri (xwidget-webkit-current-session))))
>      (with-selected-window (split-window-below)
>        (xwidget-webkit-new-session url))))
>
> @@ -147,7 +147,7 @@ xwidget-webkit-clone-and-split-right
>  Get the URL of current session, then browse to the URL
>  in `split-window-right' with a new xwidget webkit session."
>    (interactive nil xwidget-webkit-mode)
> -  (let ((url (xwidget-webkit-current-url)))
> +  (let ((url (xwidget-webkit-uri (xwidget-webkit-current-session))))
>      (with-selected-window (split-window-right)
>        (xwidget-webkit-new-session url))))
>
> @@ -531,24 +531,31 @@ xwidget-webkit-save-as-file
>  ;;; Bookmarks integration
>
>  (defcustom xwidget-webkit-bookmark-jump-new-session nil
> -  "Control bookmark jump to use new session or not.
> -If non-nil, use a new xwidget webkit session after bookmark jump.
> -Otherwise, it will use `xwidget-webkit-last-session'.
> -When you set this variable to nil, consider further customization with
> -`xwidget-webkit-last-session-buffer'."
> +  "Whether to jump to a bookmarked URL in a new xwidget webkit session.
> +If non-nil, create a new xwidget webkit session, otherwise use
> +the value of `xwidget-webkit-last-session'."
>    :version "28.1"
>    :type 'boolean)
>
>  (defun xwidget-webkit-bookmark-make-record ()
> -  "Create bookmark record in webkit xwidget.
> -See `xwidget-webkit-bookmark-jump-new-session' for whether this
> -should create a new session or not."
> +  "Create a bookmark record for a webkit xwidget."
>    (nconc (bookmark-make-record-default t t)
>           `((page . ,(xwidget-webkit-uri (xwidget-webkit-current-session)))
> -           (handler  . (lambda (bmk)
> -                         (xwidget-webkit-browse-url
> -                          (bookmark-prop-get bmk 'page)
> -                          xwidget-webkit-bookmark-jump-new-session))))))
> +           (handler . xwidget-webkit-bookmark-jump-handler))))
> +
> +;;;###autoload
> +(defun xwidget-webkit-bookmark-jump-handler (bookmark)
> +  "Jump to the web page bookmarked by the bookmark record BOOKMARK.
> +If `xwidget-webkit-bookmark-jump-new-session' is non-nil, create
> +a new xwidget-webkit session, otherwise use an existing session."
> +  (let* ((url (bookmark-prop-get bookmark 'page))
> +	 (xwbuf (if (or xwidget-webkit-bookmark-jump-new-session
> +                        (not (xwidget-webkit-current-session)))
> +	            (xwidget-webkit--create-new-session-buffer url)
> +                  (xwidget-buffer (xwidget-webkit-current-session)))))
> +    (with-current-buffer xwbuf
> +      (xwidget-webkit-goto-uri (xwidget-webkit-current-session) url))
> +    (set-buffer xwbuf)))
>
>  ;;; xwidget webkit session
>
> @@ -796,37 +803,44 @@ xwidget-webkit-adjust-size-in-frame
>    (add-to-list 'window-size-change-functions
>                 'xwidget-webkit-adjust-size-in-frame))
>
> -(defun xwidget-webkit-new-session (url &optional callback)
> -  "Create a new webkit session buffer with URL."
> +(defun xwidget-webkit--create-new-session-buffer (url &optional callback)
> +  "Create a new webkit session buffer to display URL in an xwidget.
> +Optional function CALLBACK specifies the callback for webkit xwidgets;
> +see `xwidget-webkit-callback'."
>    (let* ((bufname
> -          ;; Generate a temp-name based on current buffer name. it
> -          ;; will be renamed by `xwidget-webkit-callback' in the
> -          ;; future. This approach can limit flicker of buffer-name in
> -          ;; mode-line.
> +          ;; Generate a temp-name based on current buffer name.  The
> +          ;; buffer will subsequently be renamed by
> +          ;; `xwidget-webkit-callback'.  This approach can avoid
> +          ;; flicker of buffer-name in mode-line.
>            (generate-new-buffer-name (buffer-name)))
>           (callback (or callback #'xwidget-webkit-callback))
>           (current-session (xwidget-webkit-current-session))
>           xw)
> -    (setq xwidget-webkit-last-session-buffer (switch-to-buffer
> -                                              (get-buffer-create bufname)))
> +    (setq xwidget-webkit-last-session-buffer (get-buffer-create bufname))
>      ;; The xwidget id is stored in a text property, so we need to have
>      ;; at least character in this buffer.
>      ;; Insert invisible url, good default for next `g' to browse url.
> -    (let ((start (point)))
> -      (insert url)
> -      (put-text-property start (+ start (length url)) 'invisible t)
> -      (setq xw (xwidget-insert
> -                start 'webkit bufname
> -                (xwidget-window-inside-pixel-width (selected-window))
> -                (xwidget-window-inside-pixel-height (selected-window))
> -                nil current-session)))
> -    (when xwidget-webkit-cookie-file
> -      (xwidget-webkit-set-cookie-storage-file
> -       xw (expand-file-name xwidget-webkit-cookie-file)))
> -    (xwidget-put xw 'callback callback)
> -    (xwidget-put xw 'display-callback #'xwidget-webkit-display-callback)
> -    (xwidget-webkit-mode)
> -    (xwidget-webkit-goto-uri (xwidget-webkit-last-session) url)))
> +    (with-current-buffer xwidget-webkit-last-session-buffer
> +      (let ((start (point)))
> +        (insert url)
> +        (put-text-property start (+ start (length url)) 'invisible t)
> +        (setq xw (xwidget-insert
> +                  start 'webkit bufname
> +                  (xwidget-window-inside-pixel-width (selected-window))
> +                  (xwidget-window-inside-pixel-height (selected-window))
> +                  nil current-session)))
> +      (when xwidget-webkit-cookie-file
> +        (xwidget-webkit-set-cookie-storage-file
> +         xw (expand-file-name xwidget-webkit-cookie-file)))
> +      (xwidget-put xw 'callback callback)
> +      (xwidget-put xw 'display-callback #'xwidget-webkit-display-callback)
> +      (xwidget-webkit-mode))
> +    xwidget-webkit-last-session-buffer))
> +
> +(defun xwidget-webkit-new-session (url)
> +  "Display URL in a new webkit xwidget."
> +  (switch-to-buffer (xwidget-webkit--create-new-session-buffer url))
> +  (xwidget-webkit-goto-uri (xwidget-webkit-last-session) url))
>
>  (defun xwidget-webkit-import-widget (xwidget)
>    "Create a new webkit session buffer from XWIDGET, an existing xwidget.

Message #33 received at 52856-done <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 52856-done <at> debbugs.gnu.org
Subject: Re: bug#52856: 29.0.50; Problematic handling of webkit xwidget
 bookmarks
Date: Fri, 31 Dec 2021 10:29:57 +0100

On Fri, 31 Dec 2021 08:56:58 +0800 Po Lu <luangruo <at> yahoo.com> wrote:

> Stephen Berman <stephen.berman <at> gmx.net> writes:
>
>> How's this: "Jump to the web page bookmarked by the bookmark record
>> BOOKMARK."?
>
> Sure.
>
>> Thanks.  Here's the latest version:
>
> Thanks, LGTM.  Have you signed the copyright papers yet?

Yes, and I also have write access to the Savannah repo, so I went ahead
and pushed the change to master as commit 95ee6e8b90 and am closing the
bug.  Thanks for your feedback and advice.

Steve Berman

Message #36 received at 52856-done <at> debbugs.gnu.org (full text, mbox):

From: Po Lu <luangruo <at> yahoo.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 52856-done <at> debbugs.gnu.org
Subject: Re: bug#52856: 29.0.50; Problematic handling of webkit xwidget
 bookmarks
Date: Fri, 31 Dec 2021 17:45:58 +0800

Stephen Berman <stephen.berman <at> gmx.net> writes:

> Yes, and I also have write access to the Savannah repo, so I went ahead
> and pushed the change to master as commit 95ee6e8b90 and am closing the
> bug.  Thanks for your feedback and advice.

Thanks!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.