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