From c97b30da6b16a553365c1ecc4d9d4cdd11395df4 Mon Sep 17 00:00:00 2001 From: Jim Porter Date: Sat, 17 Feb 2024 20:49:15 -0800 Subject: [PATCH] When navigating through history in EWW, don't keep adding to 'eww-history' This resolves an issue where navigating back and then forward kept adding new history entries so you could never hit the "end" (bug#69232). * lisp/net/eww.el (eww-history-position): Add docstring. (eww-mode-map, eww-context-menu): Use correct predicates for when to enable back/forward. (eww-save-history): Save history entry in its original place when viewing a historical page. New argument CLEAR-FUTURE. (eww-setup-buffer, eww-follow-link, eww-readable): Clear future history. (eww-back-url): Set 'eww-history-position' based on the result of 'eww-save-history'. (eww-forward-url): Set 'eww-history-position' directly, since 'eww-save-history' no longer adds a new entry in this case. * test/lisp/net/eww-tests.el: New file. * etc/NEWS: Announce this change. --- etc/NEWS | 10 ++++ lisp/net/eww.el | 53 ++++++++++++------ test/lisp/net/eww-tests.el | 108 +++++++++++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+), 15 deletions(-) create mode 100644 test/lisp/net/eww-tests.el diff --git a/etc/NEWS b/etc/NEWS index 6d444daf152..7983c4be25b 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -1004,6 +1004,16 @@ When invoked with the prefix argument ('C-u'), This is useful for continuing reading the URL in the current buffer when the new URL is fetched. +--- +*** History navigation in EWW now works like other browsers. +Previously, when navigating back and forward through page history, EWW +would add a duplicate entry to the end of the history list each time. +This made it impossible to navigate to the "end" of the history list. +Now, navigating through history in EWW simply changes your position in +the history list, allowing you to reach the end as expected. In +addition, when navigating to a new page from a historical one, EWW +deletes the history entries after the current page. + ** go-ts-mode +++ diff --git a/lisp/net/eww.el b/lisp/net/eww.el index 5a25eef9e3c..904c778cb0a 100644 --- a/lisp/net/eww.el +++ b/lisp/net/eww.el @@ -312,7 +312,10 @@ eww-valid-certificate (defvar eww-data nil) (defvar eww-history nil) -(defvar eww-history-position 0) +(defvar eww-history-position 0 + "The 1-indexed position in `eww-history'. +If zero, EWW is at the newest page, which isn't yet present in +`eww-history'.") (defvar eww-prompt-history nil) (defvar eww-local-regex "localhost" @@ -958,7 +961,7 @@ eww-display-pdf (defun eww-setup-buffer () (when (or (plist-get eww-data :url) (plist-get eww-data :dom)) - (eww-save-history)) + (eww-save-history t)) (let ((inhibit-read-only t)) (remove-overlays) (erase-buffer)) @@ -1036,7 +1039,7 @@ eww-readable (libxml-parse-html-region (point-min) (point-max)))) (base (plist-get eww-data :url))) (eww-score-readability dom) - (eww-save-history) + (eww-save-history t) (eww-display-html nil nil (list 'base (list (cons 'href base)) (eww-highest-readability dom)) @@ -1129,9 +1132,9 @@ eww-mode-map ["Reload" eww-reload t] ["Follow URL in new buffer" eww-open-in-new-buffer] ["Back to previous page" eww-back-url - :active (not (zerop (length eww-history)))] + :active (< eww-history-position (length eww-history))] ["Forward to next page" eww-forward-url - :active (not (zerop eww-history-position))] + :active (> eww-history-position 1)] ["Browse with external browser" eww-browse-with-external-browser t] ["Download" eww-download t] ["View page source" eww-view-source] @@ -1155,9 +1158,9 @@ eww-context-menu (easy-menu-define nil easy-menu nil '("Eww" ["Back to previous page" eww-back-url - :visible (not (zerop (length eww-history)))] + :active (< eww-history-position (length eww-history))] ["Forward to next page" eww-forward-url - :visible (not (zerop eww-history-position))] + :active (> eww-history-position 1)] ["Reload" eww-reload t])) (dolist (item (reverse (lookup-key easy-menu [menu-bar eww]))) (when (consp item) @@ -1280,16 +1283,20 @@ eww-back-url (interactive nil eww-mode) (when (>= eww-history-position (length eww-history)) (user-error "No previous page")) - (eww-save-history) - (setq eww-history-position (+ eww-history-position 2)) + (if (eww-save-history) + ;; We were at the latest page (which was just added to the + ;; history), so go back two entries. + (setq eww-history-position 2) + (setq eww-history-position (1+ eww-history-position))) (eww-restore-history (elt eww-history (1- eww-history-position)))) (defun eww-forward-url () "Go to the next displayed page." (interactive nil eww-mode) - (when (zerop eww-history-position) + (when (<= eww-history-position 1) (user-error "No next page")) (eww-save-history) + (setq eww-history-position (1- eww-history-position)) (eww-restore-history (elt eww-history (1- eww-history-position)))) (defun eww-restore-history (elem) @@ -1958,7 +1965,7 @@ eww-follow-link ((and (setq target (url-target (url-generic-parse-url url))) (eww-same-page-p url (plist-get eww-data :url))) (let ((point (point))) - (eww-save-history) + (eww-save-history t) (plist-put eww-data :url url) (goto-char (point-min)) (if-let ((match (text-property-search-forward 'shr-target-id target #'member))) @@ -2288,12 +2295,28 @@ eww-bookmark-mode ;;; History code -(defun eww-save-history () +(defun eww-save-history (&optional clear-future) + "Save the current page's data to the history. +If the current page is a historial one loaded from +`eww-history' (e.g. by calling `eww-back-url'), this will update the +page's entry in `eww-history' and return nil. Otherwise, add a new +entry to `eww-history' and return t. + +If CLEAR-FUTURE is non-nil, clear any future history elements from +`eww-history'." (plist-put eww-data :point (point)) (plist-put eww-data :text (buffer-string)) - (let ((history-delete-duplicates nil)) - (add-to-history 'eww-history eww-data eww-history-limit t)) - (setq eww-data (list :title ""))) + (prog1 + (if (zerop eww-history-position) + (let ((history-delete-duplicates nil)) + (add-to-history 'eww-history eww-data eww-history-limit t) + t) + (setf (elt eww-history (1- eww-history-position)) eww-data) + (when (and clear-future (> eww-history-position 1)) + (setq eww-history (nthcdr (1- eww-history-position) eww-history) + eww-history-position 1)) + nil) + (setq eww-data (list :title "")))) (defvar eww-current-buffer) diff --git a/test/lisp/net/eww-tests.el b/test/lisp/net/eww-tests.el new file mode 100644 index 00000000000..9febaa33193 --- /dev/null +++ b/test/lisp/net/eww-tests.el @@ -0,0 +1,108 @@ +;;; eww-tests.el --- tests for eww.el -*- lexical-binding: t; -*- + +;; Copyright (C) 2024 Free Software Foundation, Inc. + +;; This file is part of GNU Emacs. + +;; GNU Emacs is free software: you can redistribute it and/or modify +;; it under the terms of the GNU General Public License as published by +;; the Free Software Foundation, either version 3 of the License, or +;; (at your option) any later version. + +;; GNU Emacs is distributed in the hope that it will be useful, +;; but WITHOUT ANY WARRANTY; without even the implied warranty of +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;; GNU General Public License for more details. + +;; You should have received a copy of the GNU General Public License +;; along with GNU Emacs. If not, see . + +;;; Commentary: + +;;; Code: + +(require 'ert) +(require 'eww) + +(defmacro eww-test--with-mock-retrieve (&rest body) + "Evaluate BODY with a mock implementation of `eww-retrieve'. +This avoids network requests during our tests. Additionally, prepare a +temporary EWW buffer for our tests." + (declare (indent 0)) + `(cl-letf (((symbol-function 'eww-retrieve) + (lambda (url callback args) + (with-temp-buffer + ;; Write out an empty list of headers and the URL + ;; as our body. + (insert "\n" url) + (apply callback nil args))))) + (with-temp-buffer + (eww-mode) + ,@body))) + +(defun eww-test--history-urls () + (mapcar (lambda (elem) (plist-get elem :url)) eww-history)) + +;;; Tests: + +(ert-deftest eww-test/history/new-page () + "Test that when visiting a new page, the previous one goes into the history." + (eww-test--with-mock-retrieve + (eww "one.invalid") + (eww "two.invalid") + (should (equal (eww-test--history-urls) + '("http://one.invalid/"))) + (eww "three.invalid") + (should (equal (eww-test--history-urls) + '("http://two.invalid/" + "http://one.invalid/"))))) + +(ert-deftest eww-test/history/back-forward () + "Test that navigating through history just changes our history position. +See bug#69232." + (eww-test--with-mock-retrieve + (eww "one.invalid") + (eww "two.invalid") + (eww "three.invalid") + (let ((url-history '("http://three.invalid/" + "http://two.invalid/" + "http://one.invalid/"))) + ;; Go back one page. This should add "three.invalid" to the + ;; history, making our position in the list 2. + (eww-back-url) + (should (equal (eww-test--history-urls) url-history)) + (should (= eww-history-position 2)) + ;; Go back again. + (eww-back-url) + (should (equal (eww-test--history-urls) url-history)) + (should (= eww-history-position 3)) + ;; At the beginning of the history, so trying to go back should + ;; signal an error. + (should-error (eww-back-url)) + ;; Go forward once. + (eww-forward-url) + (should (equal (eww-test--history-urls) url-history)) + (should (= eww-history-position 2)) + ;; Go forward again. + (eww-forward-url) + (should (equal (eww-test--history-urls) url-history)) + (should (= eww-history-position 1)) + ;; At the end of the history, so trying to go forward should + ;; signal an error. + (should-error (eww-forward-url))))) + +(ert-deftest eww-test/history/clear-future () + "Test that going to a new page from a historical one clears future history. +See bug#69232." + (eww-test--with-mock-retrieve + (eww "one.invalid") + (eww "two.invalid") + (eww "three.invalid") + (eww-back-url) + (eww "four.invalid") + (should (equal (eww-test--history-urls) '("http://two.invalid/" + "http://one.invalid/"))) + (should (= eww-history-position 0)))) + +(provide 'eww-tests) +;; eww-tests.el ends here -- 2.25.1