bug#52856: 29.0.50; Problematic handling of webkit xwidget bookmarks

[Stephen Berman]

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

> Stephen Berman <stephen.berman@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@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.

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."