Re: [PATCH] Use vtable for eww-bookmarks

["Sebastián Monía" <sebastian@sebasmonia.com>]

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

Sebastián Monía <sebastian@sebasmonia.com> writes:
> On Sun, Dec 8, 2024, at 11:52 PM, Jim Porter wrote:
>> On 12/1/2024 2:12 PM, Sebastián Monía wrote:
>> > Generalizing this feature can be challenging in that different types of
>> > information could need very different handing.
>> 
>> I think in this case, I'll defer to you and Adam (and Eli) about the 
>> best path forward here. The existing behavior is a bit odd, and since I 
>> don't use EWW's bookmarks, I don't have a good sense of the right way to 
>> improve things here. Or in other words, I don't have any objections to 
>> the code, but I'm not confident that I know enough about this corner to 
>> do a properly thorough review.
>
> Will submit a patch ASAP to include a column "Order".

> And then we don't need a "remove sort" binding.

Hi all,
Attached a patch with "Order" and validation for this sort before
kill/yank.
As always, open to feedback.

@Adam, apparently reverting the vtable re-generates all objects, when
using a function (as is our case here). So we can't use
`vtable-goto-object' unless the change from eq to equal is applied
first. Sad indeed.


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

From 840e8039d5e4868c89413f60b08d26d65d8c3ce9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebasti=C3=A1n=20Mon=C3=ADa?= <code@sebasmonia.com>
Date: Sun, 1 Dec 2024 00:29:08 -0500
Subject: [PATCH] Use vtable in eww-list-bookmarks

---
 lisp/net/eww.el | 174 +++++++++++++++++++++++++++---------------------
 1 file changed, 98 insertions(+), 76 deletions(-)

diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 4d4d4d6beac..0a842907bae 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2359,6 +2359,7 @@ eww-add-bookmark
 	     (plist-get eww-data :title))))
 
 (defun eww-write-bookmarks ()
+  "Write the bookmarks in `eww-bookmarks-directory'."
   (with-temp-file (expand-file-name "eww-bookmarks" eww-bookmarks-directory)
     (insert ";; Auto-generated file; don't edit -*- mode: lisp-data -*-\n")
     (let ((print-length nil)
@@ -2378,115 +2379,136 @@ eww-read-bookmarks
       (user-error "No bookmarks are defined"))))
 
 ;;;###autoload
-(defun eww-list-bookmarks ()
-  "Display the bookmarks."
+(defun eww-list-bookmarks (&optional build-only)
+  "Display the eww bookmarks.
+Optional argument BUILD-ONLY, when non-nil, means to build the buffer
+without popping it."
   (interactive)
   (eww-read-bookmarks t)
-  (pop-to-buffer "*eww bookmarks*")
-  (eww-bookmark-prepare))
-
-(defun eww-bookmark-prepare ()
-  (set-buffer (get-buffer-create "*eww bookmarks*"))
-  (eww-bookmark-mode)
-  (let* ((width (/ (window-width) 2))
-	 (format (format "%%-%ds %%s" width))
-	 (inhibit-read-only t)
-	 start title)
+  (with-current-buffer (get-buffer-create "*eww bookmarks*")
+    (eww-bookmark-mode)
+    (eww--bookmark-prepare))
+  (unless build-only
+    (pop-to-buffer "*eww bookmarks*")))
+
+(defun eww--bookmark-prepare ()
+  "Display a table with the list of eww bookmarks.
+Will remove all buffer contents first."
+  (let ((inhibit-read-only t))
     (erase-buffer)
-    (setq header-line-format (concat " " (format format "Title" "URL")))
-    (dolist (bookmark eww-bookmarks)
-      (setq start (point)
-	    title (plist-get bookmark :title))
-      (when (> (length title) width)
-	(setq title (truncate-string-to-width title width)))
-      (insert (format format title (plist-get bookmark :url)) "\n")
-      (put-text-property start (1+ start) 'eww-bookmark bookmark))
-    (goto-char (point-min))))
+    (make-vtable
+     :columns '((:name "Order" :min-width 6)
+                (:name "Title" :min-width "25%" :max-width "50%")
+                (:name "URL"))
+     :objects-function #'eww--bookmark-format-data
+     ;; use fixed-font face
+     :face 'default
+     :sort-by '((0 . ascend))
+     )))
+
+(defun eww--bookmark-format-data ()
+  "Format `eww-bookmarks' for use in a vtable.
+The data is returned as a list (order title url bookmark), for use
+in of `eww-bookmark-mode'.  Order stars counting from 1."
+  (seq-map-indexed (lambda (bm index)
+                     (list
+                      (+ 1 index)
+                      (plist-get bm :title)
+                      (plist-get bm :url)
+                      bm))
+                   eww-bookmarks))
 
 (defvar eww-bookmark-kill-ring nil)
 
+(defun eww--bookmark-check-order-sort ()
+  "Signal a user error unless the bookmark vtable is sorted by asc order."
+  ;; vtables sort respecting the previous sort column. As long as
+  ;; "order" was last, the kill/yank operations will make sense, no
+  ;; matter what sort was used before.
+  (when-let* ((the-table (vtable-current-table))
+              (last-sort-pair (car (last (vtable-sort-by the-table)))))
+    (unless (and (= 0 (car last-sort-pair))
+                 (eq 'ascend (cdr last-sort-pair)))
+      (user-error
+       "Can't kill/yank bookmarks unless the table is sorted by ascending Order"))))
+
 (defun eww-bookmark-kill ()
   "Kill the current bookmark."
   (interactive nil eww-bookmark-mode)
-  (let* ((start (line-beginning-position))
-	 (bookmark (get-text-property start 'eww-bookmark))
-	 (inhibit-read-only t))
-    (unless bookmark
+  (eww--bookmark-check-order-sort)
+  (let ((bookmark-at-point (nth 3 (vtable-current-object)))
+	(position (point)))
+    (unless bookmark-at-point
       (user-error "No bookmark on the current line"))
     (forward-line 1)
-    (push (buffer-substring start (point)) eww-bookmark-kill-ring)
-    (delete-region start (point))
-    (setq eww-bookmarks (delq bookmark eww-bookmarks))
-    (eww-write-bookmarks)))
+    (push bookmark-at-point eww-bookmark-kill-ring)
+    (setq eww-bookmarks (delq bookmark-at-point eww-bookmarks))
+    (eww-write-bookmarks)
+    (vtable-revert-command)
+    (goto-char position)))
 
 (defun eww-bookmark-yank ()
   "Yank a previously killed bookmark to the current line."
   (interactive nil eww-bookmark-mode)
+  (eww--bookmark-check-order-sort)
   (unless eww-bookmark-kill-ring
     (user-error "No previously killed bookmark"))
-  (beginning-of-line)
-  (let ((inhibit-read-only t)
-	(start (point))
-	bookmark)
-    (insert (pop eww-bookmark-kill-ring))
-    (setq bookmark (get-text-property start 'eww-bookmark))
-    (if (= start (point-min))
-	(push bookmark eww-bookmarks)
-      (let ((line (count-lines start (point))))
-	(setcdr (nthcdr (1- line) eww-bookmarks)
-		(cons bookmark (nthcdr line eww-bookmarks)))))
-    (eww-write-bookmarks)))
+  (let* ((bookmark-at-point (nth 3 (vtable-current-object)))
+         (index-bap (seq-position eww-bookmarks bookmark-at-point))
+         (position (point)))
+    ;; TODO: a simpler way of doing this?
+    (setq eww-bookmarks (seq-concatenate
+                         'list
+                         (seq-subseq eww-bookmarks 0 index-bap)
+                         (list (pop eww-bookmark-kill-ring))
+                         (seq-subseq eww-bookmarks index-bap)))
+    (eww-write-bookmarks)
+    (vtable-revert-command)
+    (vtable-beginning-of-table)
+    (goto-char position)))
 
 (defun eww-bookmark-browse ()
   "Browse the bookmark under point in eww."
   (interactive nil eww-bookmark-mode)
-  (let ((bookmark (get-text-property (line-beginning-position) 'eww-bookmark)))
-    (unless bookmark
+  (let ((bookmark-at-point (nth 3 (vtable-current-object))))
+    (unless bookmark-at-point
       (user-error "No bookmark on the current line"))
     (quit-window)
-    (eww-browse-url (plist-get bookmark :url))))
+    (eww-browse-url (plist-get bookmark-at-point :url))))
 
 (defun eww-next-bookmark ()
   "Go to the next bookmark in the list."
   (interactive nil eww-bookmark-mode)
-  (let ((first nil)
-	bookmark)
+  (let (fresh-buffer target-bookmark)
     (unless (get-buffer "*eww bookmarks*")
-      (setq first t)
-      (eww-read-bookmarks t)
-      (eww-bookmark-prepare))
+      (setq fresh-buffer t)
+      (eww-list-bookmarks t))
     (with-current-buffer "*eww bookmarks*"
-      (when (and (not first)
-		 (not (eobp)))
-	(forward-line 1))
-      (setq bookmark (get-text-property (line-beginning-position)
-					'eww-bookmark))
-      (unless bookmark
-	(user-error "No next bookmark")))
-    (eww-browse-url (plist-get bookmark :url))))
+      (unless fresh-buffer
+        (forward-line 1))
+      (setq target-bookmark (nth 3 (vtable-current-object))))
+    (unless target-bookmark
+      ;; usually because we moved past end of the table
+      (user-error "No next bookmark"))
+    (eww-browse-url (plist-get target-bookmark :url))))
 
 (defun eww-previous-bookmark ()
   "Go to the previous bookmark in the list."
   (interactive nil eww-bookmark-mode)
-  (let ((first nil)
-	bookmark)
+  (let (fresh-buffer target-bookmark)
     (unless (get-buffer "*eww bookmarks*")
-      (setq first t)
-      (eww-read-bookmarks t)
-      (eww-bookmark-prepare))
+      (setq fresh-buffer t)
+      (eww-list-bookmarks t))
     (with-current-buffer "*eww bookmarks*"
-      (if first
-	  (goto-char (point-max))
-	(beginning-of-line))
-      ;; On the final line.
-      (when (eolp)
-	(forward-line -1))
-      (if (bobp)
-	  (user-error "No previous bookmark")
-	(forward-line -1))
-      (setq bookmark (get-text-property (line-beginning-position)
-					'eww-bookmark)))
-    (eww-browse-url (plist-get bookmark :url))))
+      (when fresh-buffer
+	(vtable-end-of-table))
+      ;; didn't move to a previous line, because we
+      ;; were already on the first one
+      (unless (= -1 (forward-line -1))
+        (setq target-bookmark (nth 3 (vtable-current-object)))))
+    (unless target-bookmark
+      (user-error "No previous bookmark"))
+    (eww-browse-url (plist-get target-bookmark :url))))
 
 (defun eww-bookmark-urls ()
   "Get the URLs from the current list of bookmarks."
@@ -2501,9 +2523,9 @@ eww-bookmark-mode-map
   :menu '("Eww Bookmark"
           ["Exit" quit-window t]
           ["Browse" eww-bookmark-browse
-           :active (get-text-property (line-beginning-position) 'eww-bookmark)]
+           :active (nth 3 (vtable-current-object))]
           ["Kill" eww-bookmark-kill
-           :active (get-text-property (line-beginning-position) 'eww-bookmark)]
+           :active (nth 3 (vtable-current-object))]
           ["Yank" eww-bookmark-yank
            :active eww-bookmark-kill-ring]))
 
-- 
2.47.0


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



-- 
Sebastián Monía
https://site.sebasmonia.com/