[PATCH] Use vtable for eww-bookmarks

[PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

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

Hi all,

The attached patch is a first attempt at bringing eww-bookmarks in line
with the previous switch of eww-buffer-list to vtable.

There are features of eww-bookmarks I had no idea about, like the
ability to re-arrange them by killing and yaking. Or navigating to the
next bookmark from an eww buffer or invoking the command via M-x.

Thank you,
Seb


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

From df509a792a58c5eedd1f516c406378315c8d95ca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebasti=C3=A1n=20Mon=C3=ADa?=
 <sebastian.monia@sebasmonia.com>
Date: Tue, 5 Nov 2024 17:48:56 -0500
Subject: [PATCH] Use vtable in eww-list-bookmarks

* lisp/net/eww.el (eww-list-bookmarks): Move logic to...:
(eww--bookmark-prepare, eww--bookmark-format-data )
... these,
and use 'vtable'.
(eww-bookmark-kill, eww-bookmark-yank, eww-bookmark-browse,
 eww-next-bookmark, eww-previous-bookmark,
 eww-buffers-mode-map): Use 'vtable-current-object'.
(eww-bookmark-undo-sort): New command to revert bookmark
 table sort
---
 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 2d351dff88f..10a7663efc5 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2373,115 +2373,124 @@ 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 "Title" :min-width "25%" :max-width "50%")
+                (:name "URL"))
+     :objects-function #'eww--bookmark-format-data
+     ;; use fixed-font face
+     :face 'default)))
+
+(defun eww--bookmark-format-data ()
+  "Format `eww-bookmarks' for use in a vtable.
+The data is returned as list of (title url bookmark) triplets, for use
+in of `eww-bookmark-mode'."
+  (mapcar (lambda (bm)
+            (list (plist-get bm :title)
+                  (plist-get bm :url)
+                  bm))
+          eww-bookmarks))
 
 (defvar eww-bookmark-kill-ring nil)
 
+(defun eww--bookmark-abort-if-sorted ()
+  "Signal a user error if the bookmark vtable at point has been sorted."
+  (when (and (vtable-current-table)
+             (vtable-sort-by (vtable-current-table)))
+    (user-error "Can't kill/yank bookmarks after the table has been sorted")))
+
 (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-abort-if-sorted)
+  (let ((bookmark-at-point (nth 2 (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-abort-if-sorted)
   (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 2 (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)
+    (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 2 (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 2 (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 2 (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."
@@ -2489,16 +2498,29 @@ eww-bookmark-urls
   (eww-read-bookmarks)
   (mapcar (lambda (x) (plist-get x :url)) eww-bookmarks))
 
+(defun eww-bookmark-undo-sort ()
+  "Remove any sorting by column of the bookmarks vtable.
+This is required before killing and yanking bookmarks to re-arrange
+them."
+  (interactive)
+  (let ((bookmark-at-point (vtable-current-object)))
+    (setf (vtable-sort-by (vtable-current-table)) nil)
+    (vtable-revert-command)
+    (while (not (equal (vtable-current-object)
+                       bookmark-at-point))
+      (forward-line 1))))
+
 (defvar-keymap eww-bookmark-mode-map
   "C-k" #'eww-bookmark-kill
   "C-y" #'eww-bookmark-yank
+  "u" #'eww-bookmark-undo-sort
   "RET" #'eww-bookmark-browse
   :menu '("Eww Bookmark"
           ["Exit" quit-window t]
           ["Browse" eww-bookmark-browse
-           :active (get-text-property (line-beginning-position) 'eww-bookmark)]
+           :active (nth 2 (vtable-current-object))]
           ["Kill" eww-bookmark-kill
-           :active (get-text-property (line-beginning-position) 'eww-bookmark)]
+           :active (nth 2 (vtable-current-object))]
           ["Yank" eww-bookmark-yank
            :active eww-bookmark-kill-ring]))
 
-- 
2.45.2.windows.1


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



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

Re: [PATCH] Use vtable for eww-bookmarks

[Eli Zaretskii]

> From: Sebastián Monía <sebastian@sebasmonia.com>
> Cc: Jim Porter <jporterbugs@gmail.com>
> Date: Tue, 05 Nov 2024 17:53:44 -0500
> 
> The attached patch is a first attempt at bringing eww-bookmarks in line
> with the previous switch of eww-buffer-list to vtable.
> 
> There are features of eww-bookmarks I had no idea about, like the
> ability to re-arrange them by killing and yaking. Or navigating to the
> next bookmark from an eww buffer or invoking the command via M-x.

I didn't try that, but if it changes the UI and UX, we need to think
hard whether we want to surprise users with such changes.  What's the
rationale and the motivation for this?

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

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

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Sebastián Monía <sebastian@sebasmonia.com>
>> Cc: Jim Porter <jporterbugs@gmail.com>
>> Date: Tue, 05 Nov 2024 17:53:44 -0500
>> 
>> The attached patch is a first attempt at bringing eww-bookmarks in line
>> with the previous switch of eww-buffer-list to vtable.
>> 
>> There are features of eww-bookmarks I had no idea about, like the
>> ability to re-arrange them by killing and yaking. Or navigating to the
>> next bookmark from an eww buffer or invoking the command via M-x.
>
> I didn't try that, but if it changes the UI and UX, we need to think
> hard whether we want to surprise users with such changes.  What's the
> rationale and the motivation for this?

We decided a few weeks ago that to support reverting, the existing
'eww-list-buffers' command could be implemented using a "proper table"
mode. The old code worked by inserting text and adding some properties.

After some conversation we picked vtable for this. The code was marged
recently.

During that work I noticed 'eww-list-bookmarks' also used this artisanal
approach to building the table, and suggested it would be nice to
convert it to vtable, and make it consistent with the new buffer list.
This is the patch that follows on that suggestion.

Regarding UI/UX changes, there isn't much of a difference on how the
table looks. The new one supports sorting though. Although I had to add
a binding to "undo sorting", to support an exising feature of
eww-bookmarks that lets you re-order the list.


[-- Attachment #2: old listing --]
[-- Type: image/png, Size: 26637 bytes --]

[-- Attachment #3: new listing --]
[-- Type: image/png, Size: 27267 bytes --]

[-- Attachment #4: new listing, sorted --]
[-- Type: image/png, Size: 29674 bytes --]

[-- Attachment #5: Type: text/plain, Size: 54 bytes --]


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

Re: [PATCH] Use vtable for eww-bookmarks

[Eli Zaretskii]

> From: Sebastián Monía <sebastian@sebasmonia.com>
> Cc: emacs-devel@gnu.org,  jporterbugs@gmail.com
> Date: Wed, 06 Nov 2024 08:36:00 -0500
> 
> > I didn't try that, but if it changes the UI and UX, we need to think
> > hard whether we want to surprise users with such changes.  What's the
> > rationale and the motivation for this?
> 
> We decided a few weeks ago that to support reverting, the existing
> 'eww-list-buffers' command could be implemented using a "proper table"
> mode. The old code worked by inserting text and adding some properties.
> 
> After some conversation we picked vtable for this. The code was marged
> recently.
> 
> During that work I noticed 'eww-list-bookmarks' also used this artisanal
> approach to building the table, and suggested it would be nice to
> convert it to vtable, and make it consistent with the new buffer list.
> This is the patch that follows on that suggestion.
> 
> Regarding UI/UX changes, there isn't much of a difference on how the
> table looks. The new one supports sorting though. Although I had to add
> a binding to "undo sorting", to support an exising feature of
> eww-bookmarks that lets you re-order the list.

Thanks, then I guess there should be no problems with these changes.

Re: [PATCH] Use vtable for eww-bookmarks

[Visuwesh]

[புதன் நவம்பர் 06, 2024] Sebastián Monía wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>>> From: Sebastián Monía <sebastian@sebasmonia.com>
>>> Cc: Jim Porter <jporterbugs@gmail.com>
>>> Date: Tue, 05 Nov 2024 17:53:44 -0500
>>> 
>>> The attached patch is a first attempt at bringing eww-bookmarks in line
>>> with the previous switch of eww-buffer-list to vtable.
>>> 
>>> There are features of eww-bookmarks I had no idea about, like the
>>> ability to re-arrange them by killing and yaking. Or navigating to the
>>> next bookmark from an eww buffer or invoking the command via M-x.
>>
>> I didn't try that, but if it changes the UI and UX, we need to think
>> hard whether we want to surprise users with such changes.  What's the
>> rationale and the motivation for this?
>
> We decided a few weeks ago that to support reverting, the existing
> 'eww-list-buffers' command could be implemented using a "proper table"
> mode. The old code worked by inserting text and adding some properties.
>
> After some conversation we picked vtable for this. The code was marged
> recently.
>
> During that work I noticed 'eww-list-bookmarks' also used this artisanal
> approach to building the table, and suggested it would be nice to
> convert it to vtable, and make it consistent with the new buffer list.
> This is the patch that follows on that suggestion.
>
> Regarding UI/UX changes, there isn't much of a difference on how the
> table looks. The new one supports sorting though. Although I had to add
> a binding to "undo sorting", to support an exising feature of
> eww-bookmarks that lets you re-order the list.

Can the -> bitmaps in the fringe be removed from the new listing?  It is
ugly (and quite misleading too).  Or is there a new column that is
hidden in the new listing that is not shown in the screenshot?

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

Visuwesh <visuweshm@gmail.com> writes:
> [புதன் நவம்பர் 06, 2024] Sebastián Monía wrote:
>
> Can the -> bitmaps in the fringe be removed from the new listing?  It is
> ugly (and quite misleading too).  Or is there a new column that is
> hidden in the new listing that is not shown in the screenshot?

Thanks for the feedback, will look into it.

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

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

Sebastián Monía <sebastian@sebasmonia.com> writes:
> Visuwesh <visuweshm@gmail.com> writes:
>> [புதன் நவம்பர் 06, 2024] Sebastián Monía wrote:
>>
>> Can the -> bitmaps in the fringe be removed from the new listing?  It is
>> ugly (and quite misleading too).  Or is there a new column that is
>> hidden in the new listing that is not shown in the screenshot?
>
> Thanks for the feedback, will look into it.

Turns out that below what's visible in the screenshot, some of the URLs
were wider than the windows & frame. When I maximize the frame, the
fringe disappears.

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

Re: [PATCH] Use vtable for eww-bookmarks

[Visuwesh]

[புதன் நவம்பர் 06, 2024] Sebastián Monía wrote:

> Sebastián Monía <sebastian@sebasmonia.com> writes:
>> Visuwesh <visuweshm@gmail.com> writes:
>>> [புதன் நவம்பர் 06, 2024] Sebastián Monía wrote:
>>>
>>> Can the -> bitmaps in the fringe be removed from the new listing?  It is
>>> ugly (and quite misleading too).  Or is there a new column that is
>>> hidden in the new listing that is not shown in the screenshot?
>>
>> Thanks for the feedback, will look into it.
>
> Turns out that below what's visible in the screenshot, some of the URLs
> were wider than the windows & frame. When I maximize the frame, the
> fringe disappears.

I see, thank you for the confirmation.

Re: [PATCH] Use vtable for eww-bookmarks

[Visuwesh]

[புதன் நவம்பர் 06, 2024] Sebastián Monía wrote:

> Sebastián Monía <sebastian@sebasmonia.com> writes:
>> Visuwesh <visuweshm@gmail.com> writes:
>>> [புதன் நவம்பர் 06, 2024] Sebastián Monía wrote:
>>>
>>> Can the -> bitmaps in the fringe be removed from the new listing?  It is
>>> ugly (and quite misleading too).  Or is there a new column that is
>>> hidden in the new listing that is not shown in the screenshot?
>>
>> Thanks for the feedback, will look into it.
>
> Turns out that below what's visible in the screenshot, some of the URLs
> were wider than the windows & frame. When I maximize the frame, the
> fringe disappears.

I see, thank you for the confirmation.

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

Sebastián Monía <sebastian@sebasmonia.com> writes:
> Visuwesh <visuweshm@gmail.com> writes:
>> [புதன் நவம்பர் 06, 2024] Sebastián Monía wrote:
>>
>> Can the -> bitmaps in the fringe be removed from the new listing?  It is
>> ugly (and quite misleading too).  Or is there a new column that is
>> hidden in the new listing that is not shown in the screenshot?
>
> Thanks for the feedback, will look into it.

I realized today that Jim P. replied to these emails and I replied back
without including the rest of the addresses (again, ooops).

I said "the reason for the fringe bitmaps was that some table elements
were larger than the window width. That's because I made the frame small
so the screenshot wasn't too big. And also I didn't capture the whole
list, just the top portion."

And Jim's reply:

> Right. However, the fringe bitmap is present even on rows that would
> fit entirely in the window. That's because rows whose last cell is
> short have trailing whitespace to fill them out to the max width of
> that column. I don't think that trailing whitespace is necessary (at
> least not how EWW is using vtable), and if we removed it, you'd only
> see the fringe bitmap for individual rows that *don't* fit entirely in
> the window.
> 
> - Jim

I think we could tackle this vtable issue separate from the eww-bookmark
changes. But open to your suggestions. All of you, now that I included
everyone 🙃

Thanks,
Seb

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

Re: [PATCH] Use vtable for eww-bookmarks

[Jim Porter]

On 11/6/2024 6:43 AM, Visuwesh wrote:
> Can the -> bitmaps in the fringe be removed from the new listing?  It is
> ugly (and quite misleading too).  Or is there a new column that is
> hidden in the new listing that is not shown in the screenshot?

Looking at this, I believe it might be a bug in the vtable code. For 
every (left-aligned) cell, vtable adds space to the end so it's the same 
width as the largest cell in that column. That's important for the 
*non*-last columns of course, but I can't figure out why we need that 
for the last column. Does anyone know? Could we just remove that space? 
I think it'd be less misleading without it.

Re: [PATCH] Use vtable for eww-bookmarks

[Jim Porter]

On 11/5/2024 2:53 PM, Sebastián Monía wrote:
> Hi all,
> 
> The attached patch is a first attempt at bringing eww-bookmarks in line
> with the previous switch of eww-buffer-list to vtable.
> 
> There are features of eww-bookmarks I had no idea about, like the
> ability to re-arrange them by killing and yaking. Or navigating to the
> next bookmark from an eww buffer or invoking the command via M-x.

I think this looks good overall. Just one thought: instead of requiring 
users to manually undo sorting when killing/yanking, what if we had 
'eww-bookmark-kill' and 'eww-yank-bookmark' prompt the user to ask if 
they want to undo the sorting first? Something like this (untested):

   (defun eww-bookmark-kill (&optional interactive)
     (interactive "p")
     (when (and interactive
                (vtable-current-table)
                (vtable-sort-by (vtable-current-table)))
       (if (y-or-n-p "Reset sorting of bookmarks?")
           (eww-bookmark-undo-sort)
         (user-error "Can't kill sorted bookmarks")))
     ;; ...
     )

Then we might not even need a keybinding for 'eww-bookmark-sort'.

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

On Sat, Nov 23, 2024, at 2:12 PM, Jim Porter wrote:
> I think this looks good overall. Just one thought: instead of requiring 
> users to manually undo sorting when killing/yanking, what if we had 
> 'eww-bookmark-kill' and 'eww-yank-bookmark' prompt the user to ask if 
> they want to undo the sorting first? 

The problem is, when yanking, do you yank the bookmark to the
current position on the table? or the one where you will end up after
undoing the sort?

That (and a few other cases like it) made me opt for having the user
disable sorting explicitly.
Or even considering just not sorting at all, although it seems like a
useful feature for all cases but the kill/yank scenario.

Re: [PATCH] Use vtable for eww-bookmarks

[Jim Porter]

On 11/27/2024 12:02 PM, Sebastián Monía wrote:
> On Sat, Nov 23, 2024, at 2:12 PM, Jim Porter wrote:
>> I think this looks good overall. Just one thought: instead of requiring
>> users to manually undo sorting when killing/yanking, what if we had
>> 'eww-bookmark-kill' and 'eww-yank-bookmark' prompt the user to ask if
>> they want to undo the sorting first?
> 
> The problem is, when yanking, do you yank the bookmark to the
> current position on the table? or the one where you will end up after
> undoing the sort?

Good point. For what it's worth, I tried reordering bookmarks in the 
"Library" window in Firefox, and it works as in your patch: when sorting 
is active, you can't reorder the bookmarks. Firefox just shows a "No" 
icon and doesn't let you drag-and-drop.

Given that, let's go with your implementation, though I do have one last 
concern with the "undo sort": the keybinding ("u") is the same as what 
other table-like modes (e.g. dired, list-packages) use for "unmark". 
Currently we don't support marking/unmarking in the bookmarks menu, but 
that seems like a useful thing we might add one day: you could mark 
several bookmarks in order to open them all, delete them all, add some 
kind of tag to them, etc.

I'm not sure what a better key would be though. Maybe "C" for "clear sort"?

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

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


Jim Porter <jporterbugs@gmail.com> writes:
> Given that, let's go with your implementation, though I do have one
> last concern with the "undo sort": the keybinding ("u") is the same as
> what other table-like modes (e.g. dired, list-packages) use for
> "unmark". Currently we don't support marking/unmarking in the
> bookmarks menu, but that seems like a useful thing we might add one
> day: you could mark several bookmarks in order to open them all,
> delete them all, add some kind of tag to them, etc.

This is a good idea.

> I'm not sure what a better key would be though. Maybe "C" for "clear sort"?

Attached a patch that uses "c" ("c", not "C") and renames the function
to match.
Let me know what you think!

Thanks,
Seb


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

From 066a9f856a5d2796a99c1418c7eeefc633ced3ac 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..20c7892e599 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2378,115 +2378,124 @@ 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 "Title" :min-width "25%" :max-width "50%")
+                (:name "URL"))
+     :objects-function #'eww--bookmark-format-data
+     ;; use fixed-font face
+     :face 'default)))
+
+(defun eww--bookmark-format-data ()
+  "Format `eww-bookmarks' for use in a vtable.
+The data is returned as list of (title url bookmark) triplets, for use
+in of `eww-bookmark-mode'."
+  (mapcar (lambda (bm)
+            (list (plist-get bm :title)
+                  (plist-get bm :url)
+                  bm))
+          eww-bookmarks))
 
 (defvar eww-bookmark-kill-ring nil)
 
+(defun eww--bookmark-abort-if-sorted ()
+  "Signal a user error if the bookmark vtable at point has been sorted."
+  (when (and (vtable-current-table)
+             (vtable-sort-by (vtable-current-table)))
+    (user-error "Can't kill/yank bookmarks after the table has been sorted")))
+
 (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-abort-if-sorted)
+  (let ((bookmark-at-point (nth 2 (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-abort-if-sorted)
   (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 2 (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)
+    (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 2 (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 2 (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 2 (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."
@@ -2494,16 +2503,29 @@ eww-bookmark-urls
   (eww-read-bookmarks)
   (mapcar (lambda (x) (plist-get x :url)) eww-bookmarks))
 
+(defun eww-bookmark-clear-sort ()
+  "Clear any sorting by column of the bookmarks vtable.
+This is required before killing and yanking bookmarks to re-arrange
+them."
+  (interactive)
+  (let ((bookmark-at-point (vtable-current-object)))
+    (setf (vtable-sort-by (vtable-current-table)) nil)
+    (vtable-revert-command)
+    (while (not (equal (vtable-current-object)
+                       bookmark-at-point))
+      (forward-line 1))))
+
 (defvar-keymap eww-bookmark-mode-map
   "C-k" #'eww-bookmark-kill
   "C-y" #'eww-bookmark-yank
+  "c" #'eww-bookmark-clear-sort
   "RET" #'eww-bookmark-browse
   :menu '("Eww Bookmark"
           ["Exit" quit-window t]
           ["Browse" eww-bookmark-browse
-           :active (get-text-property (line-beginning-position) 'eww-bookmark)]
+           :active (nth 2 (vtable-current-object))]
           ["Kill" eww-bookmark-kill
-           :active (get-text-property (line-beginning-position) 'eww-bookmark)]
+           :active (nth 2 (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/

Re: [PATCH] Use vtable for eww-bookmarks

[Adam Porter]

> Jim Porter <jporterbugs@gmail.com> writes:
>> Given that, let's go with your implementation, though I do have one
>> last concern with the "undo sort": the keybinding ("u") is the same as
>> what other table-like modes (e.g. dired, list-packages) use for
>> "unmark". Currently we don't support marking/unmarking in the
>> bookmarks menu, but that seems like a useful thing we might add one
>> day: you could mark several bookmarks in order to open them all,
>> delete them all, add some kind of tag to them, etc.
> 
> This is a good idea.
> 
>> I'm not sure what a better key would be though. Maybe "C" for "clear sort"?
> 
> Attached a patch that uses "c" ("c", not "C") and renames the function
> to match.
> Let me know what you think!

Given how many similar modes and buffers that are in Emacs (i.e. where 
some kind of objects are displayed in a list with metadata), I think it 
would be best if any new such feature followed existing paradigms as 
closely as possible.  To have each vtable-like view (whether implemented 
with vtable or tabulated-list or anything else) have slightly different 
concepts and bindings is not helpful to the user.

So I'd suggest a reconsideration of the concept of sorting: There is no 
such thing as "clearing" the sorting, because the entries are always 
sorted by something, even if that's just the order in which the 
bookmarks are present in the list (which is probably the order in which 
they were created, or the inverse).

As an example, consider Dired: there is no "clearing" of sorting options 
in Dired, but there is dired-sort-toggle-or-edit: it toggles between 
sorting by date and sorting by the default (i.e. alphabetically).

In this case, the vtable-map already binds 
vtable-sort-by-current-column.  So it seems like what we need is a 
column by which to sort entries by default.  That would seem to leave us 
with two options:

   a) Sort by bookmark name by default

   b) Add a column for the bookmarks' place in the list, and sort by 
that by default.

Then the user could simply sort by one column or the other, without 
needing to add the additional concept of "clearing" the sorting (a 
concept that's not generally present in such views in other GUIs, 
anyway--usually a table view is always sorted by one column or another).

And with that, the existing binding of vtable-sort-by-current-column 
would suffice, eliminating the need for another sort-related binding.

What do you think?  :)

BTW, instead of doing:

+    (while (not (equal (vtable-current-object)
+                       bookmark-at-point))
+      (forward-line 1))))

Could you use vtable-goto-object?  I realize that it currently uses EQ 
for comparison, but 1) we've talked about changing that, and 2) ISTM 
that EQ should work for this case anyway, because entries in the 
bookmark list are being compared, and they shouldn't be getting copied. 
Am I missing something?

Thanks,
Adam

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

Adam Porter <adam@alphapapa.net> writes:
>> Jim Porter <jporterbugs@gmail.com> writes:
>>> I'm not sure what a better key would be though. Maybe "C" for "clear sort"?
>> Attached a patch that uses "c" ("c", not "C") and renames the
>> function
>> to match.
>> Let me know what you think!
> Given how many similar modes and buffers that are in Emacs (i.e. where
> some kind of objects are displayed in a list with metadata), I think
> it would be best if any new such feature followed existing paradigms
> as closely as possible.  To have each vtable-like view (whether
> implemented with vtable or tabulated-list or anything else) have
> slightly different concepts and bindings is not helpful to the user.
There is already some slight differences here and there and it is
sometimes jarring, so, great point.
For example Gnus uses # for marking, instead of m like vc-dir/dired,
sometimes it trips me.

> In this case, the vtable-map already binds
> vtable-sort-by-current-column.  So it seems like what we need is a
> column by which to sort entries by default.  That would seem to leave
> us with two options:
>
>   a) Sort by bookmark name by default
>
>   b) Add a column for the bookmarks' place in the list, and sort by
>   that by default.

The reason the bookmarks order matters, and this killing and yanking
feature exists, is that EWW binds M-n and M-p to navigate to the
next/prev bookmark. I doubt it is a popular feature (only found out
about it when writing this patch...) but I wouldn't remove it. So
defaulting to sort by name would be off the table, as it breaks the
whole thing.

Adding a column "Navigation order", or "rank" (both terrible names, open
to suggestions) seems like a great idea. 

> BTW, instead of doing:
>
> +    (while (not (equal (vtable-current-object)
> +                       bookmark-at-point))
> +      (forward-line 1))))
>
> Could you use vtable-goto-object?  I realize that it currently uses EQ
> for comparison, but 1) we've talked about changing that, and 2) ISTM
> that EQ should work for this case anyway, because entries in the
> bookmark list are being compared, and they shouldn't be getting
> copied. Am I missing something?

You are not, that code is a 1:1 conversion of the previous code. Next
patch, which may include the new column, will make better use of
vtable-goto-object when applicable. Thank you!


Regards,
Seb

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

Re: [PATCH] Use vtable for eww-bookmarks

[Jim Porter]

On 11/30/2024 10:39 PM, Adam Porter wrote:
> So I'd suggest a reconsideration of the concept of sorting: There is no 
> such thing as "clearing" the sorting, because the entries are always 
> sorted by something, even if that's just the order in which the 
> bookmarks are present in the list (which is probably the order in which 
> they were created, or the inverse).

This is probably just a difference of semantics, but I see the default 
ordering as a non-sorted *ordering*; non-sorted because none of the 
cells' data contributes to the ordering. The sorted views then apply a 
sort function to *produce* an ordering. So in my mind you can clear the 
sorting to reveal the "natural ordering" of the list.

> In this case, the vtable-map already binds 
> vtable-sort-by-current-column.  So it seems like what we need is a 
> column by which to sort entries by default.  That would seem to leave us 
> with two options:
> 
>    a) Sort by bookmark name by default
> 
>    b) Add a column for the bookmarks' place in the list, and sort by 
> that by default.

I agree that it makes sense to have some common way of handling this so 
that we don't proliferate many different implementations across Emacs. 
If we want to let users arbitrarily reorder the entries, then (a) won't 
work. However, I'm not sure I like (b). Except for providing a command 
to restore the natural ordering, the list-position column just seems 
like visual noise.

Maybe vtable would benefit from some special handling here? We could 
even go a step further and make a 'reorderable-vtable' (feel free to 
suggest a better name/interface) that includes the kill/yank commands 
from the EWW bookmarks code. Then we can also avoid proliferating many 
variations on how a user might reorder the data in a table.

That said, if everyone else prefers adding a list-position column, I 
won't fight too hard. I'm not the maintainer for EWW, just someone who's 
pushed a few commits to it recently.

> Then the user could simply sort by one column or the other, without 
> needing to add the additional concept of "clearing" the sorting (a 
> concept that's not generally present in such views in other GUIs, 
> anyway--usually a table view is always sorted by one column or another).

For what it's worth, the columns in the bookmarks Library in Firefox are 
tri-state buttons: sort ascending, sort descending, and no sort. You can 
only reorder the bookmarks if all the columns have the "no sort" state.

Columns in Emacs table modes *can* have any of these three states, but 
it's not easy to restore the initial ordering.

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

Jim Porter <jporterbugs@gmail.com> writes:
> Maybe vtable would benefit from some special handling here? We could
> even go a step further and make a 'reorderable-vtable' (feel free to
> suggest a better name/interface) that includes the kill/yank commands
> from the EWW bookmarks code. Then we can also avoid proliferating many
> variations on how a user might reorder the data in a table.

For the record, a few features in EWW Bookmarks are strange.
The kill/yank thing works more as a stack, than how you would expect a
kill ring to work (maybe it just needs a different name).
Also you can kill a number of items and never yank them, then they are
gone. In that aspect it does work as killing I guess.

Generalizing this feature can be challenging in that different types of
information could need very different handing.

Regards,
Seb

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

Re: [PATCH] Use vtable for eww-bookmarks

[Jim Porter]

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.

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

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" ("navigation order" 
is just too long, and I don't think users would associate it with M-n 
and M-p "navigation").
And then we don't need a "remove sort" binding.

Thank you everyone for your feedback!

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

[-- 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/

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

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

Sebastián Monía <sebastian@sebasmonia.com> writes:

> Sebastián Monía <sebastian@sebasmonia.com> writes:
> Hi all,
> Attached a patch with "Order" and validation for this sort before
> kill/yank.
> As always, open to feedback.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: eww-bookmarks --]
[-- Type: text/x-patch, Size: 9444 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 (eww-list-bookmarks): Move logic to...:
(eww--bookmark-prepare, eww--bookmark-format-data )
... these,
and use 'vtable'.
(eww-bookmark-kill, eww-bookmark-yank, eww-bookmark-browse,
 eww-next-bookmark, eww-previous-bookmark,
 eww-buffers-mode-map): Use 'vtable-current-object'.
---
 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: 103 bytes --]


Updated the patch to include the changelog :)

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

Re: [PATCH] Use vtable for eww-bookmarks

[Adam Porter]

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

Sorry, I've been having weird software problems on my system that have made
my usual workflows difficult, so I haven't been able to review and process
mail very well. Should have them resolved within a few days.

It sounds like this patch has proceeded well.

About regenerating the elements and not being able to use EQ: is that
because the original bookmark objects are not being used as-is?  (Sorry, I
can't look at the code at the moment.)  If so, could we address that?

Thanks.

On Thu, Dec 12, 2024, 09:25 Sebastián Monía <sebastian@sebasmonia.com>
wrote:

> Sebastián Monía <sebastian@sebasmonia.com> writes:
>
> > Sebastián Monía <sebastian@sebasmonia.com> writes:
> > Hi all,
> > Attached a patch with "Order" and validation for this sort before
> > kill/yank.
> > As always, open to feedback.
>
>
> Updated the patch to include the changelog :)
>
> --
> Sebastián Monía
> https://site.sebasmonia.com/
>

[-- Attachment #2: Type: text/html, Size: 1692 bytes --]

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

Adam Porter <adam@alphapapa.net> writes:
>  About regenerating the elements and not being able to use EQ: is that
>  because the original bookmark objects are not being used as-is?

Correct.

>  If so, could we address that?

We could add the order in the file format. Or number the items when we
read them. Then use the object including the number to track it all.
But when change the "order" number associated with a row, then it won't
be eq/equal anymore, anyway.

Maybe better: vtable provides a mechanism to have a printer function
called on each row. Then the objects stay the same, in the `eq' sense,
and we can keep the counter for order outside the row, as part of the
printer/"formatter" (in vtable parlance).

To consider: then we would call a function for each printed row vs
calling it once to generate the list in one go. Plus the code to update
the items order number when removing and inserting each one.

Of course the real answer on which one is "faster" or "leaner" needs a
benchmark. For 90% of cases the amount of bookmarks won't be more than a
couple dozen, so I don't think it will matter.

I would leave it as-is on the grounds of (add less code + the
speed/memory difference is small)(1). But as usual, if there's
consensus that we rather do the outlined above instead, I'll do it.


(1) like I said, no benchmark, so "_I assume_ the difference is small"

>  On Thu, Dec 12, 2024, 09:25 Sebastián Monía <sebastian@sebasmonia.com> wrote:
>
>  Sebastián Monía <sebastian@sebasmonia.com> writes:
>
>  > Sebastián Monía <sebastian@sebasmonia.com> writes:
>  > Hi all,
>  > Attached a patch with "Order" and validation for this sort before
>  > kill/yank.
>  > As always, open to feedback.
>
>  Updated the patch to include the changelog :)
>
>  -- 
>  Sebastián Monía
>  https://site.sebasmonia.com/
>

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

Re: [PATCH] Use vtable for eww-bookmarks

[Eli Zaretskii]

Ping!  Can we please make some progress here?

> From: Sebastián Monía <sebastian@sebasmonia.com>
> Cc: Jim Porter <jporterbugs@gmail.com>,  emacs-devel <emacs-devel@gnu.org>,
>   Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 12 Dec 2024 14:37:23 -0500
> 
> Adam Porter <adam@alphapapa.net> writes:
> >  About regenerating the elements and not being able to use EQ: is that
> >  because the original bookmark objects are not being used as-is?
> 
> Correct.
> 
> >  If so, could we address that?
> 
> We could add the order in the file format. Or number the items when we
> read them. Then use the object including the number to track it all.
> But when change the "order" number associated with a row, then it won't
> be eq/equal anymore, anyway.
> 
> Maybe better: vtable provides a mechanism to have a printer function
> called on each row. Then the objects stay the same, in the `eq' sense,
> and we can keep the counter for order outside the row, as part of the
> printer/"formatter" (in vtable parlance).
> 
> To consider: then we would call a function for each printed row vs
> calling it once to generate the list in one go. Plus the code to update
> the items order number when removing and inserting each one.
> 
> Of course the real answer on which one is "faster" or "leaner" needs a
> benchmark. For 90% of cases the amount of bookmarks won't be more than a
> couple dozen, so I don't think it will matter.
> 
> I would leave it as-is on the grounds of (add less code + the
> speed/memory difference is small)(1). But as usual, if there's
> consensus that we rather do the outlined above instead, I'll do it.
> 
> 
> (1) like I said, no benchmark, so "_I assume_ the difference is small"
> 
> >  On Thu, Dec 12, 2024, 09:25 Sebastián Monía <sebastian@sebasmonia.com> wrote:
> >
> >  Sebastián Monía <sebastian@sebasmonia.com> writes:
> >
> >  > Sebastián Monía <sebastian@sebasmonia.com> writes:
> >  > Hi all,
> >  > Attached a patch with "Order" and validation for this sort before
> >  > kill/yank.
> >  > As always, open to feedback.
> >
> >  Updated the patch to include the changelog :)
> >
> >  -- 
> >  Sebastián Monía
> >  https://site.sebasmonia.com/
> >
> 
> -- 
> Sebastián Monía
> https://site.sebasmonia.com/
> 

Re: [PATCH] Use vtable for eww-bookmarks

[Jim Porter]

On 12/28/2024 3:04 AM, Eli Zaretskii wrote:
> Ping!  Can we please make some progress here?

In the interest of making progress on this, does anyone have any 
*objections* to this patch? How about we wait a few days for anyone with 
objections to make them known, and if not, we just merge the current patch?

Once we do that, we can announce the change in a new thread on this list 
just to give people another chance to raise any objections.

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

Jim Porter <jporterbugs@gmail.com> writes:

> On 12/28/2024 3:04 AM, Eli Zaretskii wrote:
>> Ping!  Can we please make some progress here?
>
> In the interest of making progress on this, does anyone have any
> *objections* to this patch? How about we wait a few days for anyone
> with objections to make them known, and if not, we just merge the
> current patch?
>
> Once we do that, we can announce the change in a new thread on this
> list just to give people another chance to raise any objections.
Hi all,

I am open to comments, suggestions, as mentioned in my last reply. 

I still don't think it is worth it to re-use the bookmark list, but if
there's consensus to change, I will do it.

Thanks everyone,
Seb

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

Re: [PATCH] Use vtable for eww-bookmarks

[Adam Porter]

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

FWIW, I'd recommend using the vtable printer function, as described
earlier, so the real records/values can be used and compared as-is.
(Apologies if I was unclear earlier, as I thought that was what was already
being done.)  But I don't have time to contribute code to this patch now,
so that's just my recommendation.

Also FWIW, I think the long-term goal ought to be to replace EWW's bespoke
bookmarks with Emacs bookmarks (i.e. any Emacs bookmark to an HTTP(S) URL
ought to be usable by EWW, and it ought to create such bookmarks; EWW
should be just another way of opening them).  So it would probably be best
if the work done could be designed with an eye towards reuse (i.e. using a
printer function would mean that the underlying data type could be swapped
out later, so this could be just a view of Emacs bookmarks that point to
HTTP URLs).

My two cents. I won't try to hold up any work or merging for it.

Thanks,

Adam

On Mon, Dec 30, 2024, 23:43 Sebastián Monía <sebastian@sebasmonia.com>
wrote:

> Jim Porter <jporterbugs@gmail.com> writes:
>
> > On 12/28/2024 3:04 AM, Eli Zaretskii wrote:
> >> Ping!  Can we please make some progress here?
> >
> > In the interest of making progress on this, does anyone have any
> > *objections* to this patch? How about we wait a few days for anyone
> > with objections to make them known, and if not, we just merge the
> > current patch?
> >
> > Once we do that, we can announce the change in a new thread on this
> > list just to give people another chance to raise any objections.
> Hi all,
>
> I am open to comments, suggestions, as mentioned in my last reply.
>
> I still don't think it is worth it to re-use the bookmark list, but if
> there's consensus to change, I will do it.
>
> Thanks everyone,
> Seb
>
> --
> Sebastián Monía
> https://site.sebasmonia.com/
>

[-- Attachment #2: Type: text/html, Size: 2506 bytes --]

RE: [External] : Re: [PATCH] Use vtable for eww-bookmarks

[Drew Adams]

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

(Caveat: Not following this thread.)

+1 to using Emacs bookmarks instead of EWW bespoke "bookmarks".
(And Org "bookmarks"? Or has that already happened?)

FWIW:

Bookmark+ has a particular kind of bookmark for EWW pages.


  *   You can use command `bmkp-convert-eww-bookmarks' to create normal bookmarks from a file of EWW "bookmarks".

  *   An EWW bookmark records the web page title, buffer name (see above), and URL.
  *   There's an option for automatic EWW buffer renaming: (1) don't, (2) rename with page title only, (3) rename with page title + last 20 chars of URL. Buffer name always begins with prefix "*eww*-".
  *   There's an option for whether to generate a new buffer or reuse an existing buffer when bookmark jumping.
  *   There's a mode for automatically setting a bookmark when you visit a URL. An option says whether to create a bookmark or just update an existing bookmark to that URL. (Default is update-only.)
  *   For sorting, two EWW bookmarks are compared alphabetically by URL.
  *   All of the Bookmark+ features that act on a given type of bookmark work on EWW bookmarks: jumping, cycling, sorting, hide/show...

Besides EWW bookmarks, Bookmark+ has bookmark types for `browse-URL' and W3M.

(HTH somehow.)

https://www.emacswiki.org/emacs/BookmarkPlus

From: Adam Porter Sent: Tuesday, December 31, 2024 10:29 AM

...
Also FWIW, I think the long-term goal ought to be to replace EWW's bespoke bookmarks with Emacs bookmarks (i.e. any Emacs bookmark to an HTTP(S) URL ought to be usable by EWW, and it ought to create such bookmarks; EWW should be just another way of opening them).  So it would probably be best if the work done could be designed with an eye towards reuse (i.e. using a printer function would mean that the underlying data type could be swapped out later, so this could be just a view of Emacs bookmarks that point to HTTP URLs).

[-- Attachment #2: Type: text/html, Size: 11465 bytes --]

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

Adam Porter <adam@alphapapa.net> writes:
>  Also FWIW, I think the long-term goal ought to be to replace EWW's
>  bespoke bookmarks with Emacs bookmarks (i.e. any Emacs bookmark to an
>  HTTP(S) URL ought to be usable by EWW, and it ought to create such
>  bookmarks; EWW should be just another way of opening them).

You can create Emacs bookmarks or EWW bookmarks. I use one or the other
for different cases. But that's more a peculiarity of how I work, than
proof that we need both, being honest.

We could deprecate EWW-only bookmarks, I guess. If we plan to do that,
merging this patch makes no sense.
And then also drop the feature to re-arrange bookmarks, or make it work
in the context of standard bookmarks. Ditto for M-n and M-p to navigate
next/prev bookmark in EWW.

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

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

Hello everyone!

I am ok to retire the patch, with no consensus on how to progress.

Regards,
Seb

On Wed, Jan 1, 2025, at 11:25 PM, Sebastián Monía wrote:
> 
> Adam Porter <adam@alphapapa.net> writes:
> >  Also FWIW, I think the long-term goal ought to be to replace EWW's
> >  bespoke bookmarks with Emacs bookmarks (i.e. any Emacs bookmark to an
> >  HTTP(S) URL ought to be usable by EWW, and it ought to create such
> >  bookmarks; EWW should be just another way of opening them).
> 
> You can create Emacs bookmarks or EWW bookmarks. I use one or the other
> for different cases. But that's more a peculiarity of how I work, than
> proof that we need both, being honest.
> 
> We could deprecate EWW-only bookmarks, I guess. If we plan to do that,
> merging this patch makes no sense.
> And then also drop the feature to re-arrange bookmarks, or make it work
> in the context of standard bookmarks. Ditto for M-n and M-p to navigate
> next/prev bookmark in EWW.
> 
> -- 
> Sebastián Monía
> https://site.sebasmonia.com/
> 
> 

Re: [PATCH] Use vtable for eww-bookmarks

[Adam Porter]

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

I don't think it's necessary to retire it. Your patch is an improvement,
no? If it's a step in the right direction, then why not proceed? Further
improvements can be made later.  AFAIK we're not talking about grand
features that would be a crime to make changes to later.  Most development
in Emacs doesn't happen with full consensus anyway, right? Usually someone
has an itch to scratch, and unless someone really objects or a procedure is
seriously violated, it gets scratched.  I've made some suggestions, but
they're only suggestions; they aren't conditions for merging.

Am I missing something here?  :)

On Tue, Jan 7, 2025, 06:23 Sebastián Monía <sebastian@sebasmonia.com> wrote:

> Hello everyone!
>
> I am ok to retire the patch, with no consensus on how to progress.
>
> Regards,
> Seb
>
> On Wed, Jan 1, 2025, at 11:25 PM, Sebastián Monía wrote:
> >
> > Adam Porter <adam@alphapapa.net> writes:
> > >  Also FWIW, I think the long-term goal ought to be to replace EWW's
> > >  bespoke bookmarks with Emacs bookmarks (i.e. any Emacs bookmark to an
> > >  HTTP(S) URL ought to be usable by EWW, and it ought to create such
> > >  bookmarks; EWW should be just another way of opening them).
> >
> > You can create Emacs bookmarks or EWW bookmarks. I use one or the other
> > for different cases. But that's more a peculiarity of how I work, than
> > proof that we need both, being honest.
> >
> > We could deprecate EWW-only bookmarks, I guess. If we plan to do that,
> > merging this patch makes no sense.
> > And then also drop the feature to re-arrange bookmarks, or make it work
> > in the context of standard bookmarks. Ditto for M-n and M-p to navigate
> > next/prev bookmark in EWW.
> >
> > --
> > Sebastián Monía
> > https://site.sebasmonia.com/
> >
> >
>

[-- Attachment #2: Type: text/html, Size: 2486 bytes --]

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

Adam Porter <adam@alphapapa.net> writes:
> I don't think it's necessary to retire it. Your patch is an
> improvement, no?

On a feature that we are, I understand, trying to deprecate. 

> I've made some suggestions, but they're only suggestions; they aren't
> conditions for merging.

Yes, my email wasn't so much about your (appreciated and welcomed)
suggestions. But lack of movement in this thread which is probably a
gauge for interest in the change.

>  Am I missing something here?  :)

Was trying to get ahead to having no updates in the thread for a few
weeks and having a "ping" to try to make a decision. Again.

For the record, my suggestion to drop the changes doesn't come from a
place of disappointment or anything like that. Just being practical, if
EWW bookmarks will be phased out, it makes sense to let the bookmarks
buffer stay as-is :)

Thanks,
Seb

>  On Tue, Jan 7, 2025, 06:23 Sebastián Monía <sebastian@sebasmonia.com> wrote:
>
>  Hello everyone!
>
>  I am ok to retire the patch, with no consensus on how to progress.
>
>  Regards,
>  Seb
>
>  On Wed, Jan 1, 2025, at 11:25 PM, Sebastián Monía wrote:
>  > 
>  > Adam Porter <adam@alphapapa.net> writes:
>  > >  Also FWIW, I think the long-term goal ought to be to replace EWW's
>  > >  bespoke bookmarks with Emacs bookmarks (i.e. any Emacs bookmark to an
>  > >  HTTP(S) URL ought to be usable by EWW, and it ought to create such
>  > >  bookmarks; EWW should be just another way of opening them).
>  > 
>  > You can create Emacs bookmarks or EWW bookmarks. I use one or the other
>  > for different cases. But that's more a peculiarity of how I work, than
>  > proof that we need both, being honest.
>  > 
>  > We could deprecate EWW-only bookmarks, I guess. If we plan to do that,
>  > merging this patch makes no sense.
>  > And then also drop the feature to re-arrange bookmarks, or make it work
>  > in the context of standard bookmarks. Ditto for M-n and M-p to navigate
>  > next/prev bookmark in EWW.

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

Re: [PATCH] Use vtable for eww-bookmarks

[Stefan Kangas]

Sebastián Monía <sebastian@sebasmonia.com> writes:

> lack of movement in this thread which is probably a
> gauge for interest in the change.

(I have no opinion about this change as I didn't follow the discussion,
but from the title alone it sounds good to me.)

I wouldn't necessarily interpret a lack of replies or a long turnaround
time as disinterest.

We don't have many people on board who are actively working on reviewing
and installing patches, so it might take us quite some time before we
can get to your patch.  Please keep in mind that all of us are
volunteers, and do this in our free time too.

Sometimes people forget about things, so feel free to send a ping if we
haven't replied within some reasonable amount of time, usually a couple
of weeks.

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

Stefan Kangas <stefankangas@gmail.com> writes:
> Sebastián Monía <sebastian@sebasmonia.com> writes:
>
>> lack of movement in this thread which is probably a
>> gauge for interest in the change.
>
> I wouldn't necessarily interpret a lack of replies or a long
> turnaround time as disinterest[...] keep in mind that all of us are
> volunteers, and do this in our free time too.

Yes, this is why I said "probably". Heck, even when I made updates to the
patch, sometimes I took a long time to get to it :)

But since Eli's latest ping and the subsequent conversation about
deprecating EWW's own bookmark mechanism, I am thinking that maybe this
isn't worth our collective time. Again, coming from a practical place!

Regards,
Seb

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

Re: [PATCH] Use vtable for eww-bookmarks

[Stefan Kangas]

Sebastián Monía <sebastian@sebasmonia.com> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>> Sebastián Monía <sebastian@sebasmonia.com> writes:
>>
>>> lack of movement in this thread which is probably a
>>> gauge for interest in the change.
>>
>> I wouldn't necessarily interpret a lack of replies or a long
>> turnaround time as disinterest[...] keep in mind that all of us are
>> volunteers, and do this in our free time too.
>
> Yes, this is why I said "probably". Heck, even when I made updates to the
> patch, sometimes I took a long time to get to it :)
>
> But since Eli's latest ping and the subsequent conversation about
> deprecating EWW's own bookmark mechanism, I am thinking that maybe this
> isn't worth our collective time. Again, coming from a practical place!

Is there any reason not to merge your latest patch while we wait for
deprecating EWW's bookmarks to happen?  Adam says that it's already an
improvement, and I'd agree if it means replacing bespoke code with using
vtable.  From a cursory look, I don't see anything controversial in
there.

Adam also asked for people to speak up if they object to the patch over
a week ago, and no one did.

Re: [PATCH] Use vtable for eww-bookmarks

[Jim Porter]

On 1/7/2025 1:01 PM, Stefan Kangas wrote:
> Adam also asked for people to speak up if they object to the patch over
> a week ago, and no one did.

I think that was me. :) (No relation to Adam, though.)

 From my point of view, let's merge this and announce it in a new thread 
so anyone who cares get prodded again to take a look. I'll try to do 
this in the next day or two if I have time, but anyone else who has time 
before me, feel free to do so first.

Re: [PATCH] Use vtable for eww-bookmarks

[Thierry Volpiatto]

Stefan Kangas <stefankangas@gmail.com> writes:

> Sebastián Monía <sebastian@sebasmonia.com> writes:
>
>> Stefan Kangas <stefankangas@gmail.com> writes:
>>> Sebastián Monía <sebastian@sebasmonia.com> writes:
>>>
>>>> lack of movement in this thread which is probably a
>>>> gauge for interest in the change.
>>>
>>> I wouldn't necessarily interpret a lack of replies or a long
>>> turnaround time as disinterest[...] keep in mind that all of us are
>>> volunteers, and do this in our free time too.
>>
>> Yes, this is why I said "probably". Heck, even when I made updates to the
>> patch, sometimes I took a long time to get to it :)
>>
>> But since Eli's latest ping and the subsequent conversation about
>> deprecating EWW's own bookmark mechanism, I am thinking that maybe this
>> isn't worth our collective time. Again, coming from a practical place!
>
> Is there any reason not to merge your latest patch while we wait for
> deprecating EWW's bookmarks to happen?  Adam says that it's already an
> improvement, and I'd agree if it means replacing bespoke code with using
> vtable.  From a cursory look, I don't see anything controversial in
> there.
>
> Adam also asked for people to speak up if they object to the patch over
> a week ago, and no one did.

If you plan to move to emacs bookmarks, I see no reasons to use vtable,
it will complicate conversion of eww bookmarks to bookmarks which is
actually trivial. 

-- 
Thierry

Re: [PATCH] Use vtable for eww-bookmarks

[Adam Porter]

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

>
> If you plan to move to emacs bookmarks, I see no reasons to use vtable,
> it will complicate conversion of eww bookmarks to bookmarks which is
> actually trivial.
>

I'm not sure what you mean. Vtable should keep the presentation separate
from the underlying object type, so changing that later shouldn't be much
trouble.

ISTM that switching EWW bookmarks into Emacs bookmarks would be a bigger
project, because it requires conversion at runtime, likely some UI,
documentation, and testing.  That's why I suggest not letting that idea
stall this patch, because it may or may not even happen, depending on
whether anyone feels like doing it anytime soon.

>

[-- Attachment #2: Type: text/html, Size: 1158 bytes --]

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

Thierry Volpiatto <thievol@posteo.net> writes:
> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> Is there any reason not to merge your latest patch while we wait for
>> deprecating EWW's bookmarks to happen?  Adam says that it's already an
>> improvement, and I'd agree if it means replacing bespoke code with using
>> vtable.  From a cursory look, I don't see anything controversial in
>> there.
>>
>> Adam also asked for people to speak up if they object to the patch over
>> a week ago, and no one did.
>
> If you plan to move to emacs bookmarks, I see no reasons to use vtable,
> it will complicate conversion of eww bookmarks to bookmarks which is
> actually trivial. 

Adam already hinted it in his reply, but this patch doesn't change the
internal representation of EWW-bookmarks.

A minor update, I have started using M-n and M-b and relying on the
bookmark order to navigate them. We don't have any statistics, of
course, but I suspect the feature is largely unknown.
It's interesting. I suspect I found it useful because I don't use a feed
reader, most people would use elfeed/newsticker instead.
It is also relatively easy to write a similar command using the more
generic bookmarks.

Re: [PATCH] Use vtable for eww-bookmarks

[Stefan Kangas]

Sebastián Monía <sebastian@sebasmonia.com> writes:

> Thierry Volpiatto <thievol@posteo.net> writes:
>> Stefan Kangas <stefankangas@gmail.com> writes:
>>
>>> Is there any reason not to merge your latest patch while we wait for
>>> deprecating EWW's bookmarks to happen?  Adam says that it's already an
>>> improvement, and I'd agree if it means replacing bespoke code with using
>>> vtable.  From a cursory look, I don't see anything controversial in
>>> there.
>>>
>>> Adam also asked for people to speak up if they object to the patch over
>>> a week ago, and no one did.
>>
>> If you plan to move to emacs bookmarks, I see no reasons to use vtable,
>> it will complicate conversion of eww bookmarks to bookmarks which is
>> actually trivial.
>
> Adam already hinted it in his reply, but this patch doesn't change the
> internal representation of EWW-bookmarks.
>
> A minor update, I have started using M-n and M-b and relying on the
> bookmark order to navigate them. We don't have any statistics, of
> course, but I suspect the feature is largely unknown.
> It's interesting. I suspect I found it useful because I don't use a feed
> reader, most people would use elfeed/newsticker instead.
> It is also relatively easy to write a similar command using the more
> generic bookmarks.

Could you please resend the latest version of the patch?

Re: [PATCH] Use vtable for eww-bookmarks

[Sebastián Monía]

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


Stefan Kangas <stefankangas@gmail.com> writes:
> Sebastián Monía <sebastian@sebasmonia.com> writes:
>
>> Thierry Volpiatto <thievol@posteo.net> writes:
>>> Stefan Kangas <stefankangas@gmail.com> writes:
>>>
>>>> Is there any reason not to merge your latest patch while we wait for
>>>> deprecating EWW's bookmarks to happen?  Adam says that it's already an
>>>> improvement, and I'd agree if it means replacing bespoke code with using
>>>> vtable.  From a cursory look, I don't see anything controversial in
>>>> there.
>>>>
>>>> Adam also asked for people to speak up if they object to the patch over
>>>> a week ago, and no one did.
>>>
>>> If you plan to move to emacs bookmarks, I see no reasons to use vtable,
>>> it will complicate conversion of eww bookmarks to bookmarks which is
>>> actually trivial.
>>
>> Adam already hinted it in his reply, but this patch doesn't change the
>> internal representation of EWW-bookmarks.
>>
>> A minor update, I have started using M-n and M-b and relying on the
>> bookmark order to navigate them. We don't have any statistics, of
>> course, but I suspect the feature is largely unknown.
>> It's interesting. I suspect I found it useful because I don't use a feed
>> reader, most people would use elfeed/newsticker instead.
>> It is also relatively easy to write a similar command using the more
>> generic bookmarks.
>
> Could you please resend the latest version of the patch?

Attached! This version was sent December 12.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Last path (Dec 12) --]
[-- Type: text/x-patch, Size: 9444 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 (eww-list-bookmarks): Move logic to...:
(eww--bookmark-prepare, eww--bookmark-format-data )
... these,
and use 'vtable'.
(eww-bookmark-kill, eww-bookmark-yank, eww-bookmark-browse,
 eww-next-bookmark, eww-previous-bookmark,
 eww-buffers-mode-map): Use 'vtable-current-object'.
---
 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/

Re: [PATCH] Use vtable for eww-bookmarks

[Jim Porter]

On 1/21/2025 6:36 PM, Sebastián Monía wrote:
> Attached! This version was sent December 12.

Thanks for reposting. I ran with this and found the following issues.

1. Killing a bookmark works, but yanking at the end of the buffer 
produces the following error:

Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
   seq-subseq(((:url "https://www.fsf.org/" :title "Front Page — Free 
Software Foundation — working together for free software" :time "Tue Jan 
21 19:48:33 2025")) nil)
   eww-bookmark-yank()
   funcall-interactively(eww-bookmark-yank)
   command-execute(eww-bookmark-yank)



2. Killing the last bookmark in the buffer produces this error:

Debugger entered--Lisp error: (cl-no-applicable-method 
vtable-objects-function nil)
   signal(cl-no-applicable-method (vtable-objects-function nil))
   cl-no-applicable-method(#s(cl--generic :name vtable-objects-function 
:dispatches ((0 #s(cl--generic-generalizer :name 
eieio--generic-generalizer :priority 50 :tagcode-function 
cl--generic-struct-tag :specializers-function #f(compiled-function (tag 
&rest _) #<bytecode 0x137071fd5144f137>)) #s(cl--generic-generalizer 
:name cl--generic-t-generalizer :priority 0 :tagcode-function 
#f(compiled-function (name &rest _) #<bytecode 0x111a2082463a1535>) 
:specializers-function #f(compiled-function (tag &rest _) #<bytecode 
-0x1a05678245d32db5>)))) :method-table (#s(cl--generic-method 
:specializers (vtable) :qualifiers nil :call-con nil :function 
#f(compiled-function (this) "Retrieve the slot `objects-function' from 
an object of class `vtable'." #<bytecode -0x65b654f1e7958ff>))) :options 
nil) nil)
   apply(cl-no-applicable-method #s(cl--generic :name 
vtable-objects-function :dispatches ((0 #s(cl--generic-generalizer :name 
eieio--generic-generalizer :priority 50 :tagcode-function 
cl--generic-struct-tag :specializers-function #f(compiled-function (tag 
&rest _) #<bytecode 0x137071fd5144f137>)) #s(cl--generic-generalizer 
:name cl--generic-t-generalizer :priority 0 :tagcode-function 
#f(compiled-function (name &rest _) #<bytecode 0x111a2082463a1535>) 
:specializers-function #f(compiled-function (tag &rest _) #<bytecode 
-0x1a05678245d32db5>)))) :method-table (#s(cl--generic-method 
:specializers (vtable) :qualifiers nil :call-con nil :function 
#f(compiled-function (this) "Retrieve the slot `objects-function' from 
an object of class `vtable'." #<bytecode -0x65b654f1e7958ff>))) :options 
nil) nil)
   #f(compiled-function (&rest args) #<bytecode 0x159c15a915096a8f>)(nil)
   apply(#f(compiled-function (&rest args) #<bytecode 
0x159c15a915096a8f>) nil nil)
   vtable-objects-function(nil)
   vtable-revert-command()
   eww-bookmark-kill()
   funcall-interactively(eww-bookmark-kill)
   command-execute(eww-bookmark-kill)