bug#71450: [PATCH] Wrong eww-history-position after desktop restore if within history

bug#71450: [PATCH] Wrong eww-history-position after desktop restore if within history

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

- emacs -Q
- M-x eww RET <any website> RET
- RET (on any link)
- l
- M-x desktop-save RET <any directory> RET
- C-x C-c (exit)
- emacs -Q
- M-x desktop-read RET <the above directory> RET
- g (to reload the webpage)
- RET (on any link) fails

In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.18.0) of 2024-06-09 built on
 user-Inspiron-3493
Repository revision: e9a0256a556622474bcbb015f88d790666db2cc9
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101007
System Description: Ubuntu 23.10

Configured using:
 'configure --with-native-compilation=aot'

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

Important settings:
  value of $LANG: en_IN
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: iso-latin-1-unix

Major mode: VC dir

Minor modes in effect:
  rcirc-track-minor-mode: t
  display-time-mode: t
  pdf-occur-global-minor-mode: t
  vc-dir-git-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  desktop-environment-mode: t
  server-mode: t
  erc-track-mode: t
  erc-ring-mode: t
  erc-netsplit-mode: t
  erc-menu-mode: t
  erc-match-mode: t
  erc-log-mode: t
  erc-list-mode: t
  erc-irccontrols-mode: t
  erc-noncommands-mode: t
  erc-move-to-prompt-mode: t
  erc-readonly-mode: t
  erc-button-mode: t
  erc-fill-mode: t
  erc-stamp-mode: t
  erc-autojoin-mode: t
  savehist-mode: t
  midnight-mode: t
  icomplete-mode: t
  fido-mode: t
  erc-networks-mode: t
  display-battery-mode: t
  desktop-save-mode: t
  override-global-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  minibuffer-regexp-mode: t
  buffer-read-only: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

This is a patch that I think is simple enough to forgo the extensive
testing which it hasn't been subjected to.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Correct eww-history-position in desktop restore --]
[-- Type: text/x-patch, Size: 2454 bytes --]

From c5a9be613fb2c0a96db0dadb11ff2584c4ebbc8c Mon Sep 17 00:00:00 2001
From: James Thomas <jimjoe@gmx.net>
Date: Sun, 9 Jun 2024 17:35:21 +0530
Subject: [PATCH] Correct eww-history-position in desktop restore

Account for duplicate removal from eww-history.

* lisp/net/eww.el (eww-desktop-misc-data): Add :history-position
(eww-restore-desktop): Use it.
(desktop-locals-to-save): Remove the raw variable.
---
 lisp/net/eww.el | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 977210e9cc8..bd63e52ee77 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2754,11 +2754,17 @@ eww-desktop-misc-data
 Generally, the list should not include the (usually overly large)
 :dom, :source and :text properties."
   (let ((history (mapcar #'eww-desktop-data-1
-                         (cons eww-data eww-history))))
-    (list :history (if eww-desktop-remove-duplicates
-                       (cl-remove-duplicates
-                        history :test #'eww-desktop-history-duplicate)
-                     history))))
+                         (cons eww-data eww-history)))
+        rval)
+    (list :history
+          (setq rval (if eww-desktop-remove-duplicates
+                         (cl-remove-duplicates
+                          history :test #'eww-desktop-history-duplicate)
+                       history))
+          :history-position
+          (cl-position
+           (elt history eww-history-position)
+           rval :test #'eww-desktop-history-duplicate))))

 (defun eww-restore-desktop (file-name buffer-name misc-data)
   "Restore an eww buffer from its desktop file record.
@@ -2772,7 +2778,8 @@ eww-restore-desktop
     (setq eww-history       (cdr (plist-get misc-data :history))
 	  eww-data      (or (car (plist-get misc-data :history))
 			    ;; backwards compatibility
-			    (list :url (plist-get misc-data :uri))))
+			    (list :url (plist-get misc-data :uri)))
+          eww-history-position (plist-get misc-data :history-position))
     (unless file-name
       (when (plist-get eww-data :url)
 	(cl-case eww-restore-desktop
@@ -2784,8 +2791,6 @@ eww-restore-desktop
     ;; .
     (current-buffer)))

-(add-to-list 'desktop-locals-to-save
-	     'eww-history-position)
 (add-to-list 'desktop-buffer-mode-handlers
              '(eww-mode . eww-restore-desktop))

--
2.40.1

bug#71450: [PATCH] Wrong eww-history-position after desktop restore if within history

[Jim Porter]

On 6/9/2024 5:23 AM, James Thomas via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> This is a patch that I think is simple enough to forgo the extensive
> testing which it hasn't been subjected to.

Thanks for the patch.

> +          :history-position
> +          (cl-position
> +           (elt history eww-history-position)
> +           rval :test #'eww-desktop-history-duplicate))))

Two questions here:

1. Is that the right test function? I'd have expected 'eq', since we 
want to find the position where our history index has moved to, right?

2. Should this part check for 'eww-desktop-remove-duplicates' too? If 
that option is nil, I think we could avoid the 'cl-position' call. Or 
maybe lift the 'eww-desktop-remove-duplicates' call outside of the 
'list' and just construct two totally different lists in the THEN/ELSE 
blocks.

bug#71450: [PATCH] Wrong eww-history-position after desktop restore if within history

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Jim Porter wrote:

> On 6/9/2024 5:23 AM, James Thomas via Bug reports for GNU Emacs, the
> Swiss army knife of text editors wrote:
>> This is a patch that I think is simple enough to forgo the extensive
>> testing which it hasn't been subjected to.
>
> Thanks for the patch.
>
>> +          :history-position
>> +          (cl-position
>> +           (elt history eww-history-position)
>> +           rval :test #'eww-desktop-history-duplicate))))
>
> Two questions here:
>
> 1. Is that the right test function? I'd have expected 'eq', since we
> want to find the position where our history index has moved to, right?

I'd thought that this would be more robust because it was used for the
original removal. But I guess 'eq' would be enough since only succeeding
duplicates are removed.

> 2. Should this part check for 'eww-desktop-remove-duplicates' too? If
> that option is nil, I think we could avoid the 'cl-position' call. Or
> maybe lift the 'eww-desktop-remove-duplicates' call outside of the
> 'list' and just construct two totally different lists in the THEN/ELSE
> blocks.

In fact, the following patch was the one with which I got it working
originally, before favouring the earlier one for simplicity:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Alternate patch --]
[-- Type: text/x-diff, Size: 1950 bytes --]

diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 977210e9cc8..98421828bb9 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2755,10 +2755,19 @@ eww-desktop-misc-data
 :dom, :source and :text properties."
   (let ((history (mapcar #'eww-desktop-data-1
                          (cons eww-data eww-history))))
-    (list :history (if eww-desktop-remove-duplicates
-                       (cl-remove-duplicates
-                        history :test #'eww-desktop-history-duplicate)
-                     history))))
+    (let ((posn eww-history-position) rval)
+      (list :history
+            (if eww-desktop-remove-duplicates
+                (prog1
+                    (setq
+                     rval (cl-remove-duplicates
+                           history :test #'eww-desktop-history-duplicate))
+                  (setq posn
+                        (cl-position
+                         (elt history eww-history-position)
+                         rval :test #'eww-desktop-history-duplicate)))
+              history)
+              :history-position posn))))

 (defun eww-restore-desktop (file-name buffer-name misc-data)
   "Restore an eww buffer from its desktop file record.
@@ -2772,7 +2781,8 @@ eww-restore-desktop
     (setq eww-history       (cdr (plist-get misc-data :history))
 	  eww-data      (or (car (plist-get misc-data :history))
 			    ;; backwards compatibility
-			    (list :url (plist-get misc-data :uri))))
+			    (list :url (plist-get misc-data :uri)))
+          eww-history-position (plist-get misc-data :history-position))
     (unless file-name
       (when (plist-get eww-data :url)
 	(cl-case eww-restore-desktop
@@ -2784,8 +2794,6 @@ eww-restore-desktop
     ;; .
     (current-buffer)))

-(add-to-list 'desktop-locals-to-save
-	     'eww-history-position)
 (add-to-list 'desktop-buffer-mode-handlers
              '(eww-mode . eww-restore-desktop))


[-- Attachment #3: Type: text/plain, Size: 16 bytes --]


Regards,
James

bug#71450: [PATCH] Wrong eww-history-position after desktop restore if within history

[James Thomas via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Jim Porter wrote:

> On 6/9/2024 5:23 AM, James Thomas via Bug reports for GNU Emacs, the
> Swiss army knife of text editors wrote:
>> This is a patch that I think is simple enough to forgo the extensive
>> testing which it hasn't been subjected to.
>
> Thanks for the patch.
>
>> +          :history-position
>> +          (cl-position
>> +           (elt history eww-history-position)
>> +           rval :test #'eww-desktop-history-duplicate))))
>
> Two questions here:
>
> 1. Is that the right test function? I'd have expected 'eq', since we
> want to find the position where our history index has moved to, right?
>
> 2. Should this part check for 'eww-desktop-remove-duplicates' too? If
> that option is nil, I think we could avoid the 'cl-position' call. Or
> maybe lift the 'eww-desktop-remove-duplicates' call outside of the
> 'list' and just construct two totally different lists in the THEN/ELSE
> blocks.

Here's an updated patch, which I've tested somewhat:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Account for duplicate removal on restoring --]
[-- Type: text/x-patch, Size: 2524 bytes --]

From bc0e3f2653e1c4c1f683d4b10ff139dbf963ee8d Mon Sep 17 00:00:00 2001
From: James Thomas <jimjoe@gmx.net>
Date: Tue, 11 Jun 2024 03:00:33 +0530
Subject: [PATCH] Account for duplicate removal on restoring
 eww-history-position.

* lisp/net/eww.el (eww-desktop-misc-data): Add :history-position
(eww-restore-desktop): Use it.
(desktop-locals-to-save): Remove the raw variable.
---
 lisp/net/eww.el | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 977210e9cc8..fd8f80065b1 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2754,11 +2754,20 @@ eww-desktop-misc-data
 Generally, the list should not include the (usually overly large)
 :dom, :source and :text properties."
   (let ((history (mapcar #'eww-desktop-data-1
-                         (cons eww-data eww-history))))
-    (list :history (if eww-desktop-remove-duplicates
-                       (cl-remove-duplicates
-                        history :test #'eww-desktop-history-duplicate)
-                     history))))
+                         (cons eww-data eww-history)))
+        (posn eww-history-position) rval)
+    (list :history
+          (if eww-desktop-remove-duplicates
+              (prog1
+                  (setq
+                   rval (cl-remove-duplicates
+                         history :test #'eww-desktop-history-duplicate))
+                (setq posn
+                      (cl-position
+                       (elt history eww-history-position)
+                       rval :test #'eq)))
+            history)
+          :history-position posn)))

 (defun eww-restore-desktop (file-name buffer-name misc-data)
   "Restore an eww buffer from its desktop file record.
@@ -2772,7 +2781,8 @@ eww-restore-desktop
     (setq eww-history       (cdr (plist-get misc-data :history))
 	  eww-data      (or (car (plist-get misc-data :history))
 			    ;; backwards compatibility
-			    (list :url (plist-get misc-data :uri))))
+			    (list :url (plist-get misc-data :uri)))
+          eww-history-position (plist-get misc-data :history-position))
     (unless file-name
       (when (plist-get eww-data :url)
 	(cl-case eww-restore-desktop
@@ -2784,8 +2794,6 @@ eww-restore-desktop
     ;; .
     (current-buffer)))

-(add-to-list 'desktop-locals-to-save
-	     'eww-history-position)
 (add-to-list 'desktop-buffer-mode-handlers
              '(eww-mode . eww-restore-desktop))

--
2.40.1


[-- Attachment #3: Type: text/plain, Size: 16 bytes --]


Regards,
James

bug#71450: [PATCH] Wrong eww-history-position after desktop restore if within history

[Jim Porter]

On 6/10/2024 2:36 PM, James Thomas via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> Here's an updated patch, which I've tested somewhat:

Thanks, this looks good to me, so I've merged it as 65b7f87a31d. Closing 
this bug now.