[PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

---
Small quality of life change in EWW. I was interested in reverting the
buffer list, then I found the "FIXME" note about reverting in
eww-mode, and figured, why not :)

I didn't want to alter eww-reload (since it is part of the public API)
hence the new function for reverting that just calls it.

Thank you,
Seb

 lisp/net/eww.el | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index b2e1c5a72e5..c0ab8761997 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -1193,7 +1193,6 @@ This consults the entries in `eww-readable-urls' (which see)."
           (throw 'found result))))))
 
 (defvar-keymap eww-mode-map
-  "g" #'eww-reload             ;FIXME: revert-buffer-function instead!
   "G" #'eww
   "M-RET" #'eww-open-in-new-buffer
   "TAB" #'shr-next-link
@@ -1331,6 +1330,7 @@ within text input fields."
   (add-hook 'context-menu-functions 'eww-context-menu 5 t)
   (setq-local eww-history nil)
   (setq-local eww-history-position 0)
+  (setq-local revert-buffer-function #'eww--revert-function)
   (when (boundp 'tool-bar-map)
     (setq-local tool-bar-map eww-tool-bar-map))
   ;; desktop support
@@ -1529,6 +1529,13 @@ just re-display the HTML already fetched."
             (eww-retrieve url #'eww-render
 		          (list url (point) (current-buffer) encode))))))))
 
+(defun eww--revert-function (local _noconfirm)
+  "Revert function for EWW buffers.
+LOCAL works like in `eww-reload': when non-nil, reload the page from the
+network instead of the HTML already retrieved. It is the prefix arg."
+  (eww-reload local)
+  (message "Page reloaded."))
+
 ;; Form support.
 
 (defvar eww-form nil)
@@ -2601,8 +2608,10 @@ see)."
 
 ;;; eww buffers list
 
-(defun eww-list-buffers ()
-  "Enlist eww buffers."
+(defun eww-list-buffers (&optional _ignore-auto _noconfirm)
+  "Pop a buffer with a list of eww buffers.
+Optional arguments make this function compatible with the
+`revert-buffer-function' interface."
   (interactive)
   (let (buffers-info
         (current (current-buffer)))
@@ -2621,6 +2630,7 @@ see)."
           (domain-length 0)
           (title-length 0)
           url title format start)
+      (setq-local revert-buffer-function #'eww-list-buffers)
       (erase-buffer)
       (dolist (buffer-info buffers-info)
         (setq title-length (max title-length
-- 
2.45.2.windows.1
-- 
Sebastián Monía
https://site.sebasmonia.com/

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Eli Zaretskii]

> From: Sebastián Monía <sebastian@sebasmonia.com>
> Date: Fri, 16 Aug 2024 13:28:54 -0400
> 
> Small quality of life change in EWW. I was interested in reverting the
> buffer list, then I found the "FIXME" note about reverting in
> eww-mode, and figured, why not :)
> 
> I didn't want to alter eww-reload (since it is part of the public API)
> hence the new function for reverting that just calls it.

Is it really TRT to call eww-reload?  The FIXME says we need a real
revert-buffer-function, so I presume just calling eww-reload is
somehow not the best solution?

In any case, what is the improvement provided by this change?

Thanks.

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

Eli Zaretskii <eliz@gnu.org> writes:

For reloading EWW buffers, I guess not much, except for using the
"standard" machinery to revert buffers. My first version changed
eww-reload instead, but then I realized someone might be already be
using that command.

The revert/reload for EWW buffer list is convenient. Without it, you get
a message that the buffer is not backed by a file.

I am fine keeping only buffer list change :)


>> From: Sebastián Monía <sebastian@sebasmonia.com>
>> Date: Fri, 16 Aug 2024 13:28:54 -0400
>> 
>> Small quality of life change in EWW. I was interested in reverting the
>> buffer list, then I found the "FIXME" note about reverting in
>> eww-mode, and figured, why not :)
>> 
>> I didn't want to alter eww-reload (since it is part of the public API)
>> hence the new function for reverting that just calls it.
>
> Is it really TRT to call eww-reload?  The FIXME says we need a real
> revert-buffer-function, so I presume just calling eww-reload is
> somehow not the best solution?
>
> In any case, what is the improvement provided by this change?
>
> Thanks.

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

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

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

---
This smaller patch does only the eww buffer list portion, and we leave
EWW buffer reloads as they are now.

 lisp/net/eww.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index b2e1c5a72e5..660731faa30 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2601,8 +2601,10 @@ see)."
 
 ;;; eww buffers list
 
-(defun eww-list-buffers ()
-  "Enlist eww buffers."
+(defun eww-list-buffers (&optional _ignore-auto _noconfirm)
+  "Pop a buffer with a list of eww buffers.
+Optional arguments make this function compatible with the
+`revert-buffer-function' interface."
   (interactive)
   (let (buffers-info
         (current (current-buffer)))
@@ -2719,6 +2721,7 @@ see)."
 \\{eww-buffers-mode-map}"
   :interactive nil
   (buffer-disable-undo)
+  (setq-local revert-buffer-function #'eww-list-buffers)
   (setq truncate-lines t))
 
 ;;; Desktop support
-- 
2.45.2.windows.1





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

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Eli Zaretskii]

> From: Sebastián Monía <sebastian@sebasmonia.com>
> Cc: emacs-devel@gnu.org
> Date: Sat, 17 Aug 2024 08:35:36 -0400
> 
> Sebastián Monía <sebastian@sebasmonia.com> writes:
> 
> ---
> This smaller patch does only the eww buffer list portion, and we leave
> EWW buffer reloads as they are now.
> 
>  lisp/net/eww.el | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/lisp/net/eww.el b/lisp/net/eww.el
> index b2e1c5a72e5..660731faa30 100644
> --- a/lisp/net/eww.el
> +++ b/lisp/net/eww.el
> @@ -2601,8 +2601,10 @@ see)."
>  
>  ;;; eww buffers list
>  
> -(defun eww-list-buffers ()
> -  "Enlist eww buffers."
> +(defun eww-list-buffers (&optional _ignore-auto _noconfirm)
> +  "Pop a buffer with a list of eww buffers.
> +Optional arguments make this function compatible with the
> +`revert-buffer-function' interface."
>    (interactive)
>    (let (buffers-info
>          (current (current-buffer)))
> @@ -2719,6 +2721,7 @@ see)."
>  \\{eww-buffers-mode-map}"
>    :interactive nil
>    (buffer-disable-undo)
> +  (setq-local revert-buffer-function #'eww-list-buffers)
>    (setq truncate-lines t))
>  
>  ;;; Desktop support
> -- 
> 2.45.2.windows.1

Jim, would you please review this and provide comments, if any?

Thanks.

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Jim Porter]

On 8/24/2024 1:42 AM, Eli Zaretskii wrote:
> Jim, would you please review this and provide comments, if any?

Hmm, it seems strange to me that this patch makes 'eww-list-buffers' be 
the 'revert-buffer-function'. That function contains some additional 
logic that I don't think belongs for reverting the buffer. For example, 
if I opened the EWW buffer list and then renamed the buffer, I'd expect 
'revert-buffer' to update the contents of that (now-renamed) buffer. 
 From reading the code, I think what would happen is that it pops to a 
*new* buffer with the original "*eww buffers*" name.

So I think we'd want some new function like 'eww--do-list-buffers' that 
takes the code from 'eww-list-buffers' that actually writes out the text 
(i.e. the 'let' form starting with '(inhibit-read-only t)'). Then have 
'eww-list-buffers' call that, and set 'revert-buffer-function' to 
'eww--do-list-buffers'. Or something like that anyway...

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

Hi Jim,

Thanks for your review.

On Sat, Aug 24, 2024, at 1:27 PM, Jim Porter wrote:
> So I think we'd want some new function like 'eww--do-list-buffers' that 
> takes the code from 'eww-list-buffers' that actually writes out the text 
> (i.e. the 'let' form starting with '(inhibit-read-only t)'). Then have 
> 'eww-list-buffers' call that, and set 'revert-buffer-function' to 
> 'eww--do-list-buffers'. Or something like that anyway...

You are entirely correct, will provide an updated patch whenever I get a chance.
Thank you,
Seb

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Eli Zaretskii]

> Date: Fri, 30 Aug 2024 16:01:00 -0400
> From: Sebastián Monía <sebastian@sebasmonia.com>
> Cc: emacs-devel@gnu.org
> 
> Hi Jim,
> 
> Thanks for your review.
> 
> On Sat, Aug 24, 2024, at 1:27 PM, Jim Porter wrote:
> > So I think we'd want some new function like 'eww--do-list-buffers' that 
> > takes the code from 'eww-list-buffers' that actually writes out the text 
> > (i.e. the 'let' form starting with '(inhibit-read-only t)'). Then have 
> > 'eww-list-buffers' call that, and set 'revert-buffer-function' to 
> > 'eww--do-list-buffers'. Or something like that anyway...
> 
> You are entirely correct, will provide an updated patch whenever I get a chance.

Any progress there?

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Eli Zaretskii]

[Please use Reply All to reply, to CC everyone else and the list.]

Resending to the list:

> From: Sebastián Monía <sebastian@sebasmonia.com>
> Date: Thu, 19 Sep 2024 16:47:38 -0400
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Any progress there?
> 
> Not really, but was looking at this for a bit now.
> 
> The way we collect the buffer list first, then print "No buffers"
> doesn't help in splitting the function.
> 
> There are many (hacky) ways to work around that, but I am thinking that
> maybe it would it be better to use tabulated-list-mode or vtable to
> display this?
> 
> I know, the change is more involved. But seems like a better path
> forward.
> 
> If you think that's overkill, then one possible hacky solution is:
> 
> 1. split the function in three: eww-list-buffers (command)
> eww--print-list-buffers (writes the buffer), eww--collect-list-buffers
> (gets the list of buffers and vectors with eww-data)
> 
> 2. setting a buffer-local variable with the EWW buffers info in
> eww-list-buffers.
> 
> 3. in eww--print-list-buffers, use the variable if present, discard the
> value after using it. Calls for reverting won't find a value and will
> call eww--collect-list-buffer. So the first time it re-uses data, and
> subsequent calls get it fresh.
> 
> Honestly, seems hacky and convoluted. That's why I thought, hey
> maybe this should be a tabulated-list instead. Or vtable.
> 
> -- 
> Sebastián Monía
> https://site.sebasmonia.com/
> 

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

Eli Zaretskii <eliz@gnu.org> writes:
> [Please use Reply All to reply, to CC everyone else and the list.]

This was a very silly mistake, thanks for resending.

> Resending to the list:

>> > Any progress there?
>> 
>> Not really, but was looking at this for a bit now.
>> 
>> The way we collect the buffer list first, then print "No buffers"
>> doesn't help in splitting the function.
>> 
>> There are many (hacky) ways to work around that, but I am thinking that
>> maybe it would it be better to use tabulated-list-mode or vtable to
>> display this?
>> 
>> I know, the change is more involved. But seems like a better path
>> forward.

@Jim

A simpler hacky solution that I thought of this morning. And it doesn't
modify the original code as much:

On top of my last patch, check if the current buffer is in
`eww-buffers-mode` then use it, if not, get-create "*eww buffers list*".
That would work even if the user renamed the buffer. And it is a very
simple change.

Do you think it is an acceptable solution?

If not, I will convert it to tabulated-list (or vtable), only
disadvantage I see, is that it would change the internals of the buffer
list quite a bit. I don't expect anyone to rely on them, though.

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

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Jim Porter]

On 9/20/2024 8:06 AM, Sebastián Monía wrote:
> If not, I will convert it to tabulated-list (or vtable), only
> disadvantage I see, is that it would change the internals of the buffer
> list quite a bit. I don't expect anyone to rely on them, though.

I think a tabulated list would be fine.

Your other earlier suggestion of splitting 'eww-list-buffers' in three 
also makes sense to me though. Then one of the functions could be 
'eww-buffer-list' (which returns a list of EWW buffers), just like how 
we have 'buffer-list' and 'list-buffers' in Emacs.

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

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

Jim Porter <jporterbugs@gmail.com> writes:

> On 9/20/2024 8:06 AM, Sebastián Monía wrote:
>> If not, I will convert it to tabulated-list (or vtable), only
>> disadvantage I see, is that it would change the internals of the buffer
>> list quite a bit. I don't expect anyone to rely on them, though.
>
> I think a tabulated list would be fine.

I included below a patch that does this

> Your other earlier suggestion of splitting 'eww-list-buffers' in three
> also makes sense to me though. Then one of the functions could be
> 'eww-buffer-list' (which returns a list of EWW buffers), just like how
> we have 'buffer-list' and 'list-buffers' in Emacs.

I read this just now :(
I can add it in a subsequent patch.

BTW, 'eww-bookmark-mode' could also use a tabulated list.
I use the eww specific bookmarks or some pages (and "general" bookmarks
for others, everyone has their workflows I guess).

If you all think it is worth it, I can make a similar change to this one
for bookmarks. And add 'eww-list-buffers' too.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Derive-eww-buffers-mode-from-tabulated-list-adjust-c.patch --]
[-- Type: text/x-patch, Size: 5558 bytes --]

From 5fc0308ea6569ab5bc06db45ac1e7f7590c90c42 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebasti=C3=A1n=20Mon=C3=ADa?= <sebastian@sebasmonia.com>
Date: Fri, 20 Sep 2024 21:52:05 -0400
Subject: [PATCH] Derive eww-buffers-mode from tabulated-list, adjust command
 eww-list-buffers to use it

---
 lisp/net/eww.el | 95 +++++++++++++++++--------------------------------
 1 file changed, 32 insertions(+), 63 deletions(-)

diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index b5d2f20781a..a651d9d5020 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2605,57 +2605,31 @@ see)."
 ;;; eww buffers list
 
 (defun eww-list-buffers ()
-  "Enlist eww buffers."
+  "Pop a buffer with a list of eww buffers."
   (interactive)
-  (let (buffers-info
-        (current (current-buffer)))
-    (dolist (buffer (buffer-list))
-      (with-current-buffer buffer
-        (when (derived-mode-p 'eww-mode)
-          (push (vector buffer (plist-get eww-data :title)
-                        (plist-get eww-data :url))
-                buffers-info))))
-    (unless buffers-info
-      (error "No eww buffers"))
-    (setq buffers-info (nreverse buffers-info)) ;more recent on top
-    (set-buffer (get-buffer-create "*eww buffers*"))
-    (eww-buffers-mode)
-    (let ((inhibit-read-only t)
-          (domain-length 0)
-          (title-length 0)
-          url title format start)
-      (erase-buffer)
-      (dolist (buffer-info buffers-info)
-        (setq title-length (max title-length
-                                (length (elt buffer-info 1)))
-              domain-length (max domain-length
-                                 (length (elt buffer-info 2)))))
-      (setq format (format "%%-%ds %%-%ds" title-length domain-length)
-            header-line-format
-            (concat " " (format format "Title" "URL")))
-      (let ((line 0)
-            (current-buffer-line 1))
-        (dolist (buffer-info buffers-info)
-          (setq start (point)
-                title (elt buffer-info 1)
-                url (elt buffer-info 2)
-                line (1+ line))
-          (insert (format format title url))
-          (insert "\n")
-          (let ((buffer (elt buffer-info 0)))
-            (put-text-property start (1+ start) 'eww-buffer
-                               buffer)
-            (when (eq current buffer)
-              (setq current-buffer-line line))))
-        (goto-char (point-min))
-        (forward-line (1- current-buffer-line)))))
+  (with-current-buffer (get-buffer-create "*eww buffers*")
+    (eww-buffers-mode))
   (pop-to-buffer "*eww buffers*"))
 
+(defun eww--list-buffers-get-eww-data (buf)
+  "Return the eww-data of BUF, if its major mode is eww.
+The format (buffer [title url]) is for use in of `eww-buffers-mode'.
+If BUF has another mode, return nil."
+  (with-current-buffer buf
+    (when (derived-mode-p 'eww-mode)
+      (list buf
+            (vector
+             (plist-get eww-data :title)
+             (plist-get eww-data :url))))))
+
+(defun eww--list-buffers-collect ()
+  "Update the eww buffers list displayed in this buffer."
+  (delq nil (mapcar #'eww--list-buffers-get-eww-data (buffer-list))))
+
 (defun eww-buffer-select ()
   "Switch to eww buffer."
   (interactive nil eww-buffers-mode)
-  (let ((buffer (get-text-property (line-beginning-position)
-                                   'eww-buffer)))
+  (let ((buffer (tabulated-list-get-id)))
     (unless buffer
       (error "No buffer on current line"))
     (quit-window)
@@ -2663,8 +2637,7 @@ see)."
 
 (defun eww-buffer-show ()
   "Display buffer under point in eww buffer list."
-  (let ((buffer (get-text-property (line-beginning-position)
-                                   'eww-buffer)))
+  (let ((buffer (tabulated-list-get-id)))
     (unless buffer
       (error "No buffer on current line"))
     (other-window -1)
@@ -2691,17 +2664,11 @@ see)."
 (defun eww-buffer-kill ()
   "Kill buffer from eww list."
   (interactive nil eww-buffers-mode)
-  (let* ((start (line-beginning-position))
-	 (buffer (get-text-property start 'eww-buffer))
-	 (inhibit-read-only t))
+  (let ((buffer (tabulated-list-get-id)))
     (unless buffer
       (user-error "No buffer on the current line"))
     (kill-buffer buffer)
-    (forward-line 1)
-    (delete-region start (point)))
-  (when (eobp)
-    (forward-line -1))
-  (eww-buffer-show))
+    (revert-buffer)))
 
 (defvar-keymap eww-buffers-mode-map
   "C-k" #'eww-buffer-kill
@@ -2709,20 +2676,22 @@ see)."
   "n" #'eww-buffer-show-next
   "p" #'eww-buffer-show-previous
   :menu '("Eww Buffers"
-          ["Exit" quit-window t]
           ["Select" eww-buffer-select
-           :active (get-text-property (line-beginning-position) 'eww-buffer)]
+           :active (tabulated-list-get-id)]
           ["Kill" eww-buffer-kill
-           :active (get-text-property (line-beginning-position)
-                                      'eww-buffer)]))
+           :active (tabulated-list-get-id)]
+          ["Exit" quit-window]))
 
-(define-derived-mode eww-buffers-mode special-mode "eww buffers"
+(define-derived-mode eww-buffers-mode tabulated-list-mode "eww buffers"
   "Mode for listing buffers.
-
 \\{eww-buffers-mode-map}"
   :interactive nil
-  (buffer-disable-undo)
-  (setq truncate-lines t))
+  (setq tabulated-list-format [("Title" 30 t)
+                               ("URL" 0 t)])
+  (setq tabulated-list-padding 1
+        tabulated-list-entries #'eww--list-buffers-collect)
+  (tabulated-list-init-header)
+  (tabulated-list-print))
 
 ;;; Desktop support
 
-- 
2.45.2.windows.1


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


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

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

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

> Jim Porter <jporterbugs@gmail.com> writes:
>
>> On 9/20/2024 8:06 AM, Sebastián Monía wrote:
>>> If not, I will convert it to tabulated-list (or vtable), only
>>> disadvantage I see, is that it would change the internals of the buffer
>>> list quite a bit. I don't expect anyone to rely on them, though.
>>
>> I think a tabulated list would be fine.
>
> I included below a patch that does this
>
>> Your other earlier suggestion of splitting 'eww-list-buffers' in three
>> also makes sense to me though. Then one of the functions could be
>> 'eww-buffer-list' (which returns a list of EWW buffers), just like how
>> we have 'buffer-list' and 'list-buffers' in Emacs.
>
> I read this just now :(
> I can add it in a subsequent patch.
>
> BTW, 'eww-bookmark-mode' could also use a tabulated list.
> I use the eww specific bookmarks or some pages (and "general" bookmarks
> for others, everyone has their workflows I guess).
>
> If you all think it is worth it, I can make a similar change to this one
> for bookmarks. And add 'eww-list-buffers' too.

Hello!
Reviving this thread before attempting the bookmark changes - just in
case there's some feedback on the tabulated list setup and usage.

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

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Adam Porter]

Hi Sebastián,

> Sebastián Monía <sebastian@sebasmonia.com> writes:
> 
>> Jim Porter <jporterbugs@gmail.com> writes:
>>
>>> On 9/20/2024 8:06 AM, Sebastián Monía wrote:
>>>> If not, I will convert it to tabulated-list (or vtable), only
>>>> disadvantage I see, is that it would change the internals of the buffer
>>>> list quite a bit. I don't expect anyone to rely on them, though.
>>>
>>> I think a tabulated list would be fine.
>>
>> I included below a patch that does this
>>
>>> Your other earlier suggestion of splitting 'eww-list-buffers' in three
>>> also makes sense to me though. Then one of the functions could be
>>> 'eww-buffer-list' (which returns a list of EWW buffers), just like how
>>> we have 'buffer-list' and 'list-buffers' in Emacs.
>>
>> I read this just now :(
>> I can add it in a subsequent patch.
>>
>> BTW, 'eww-bookmark-mode' could also use a tabulated list.
>> I use the eww specific bookmarks or some pages (and "general" bookmarks
>> for others, everyone has their workflows I guess).
>>
>> If you all think it is worth it, I can make a similar change to this one
>> for bookmarks. And add 'eww-list-buffers' too.
> 
> Hello!
> Reviving this thread before attempting the bookmark changes - just in
> case there's some feedback on the tabulated list setup and usage.

A list buffer for EWW bookmarks sounds useful, but I'd suggest using the 
newer Vtable library rather than tabulated-list.  :)

--Adam

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

Hello!

On Wed, Oct 2, 2024, at 8:20 PM, Adam Porter wrote:
> A list buffer for EWW bookmarks sounds useful, but I'd suggest using the 
> newer Vtable library rather than tabulated-list.  :)
> --Adam

I suggested a vtable or tablist initially, and we agreed to tablist.
I don't have a strong preference, and I am familiar with both packages. We can revisit the decision.

The buffer list is an existing feature BTW. This is about changing it to a tablist. And while we are at it doing the same thing with the EWW bookmarks. 
Both are implemented using text with properties for actions. 

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Jim Porter]

On 10/2/2024 8:17 PM, Sebastián Monía wrote:
> On Wed, Oct 2, 2024, at 8:20 PM, Adam Porter wrote:
>> A list buffer for EWW bookmarks sounds useful, but I'd suggest using the
>> newer Vtable library rather than tabulated-list.  :)
>> --Adam
> 
> I suggested a vtable or tablist initially, and we agreed to tablist.
> I don't have a strong preference, and I am familiar with both packages. We can revisit the decision.

I don't have a strong opinion either, though I'm not sure what vtable 
gets us over tabulated-list. I just haven't had time to take a look at 
the patch yet...

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

Jim Porter <jporterbugs@gmail.com> writes:
> I don't have a strong opinion either, though I'm not sure what vtable
> gets us over tabulated-list. I just haven't had time to take a look at
> the patch yet...


vtables are variable pitch by default, and offer a different mode of
interaction (the table is just an object in a portion of the buffer,
rather than a major mode).

I don't think it makes a difference in this case, but that speaks more
of my preference for fixed-width fonts in all tables :)

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

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Jim Porter]

On 9/20/2024 7:14 PM, Sebastián Monía wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
> 
>> On 9/20/2024 8:06 AM, Sebastián Monía wrote:
>>> If not, I will convert it to tabulated-list (or vtable), only
>>> disadvantage I see, is that it would change the internals of the buffer
>>> list quite a bit. I don't expect anyone to rely on them, though.
>>
>> I think a tabulated list would be fine.
> 
> I included below a patch that does this

This patch looks good to me, though I don't know a whole lot about the 
details of 'tabulated-list'. If Eli thinks this is ok too, feel free to 
merge.

> BTW, 'eww-bookmark-mode' could also use a tabulated list.
> I use the eww specific bookmarks or some pages (and "general" bookmarks
> for others, everyone has their workflows I guess).

That makes sense to me.

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Adam Porter]

On 10/3/24 07:47, Sebastián Monía wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
>> I don't have a strong opinion either, though I'm not sure what vtable
>> gets us over tabulated-list. I just haven't had time to take a look at
>> the patch yet...
> 
> vtables are variable pitch by default, and offer a different mode of
> interaction (the table is just an object in a portion of the buffer,
> rather than a major mode).
> 
> I don't think it makes a difference in this case, but that speaks more
> of my preference for fixed-width fonts in all tables :)

AFAIK Vtable is intended to support fixed-width fonts, if so-configured 
(and if a case is found in which it doesn't, that would be a bug, AFAIK).

IMHO, I would strongly encourage the use of Vtable going forward, over 
tabulated-list-mode.  It's more powerful (e.g. can show images, and 
individual objects can be updated in-place), and its API is easier to 
use (e.g. objects can be used as-is rather than having to make vector 
containers).

An example of using Vtable can be seen in my listen.el package: 
<https://github.com/alphapapa/listen.el/blob/69432d45b9654e4a9592e229b0315fd446390e1b/listen-queue.el> 
  You can see that it's pretty straightforward to use.

My two cents.  :)

--Adam

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

On Thu, Oct 3, 2024, at 7:41 PM, Adam Porter wrote:
> On 10/3/24 07:47, Sebastián Monía wrote:
> > Jim Porter <jporterbugs@gmail.com> writes:
> >> I don't have a strong opinion either, though I'm not sure what vtable
> >> gets us over tabulated-list. I just haven't had time to take a look at
> >> the patch yet...
> > 
> > I don't think it makes a difference in this case, but that speaks more
> > of my preference for fixed-width fonts in all tables :)
> 
> IMHO, I would strongly encourage the use of Vtable going forward, over 
> tabulated-list-mode.  It's more powerful (e.g. can show images, and 
> individual objects can be updated in-place), and its API is easier to 
> use (e.g. objects can be used as-is rather than having to make vector 
> containers).

I've used them a bit myself, and to be honest I go for vtable for code I write for my own use.
But tabulated-list has been around longer (so, more proven, probably more stable). So I went with that. 

Like I said, fine to change if there's a will or hard preference.
Or an official stance on it? @Eli

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Eli Zaretskii]

> Date: Fri, 11 Oct 2024 18:14:37 -0400
> From: Sebastián Monía <sebastian@sebasmonia.com>
> Cc: "Eli Zaretskii" <eliz@gnu.org>, emacs-devel@gnu.org
> 
> On Thu, Oct 3, 2024, at 7:41 PM, Adam Porter wrote:
> > On 10/3/24 07:47, Sebastián Monía wrote:
> > > Jim Porter <jporterbugs@gmail.com> writes:
> > >> I don't have a strong opinion either, though I'm not sure what vtable
> > >> gets us over tabulated-list. I just haven't had time to take a look at
> > >> the patch yet...
> > > 
> > > I don't think it makes a difference in this case, but that speaks more
> > > of my preference for fixed-width fonts in all tables :)
> > 
> > IMHO, I would strongly encourage the use of Vtable going forward, over 
> > tabulated-list-mode.  It's more powerful (e.g. can show images, and 
> > individual objects can be updated in-place), and its API is easier to 
> > use (e.g. objects can be used as-is rather than having to make vector 
> > containers).
> 
> I've used them a bit myself, and to be honest I go for vtable for code I write for my own use.
> But tabulated-list has been around longer (so, more proven, probably more stable). So I went with that. 
> 
> Like I said, fine to change if there's a will or hard preference.
> Or an official stance on it? @Eli

I didn't yet have time to read your discussion and make up my mind.  I
was waiting for you to reach some agreement, but it seems you are
still discussing the various aspects?

Are there any downsides to using vtable?

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Jim Porter]

On 10/12/2024 1:05 AM, Eli Zaretskii wrote:
>> Date: Fri, 11 Oct 2024 18:14:37 -0400
>> From: Sebastián Monía <sebastian@sebasmonia.com>
>> Cc: "Eli Zaretskii" <eliz@gnu.org>, emacs-devel@gnu.org
>>
>> Like I said, fine to change if there's a will or hard preference.
>> Or an official stance on it? @Eli
> 
> I didn't yet have time to read your discussion and make up my mind.  I
> was waiting for you to reach some agreement, but it seems you are
> still discussing the various aspects?
> 
> Are there any downsides to using vtable?

This is probably partly my fault since I said that using 
tabulated-list-mode would be fine (and didn't mention vtable one way or 
the other). I truly have no opinion on their relative merits, not having 
written code for either. So long as there are no major downsides to 
vtable (and it sounds like there are not), then I'm totally fine with 
using that.

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

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


Eli Zaretskii <eliz@gnu.org> writes:
> I didn't yet have time to read your discussion and make up my mind.  I
> was waiting for you to reach some agreement, but it seems you are
> still discussing the various aspects?

I was wondering if there was an official project stance on using vtable
or tabulated-list. Like there's a preference for seq vs cl-lib (where
both apply) for example.

> Are there any downsides to using vtable?

None that I know of, and as Adam suggested, vtables are sometimes easier
to handle.
It doesn't make much of a difference in this case, though, the table is
pretty simple.

Attached a new patch that used vtable, and also adds the
(eww-buffer-list) function that Jim mentioned before.
Once this one is merged (after review & corrections :) etc.) I can take
the same vtable approach for eww-bookmarks.

Thank you,
Seb


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: eww-list-buffers and eww-buffer-list --]
[-- Type: text/x-patch, Size: 5916 bytes --]

From bb090996db7eb7d819b7d28e3dd1a97d6bccc959 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebasti=C3=A1n=20Mon=C3=ADa?= <code@sebasmonia.com>
Date: Sun, 13 Oct 2024 21:00:03 -0400
Subject: [PATCH] Use vtable in eww-list-buffers, add function eww-buffer-list

---
 lisp/net/eww.el | 96 +++++++++++++++++++++++--------------------------
 1 file changed, 45 insertions(+), 51 deletions(-)

diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index b5d2f20781a..b0467500ef3 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -33,6 +33,7 @@
 (require 'url)
 (require 'url-queue)
 (require 'url-file)
+(require 'vtable)
 (require 'xdg)
 (eval-when-compile (require 'subr-x))
 
@@ -2604,58 +2605,50 @@ eww-history-mode
 
 ;;; eww buffers list
 
+(defun eww-buffer-list ()
+  "Return a list of all live eww buffers."
+  (seq-filter (lambda (buf)
+                (with-current-buffer buf
+                  (derived-mode-p 'eww-mode)))
+              (buffer-list)))
+
 (defun eww-list-buffers ()
-  "Enlist eww buffers."
+  "Pop a buffer with a list of eww buffers."
   (interactive)
-  (let (buffers-info
-        (current (current-buffer)))
-    (dolist (buffer (buffer-list))
-      (with-current-buffer buffer
-        (when (derived-mode-p 'eww-mode)
-          (push (vector buffer (plist-get eww-data :title)
-                        (plist-get eww-data :url))
-                buffers-info))))
-    (unless buffers-info
-      (error "No eww buffers"))
-    (setq buffers-info (nreverse buffers-info)) ;more recent on top
-    (set-buffer (get-buffer-create "*eww buffers*"))
+  (with-current-buffer (get-buffer-create "*eww buffers*")
     (eww-buffers-mode)
-    (let ((inhibit-read-only t)
-          (domain-length 0)
-          (title-length 0)
-          url title format start)
-      (erase-buffer)
-      (dolist (buffer-info buffers-info)
-        (setq title-length (max title-length
-                                (length (elt buffer-info 1)))
-              domain-length (max domain-length
-                                 (length (elt buffer-info 2)))))
-      (setq format (format "%%-%ds %%-%ds" title-length domain-length)
-            header-line-format
-            (concat " " (format format "Title" "URL")))
-      (let ((line 0)
-            (current-buffer-line 1))
-        (dolist (buffer-info buffers-info)
-          (setq start (point)
-                title (elt buffer-info 1)
-                url (elt buffer-info 2)
-                line (1+ line))
-          (insert (format format title url))
-          (insert "\n")
-          (let ((buffer (elt buffer-info 0)))
-            (put-text-property start (1+ start) 'eww-buffer
-                               buffer)
-            (when (eq current buffer)
-              (setq current-buffer-line line))))
-        (goto-char (point-min))
-        (forward-line (1- current-buffer-line)))))
+    (eww--list-buffers-display-table))
   (pop-to-buffer "*eww buffers*"))
 
+(defun eww--list-buffers-display-table (&optional ignore-auto noconfirm)
+  "Display a table with the list of eww buffers.
+Will remove all buffer contents first.  The parameters IGNORE-AUTO and
+NOCONFIRM are ignored, they are for compatibility with
+`revert-buffer-function'."
+  (let ((inhibit-read-only t))
+    (erase-buffer)
+    (make-vtable
+     :columns '((:name "Title" :width 30)
+                (:name "URL"))
+     :objects-function #'eww--list-buffers-get-data
+     ;; use fixed-font face
+     :face 'default)))
+
+(defun eww--list-buffers-get-data ()
+  "Return the eww-data of BUF, assumed to be a eww buffer.
+The format of the data is (title url buffer), for use in of
+`eww-buffers-mode'."
+  (mapcar (lambda (buf)
+            (with-current-buffer buf
+              (list (plist-get eww-data :title)
+                    (plist-get eww-data :url)
+                    buf)))
+          (eww-buffer-list)))
+
 (defun eww-buffer-select ()
   "Switch to eww buffer."
   (interactive nil eww-buffers-mode)
-  (let ((buffer (get-text-property (line-beginning-position)
-                                   'eww-buffer)))
+  (let ((buffer (nth 2 (vtable-current-object))))
     (unless buffer
       (error "No buffer on current line"))
     (quit-window)
@@ -2663,8 +2656,7 @@ eww-buffer-select
 
 (defun eww-buffer-show ()
   "Display buffer under point in eww buffer list."
-  (let ((buffer (get-text-property (line-beginning-position)
-                                   'eww-buffer)))
+  (let ((buffer (nth 2 (vtable-current-object))))
     (unless buffer
       (error "No buffer on current line"))
     (other-window -1)
@@ -2692,7 +2684,7 @@ eww-buffer-kill
   "Kill buffer from eww list."
   (interactive nil eww-buffers-mode)
   (let* ((start (line-beginning-position))
-	 (buffer (get-text-property start 'eww-buffer))
+	 (buffer (nth 2 (vtable-current-object)))
 	 (inhibit-read-only t))
     (unless buffer
       (user-error "No buffer on the current line"))
@@ -2711,10 +2703,9 @@ eww-buffers-mode-map
   :menu '("Eww Buffers"
           ["Exit" quit-window t]
           ["Select" eww-buffer-select
-           :active (get-text-property (line-beginning-position) 'eww-buffer)]
+           :active (nth 2 (vtable-current-object))]
           ["Kill" eww-buffer-kill
-           :active (get-text-property (line-beginning-position)
-                                      'eww-buffer)]))
+           :active (nth 2 (vtable-current-object))]))
 
 (define-derived-mode eww-buffers-mode special-mode "eww buffers"
   "Mode for listing buffers.
@@ -2722,7 +2713,10 @@ eww-buffers-mode
 \\{eww-buffers-mode-map}"
   :interactive nil
   (buffer-disable-undo)
-  (setq truncate-lines t))
+  (setq truncate-lines t
+        ;; this is set so that pressing "g" with point just below the
+        ;; table will still update the listing
+        revert-buffer-function #'eww--list-buffers-display-table))
 
 ;;; Desktop support
 
-- 
2.43.0

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Adam Porter]

Hi Seb,

On 10/13/24 20:06, Sebastián Monía wrote:
> 
> Eli Zaretskii <eliz@gnu.org> writes:
>> I didn't yet have time to read your discussion and make up my mind.  I
>> was waiting for you to reach some agreement, but it seems you are
>> still discussing the various aspects?
> 
> I was wondering if there was an official project stance on using vtable
> or tabulated-list. Like there's a preference for seq vs cl-lib (where
> both apply) for example.
> 
>> Are there any downsides to using vtable?
> 
> None that I know of, and as Adam suggested, vtables are sometimes easier
> to handle.
> It doesn't make much of a difference in this case, though, the table is
> pretty simple.
> 
> Attached a new patch that used vtable, and also adds the
> (eww-buffer-list) function that Jim mentioned before.
> Once this one is merged (after review & corrections :) etc.) I can take
> the same vtable approach for eww-bookmarks.

Small suggestion: When possible, using BUFFER-LOCAL-VALUE is preferable 
over WITH-CURRENT-BUFFER, as it's significantly faster (in my testing). 
You could do this in `eww--list-buffers-get-data'.

Also, you might want to disable the undo list in the list buffer to save 
memory, since there's no need to undo in that buffer.

Finally, in `eww-buffer-list', you could use `match-buffers' instead of 
a bespoke loop.

Thanks for working on this.

--Adam

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Jim Porter]

On 10/13/2024 6:06 PM, Sebastián Monía wrote:
> Attached a new patch that used vtable, and also adds the
> (eww-buffer-list) function that Jim mentioned before.
> Once this one is merged (after review & corrections :) etc.) I can take
> the same vtable approach for eww-bookmarks.

Thanks for the updated patch. Aside from the things Adam has already 
mentioned, this looks good to me. Just one small comment:

> +(defun eww--list-buffers-display-table (&optional ignore-auto noconfirm)
> +  "Display a table with the list of eww buffers.
> +Will remove all buffer contents first.  The parameters IGNORE-AUTO and
> +NOCONFIRM are ignored, they are for compatibility with
> +`revert-buffer-function'."
> +  (let ((inhibit-read-only t))
> +    (erase-buffer)
> +    (make-vtable
> +     :columns '((:name "Title" :width 30)

Maybe something like ':min-width "25%" :max-width "50%"' here? That way 
users can see longish page titles, and if all the titles are short, we 
give most of the space to the URLs.

(As an aside, I wonder how hard it would be to add support for 
'auto-revert-mode' to the EWW buffer list.)

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

Hello!

Adam Porter <adam@alphapapa.net> writes:
> Small suggestion: When possible, using BUFFER-LOCAL-VALUE is
> preferable over WITH-CURRENT-BUFFER, as it's significantly faster (in
> my testing). You could do this in `eww--list-buffers-get-data'.
>
> Also, you might want to disable the undo list in the list buffer to
> save memory, since there's no need to undo in that buffer.
>
> Finally, in `eww-buffer-list', you could use `match-buffers' instead
> of a bespoke loop.
>
> Thanks for working on this.
>
> --Adam

First, thank you, as I wasn't aware of buffer-local-value nor
match-buffers!

These sound a bit like micro-optimizations, how many eww buffers can one
person have open that these speed differences would matter?

And yes, I know "who would do that" is a dangerous question to ask in
the Emacs world :)

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Adam Porter]

On 10/13/24 23:06, Sebastián Monía wrote:
> 
> Hello!
> 
> Adam Porter <adam@alphapapa.net> writes:
>> Small suggestion: When possible, using BUFFER-LOCAL-VALUE is
>> preferable over WITH-CURRENT-BUFFER, as it's significantly faster (in
>> my testing). You could do this in `eww--list-buffers-get-data'.
>>
>> Also, you might want to disable the undo list in the list buffer to
>> save memory, since there's no need to undo in that buffer.
>>
>> Finally, in `eww-buffer-list', you could use `match-buffers' instead
>> of a bespoke loop.
>>
>> Thanks for working on this.
>>
>> --Adam
> 
> First, thank you, as I wasn't aware of buffer-local-value nor
> match-buffers!
> 
> These sound a bit like micro-optimizations, how many eww buffers can one
> person have open that these speed differences would matter?

I'm sure that the difference would not be noticeable.  But when 
committing new code, why not choose the most efficient implementation, 
when the size and complexity of the source code is equivalent?  Over 30 
or 40 years, little things add up.

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

Adam Porter <adam@alphapapa.net> writes:

>> These sound a bit like micro-optimizations, how many eww buffers can
>> one
>> person have open that these speed differences would matter?
>
> I'm sure that the difference would not be noticeable.  But when
> committing new code, why not choose the most efficient implementation,
> when the size and complexity of the source code is equivalent?  Over
> 30 or 40 years, little things add up.

And I was wrong, the difference doesn't depend only on the number of eww
buffers. The older version of eww-buffer-list' also used
with-current-buffer and it checked _all live buffers_.
That's potentially a lot more than only eww buffers.

Thank you!
Seb

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

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


Hello!


Jim Porter <jporterbugs@gmail.com> writes:
> Thanks for the updated patch. Aside from the things Adam has already
> mentioned

I added his suggestions in this patch.

>> +     :columns '((:name "Title" :width 30)>
> Maybe something like ':min-width "25%" :max-width "50%"' here? That
> way users can see longish page titles, and if all the titles are
> short, we give most of the space to the URLs.

Added in this patch too.

> (As an aside, I wonder how hard it would be to add support for
> 'auto-revert-mode' to the EWW buffer list.)

I was looking into it, we "just" need a good 'buffer-stale-function'.
All I could think of was to store the list used to populate the vtable
in a buffer-local variable, and check if the list has changed?

In an earlier version I used a variable to store the vtable data, but
moved to ':objects-function' because of the default binding for "g" in
vtables.


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

From 4ad41966d4ef80a7b1acaeab1b3676002cdb211f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebasti=C3=A1n=20Mon=C3=ADa?= <code@sebasmonia.com>
Date: Sun, 13 Oct 2024 21:00:03 -0400
Subject: [PATCH] Use vtable in eww-list-buffers, add function eww-buffer-list

---
 lisp/net/eww.el | 93 ++++++++++++++++++++++---------------------------
 1 file changed, 42 insertions(+), 51 deletions(-)

diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index b5d2f20781a..17ebccc9453 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -33,6 +33,7 @@
 (require 'url)
 (require 'url-queue)
 (require 'url-file)
+(require 'vtable)
 (require 'xdg)
 (eval-when-compile (require 'subr-x))
 
@@ -2604,58 +2605,47 @@ eww-history-mode
 
 ;;; eww buffers list
 
+(defun eww-buffer-list ()
+  "Return a list of all live eww buffers."
+  (match-buffers '(derived-mode . eww-mode)))
+
 (defun eww-list-buffers ()
-  "Enlist eww buffers."
+  "Pop a buffer with a list of eww buffers."
   (interactive)
-  (let (buffers-info
-        (current (current-buffer)))
-    (dolist (buffer (buffer-list))
-      (with-current-buffer buffer
-        (when (derived-mode-p 'eww-mode)
-          (push (vector buffer (plist-get eww-data :title)
-                        (plist-get eww-data :url))
-                buffers-info))))
-    (unless buffers-info
-      (error "No eww buffers"))
-    (setq buffers-info (nreverse buffers-info)) ;more recent on top
-    (set-buffer (get-buffer-create "*eww buffers*"))
+  (with-current-buffer (get-buffer-create "*eww buffers*")
     (eww-buffers-mode)
-    (let ((inhibit-read-only t)
-          (domain-length 0)
-          (title-length 0)
-          url title format start)
-      (erase-buffer)
-      (dolist (buffer-info buffers-info)
-        (setq title-length (max title-length
-                                (length (elt buffer-info 1)))
-              domain-length (max domain-length
-                                 (length (elt buffer-info 2)))))
-      (setq format (format "%%-%ds %%-%ds" title-length domain-length)
-            header-line-format
-            (concat " " (format format "Title" "URL")))
-      (let ((line 0)
-            (current-buffer-line 1))
-        (dolist (buffer-info buffers-info)
-          (setq start (point)
-                title (elt buffer-info 1)
-                url (elt buffer-info 2)
-                line (1+ line))
-          (insert (format format title url))
-          (insert "\n")
-          (let ((buffer (elt buffer-info 0)))
-            (put-text-property start (1+ start) 'eww-buffer
-                               buffer)
-            (when (eq current buffer)
-              (setq current-buffer-line line))))
-        (goto-char (point-min))
-        (forward-line (1- current-buffer-line)))))
+    (eww--list-buffers-display-table))
   (pop-to-buffer "*eww buffers*"))
 
+(defun eww--list-buffers-display-table (&optional ignore-auto noconfirm)
+  "Display a table with the list of eww buffers.
+Will remove all buffer contents first.  The parameters IGNORE-AUTO and
+NOCONFIRM are ignored, they are for compatibility with
+`revert-buffer-function'."
+  (let ((inhibit-read-only t))
+    (erase-buffer)
+    (make-vtable
+     :columns '((:name "Title" :min-width "25%" :max-width "50%")
+                (:name "URL"))
+     :objects-function #'eww--list-buffers-get-data
+     ;; use fixed-font face
+     :face 'default)))
+
+(defun eww--list-buffers-get-data ()
+  "Return the eww-data of BUF, assumed to be a eww buffer.
+The format of the data is (title url buffer), for use in of
+`eww-buffers-mode'."
+  (mapcar (lambda (buf)
+            (let ((buf-eww-data (buffer-local-value 'eww-data buf)))
+              (list (plist-get buf-eww-data :title)
+                    (plist-get buf-eww-data :url)
+                    buf)))
+          (eww-buffer-list)))
+
 (defun eww-buffer-select ()
   "Switch to eww buffer."
   (interactive nil eww-buffers-mode)
-  (let ((buffer (get-text-property (line-beginning-position)
-                                   'eww-buffer)))
+  (let ((buffer (nth 2 (vtable-current-object))))
     (unless buffer
       (error "No buffer on current line"))
     (quit-window)
@@ -2663,8 +2653,7 @@ eww-buffer-select
 
 (defun eww-buffer-show ()
   "Display buffer under point in eww buffer list."
-  (let ((buffer (get-text-property (line-beginning-position)
-                                   'eww-buffer)))
+  (let ((buffer (nth 2 (vtable-current-object))))
     (unless buffer
       (error "No buffer on current line"))
     (other-window -1)
@@ -2692,7 +2681,7 @@ eww-buffer-kill
   "Kill buffer from eww list."
   (interactive nil eww-buffers-mode)
   (let* ((start (line-beginning-position))
-	 (buffer (get-text-property start 'eww-buffer))
+	 (buffer (nth 2 (vtable-current-object)))
 	 (inhibit-read-only t))
     (unless buffer
       (user-error "No buffer on the current line"))
@@ -2711,10 +2700,9 @@ eww-buffers-mode-map
   :menu '("Eww Buffers"
           ["Exit" quit-window t]
           ["Select" eww-buffer-select
-           :active (get-text-property (line-beginning-position) 'eww-buffer)]
+           :active (nth 2 (vtable-current-object))]
           ["Kill" eww-buffer-kill
-           :active (get-text-property (line-beginning-position)
-                                      'eww-buffer)]))
+           :active (nth 2 (vtable-current-object))]))
 
 (define-derived-mode eww-buffers-mode special-mode "eww buffers"
   "Mode for listing buffers.
@@ -2722,7 +2710,10 @@ eww-buffers-mode
 \\{eww-buffers-mode-map}"
   :interactive nil
   (buffer-disable-undo)
-  (setq truncate-lines t))
+  (setq truncate-lines t
+        ;; this is set so that pressing "g" with point just below the
+        ;; table will still update the listing
+        revert-buffer-function #'eww--list-buffers-display-table))
 
 ;;; Desktop support
 
-- 
2.43.0

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Adam Porter]

On 10/14/24 21:59, Sebastián Monía wrote:
> I was looking into it, we "just" need a good 'buffer-stale-function'.
> All I could think of was to store the list used to populate the vtable
> in a buffer-local variable, and check if the list has changed?

AFAIK it's generally good to avoid adding more buffer-local variables, 
as their presence has a performance penalty in general.

Updating a buffer list buffer automatically could be done by hooking 
into the machinery that renders a single EWW buffer and having it update 
a buffer list buffer if one exists.  But I guess that wouldn't work 
exactly like `auto-revert-mode` and `buffer-stale-function`.

Another option could be to use a special, non-buffer-local variable to 
store 1) a timestamp of the last time any EWW buffer was updated, then 
update that timestamp in an EWW post-render hook (haven't looked for 
appropriate one), and 2) the time that the list buffer was last updated 
(in the same variable, using e.g. an alist), and compare the timestamps 
in the buffer-stale-function.

I don't mean to say what you should do; just throwing out some ideas.  :)

--Adam

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Adam Porter]

On 10/14/24 21:39, Sebastián Monía wrote:
> Adam Porter <adam@alphapapa.net> writes:
> 
>>> These sound a bit like micro-optimizations, how many eww buffers can
>>> one
>>> person have open that these speed differences would matter?
>>
>> I'm sure that the difference would not be noticeable.  But when
>> committing new code, why not choose the most efficient implementation,
>> when the size and complexity of the source code is equivalent?  Over
>> 30 or 40 years, little things add up.
> 
> And I was wrong, the difference doesn't depend only on the number of eww
> buffers. The older version of eww-buffer-list' also used
> with-current-buffer and it checked _all live buffers_.
> That's potentially a lot more than only eww buffers.

Yeah, iterating over `buffer-list' should be done thoughtfully.  :) 
It's also easy to overlook extra consing while doing so, e.g. doing a 
regexp match on each buffer's major-mode as a string (sometimes that 
seems unavoidable without, e.g. setting up a mode for all of a library's 
modes to inherit from so DERIVED-MODE-P could be used).

> Thank you!

:)

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

I just realized that I sent this only to Adam instead of the
mailing list. 

My assignment did come through, and I still think there's value
in merging the current patch (without auto-reverting).

On Thu, Oct 17, 2024, at 10:37 AM, Sebastián Monía wrote:
> Adam Porter <adam@alphapapa.net> writes:
> 
> > On 10/14/24 21:59, Sebastián Monía wrote:
> >> I was looking into it, we "just" need a good 'buffer-stale-function'.
> >> All I could think of was to store the list used to populate the vtable
> >> in a buffer-local variable, and check if the list has changed?
> >
> > AFAIK it's generally good to avoid adding more buffer-local variables,
> > as their presence has a performance penalty in general.
> 
> Noted!
> 
> > Updating a buffer list buffer automatically could be done by hooking
> > into the machinery that renders a single EWW buffer and having it
> > update a buffer list buffer if one exists.  But I guess that wouldn't
> > work exactly like `auto-revert-mode` and `buffer-stale-function`.
> 
> That same machinery could trigger auto-revert by marking the "buffer list"
> buffer stale.
> 
> > I don't mean to say what you should do; just throwing out some ideas.  :)
> 
> And it is much appreciated! :)
> 
> I don't think we should hold on merging this for auto-reverting though.
> The patch is big enough as it is.
> IMHO.
> 
> 
> PS: still can't merge as my assignment didn't come through.
> 
> -- 
> Sebastián Monía
> https://site.sebasmonia.com/

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Jim Porter]

On 10/19/2024 1:51 PM, Sebastián Monía wrote:
> My assignment did come through, and I still think there's value
> in merging the current patch (without auto-reverting).

Thanks for the update. Your patch looks good, so I merged it to the 
master branch as d3975cc925a. I added the usual ChangeLog format to your 
commit message, so you can take a look at how I did that for your future 
commits; it makes merging patches a bit easier.

(There are ways to generate a ChangeLog skeleton in Emacs, but the way I 
do it is pretty dependent on other customizations in my config so I 
don't think I could provide any useful advice here. Others might be able 
to help though.)

Re: [PATCH] EWW - use revert--buffer-function to reload, and allow reload in eww-list-buffer

[Sebastián Monía]

Jim Porter <jporterbugs@gmail.com> writes:

> Thanks for the update. Your patch looks good, so I merged it to the
> master branch as d3975cc925a. I added the usual ChangeLog format to
> your commit message, so you can take a look at how I did that for your
> future commits; it makes merging patches a bit easier.

Thank you, will check it out and keep that in mind for future patches!


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