[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

[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

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