bug#59212: 29.0.50; [PATCH] Set default value of bookmark Type

bug#59212: 29.0.50; [PATCH] Set default value of bookmark Type

[Gabriel]

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

Description:
Not a bug per se, but a suggestion for improvement in how Bookmark
displays the Type (added by commit 7c995264359824cc1aca40ca37201db5ed44e659).

Currently, bookmark-bmenu-list (C-x r l) displays a column named Type,
which has a proper value for all handlers (e.g.: Eshell, VC, EWW etc)
except for regular files, in which an empty string is displayed. I
believe the current behavior could be improved by displaying a default
value ("Files") to make the interface more uniform, explicit and to
avoid confusion.

Steps:
1) emacs -Q
2) M-x eshell
3) C-x r m <RET>
4) C-x C-f ~/foo.bar
5) C-x r m <RET>
6) C-x r b

These steps will add two bookmarks (eshell and foo.bar) and will open
the Bookmarks list. The Type column will have values "" and
"Eshell". After the patch below is applied, the Type column will have
values "File" and "Eshell".


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Set-default-value-of-bookmark-Type.patch --]
[-- Type: text/x-diff, Size: 1507 bytes --]

From 0d9379e2d470239e2da6369d27979672ab10a719 Mon Sep 17 00:00:00 2001
From: Gabriel do Nascimento Ribeiro <gabriel376@hotmail.com>
Date: Sat, 12 Nov 2022 05:13:36 -0300
Subject: [PATCH 1/1] Set default value of bookmark Type

* lisp/bookmark.el (bookmark-type-from-full-record): Return default
  value "File" when type handler is missing.
  (bookmark-bmenu--revert): Remove conditional around type.
---
 lisp/bookmark.el | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index b57ad12986..ad1a1f1acf 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -372,9 +372,9 @@ bookmark-type-from-full-record
   (let ((handler (bookmark-get-handler bookmark-record)))
     (when (autoloadp (symbol-function handler))
       (autoload-do-load (symbol-function handler)))
-    (if (symbolp handler)
-        (get handler 'bookmark-handler-type)
-     "")))
+    (or (and (symbolp handler)
+             (get handler 'bookmark-handler-type))
+        "File")))
 
 (defun bookmark-all-names ()
   "Return a list of all current bookmark names."
@@ -1889,7 +1889,7 @@ bookmark-bmenu--revert
                                   'follow-link t
                                   'help-echo "mouse-2: go to this bookmark in other window")
                     name)
-                 ,(or type "")
+                 ,type
                  ,@(if bookmark-bmenu-toggle-filenames
                        (list location))])
               entries)))
-- 
2.34.1


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


---
Gabriel

bug#59212: 29.0.50; [PATCH] Set default value of bookmark Type

[Eli Zaretskii]

> From: Gabriel <gabriel376@hotmail.com>
> Date: Sat, 12 Nov 2022 05:56:22 -0300
> 
> Description:
> Not a bug per se, but a suggestion for improvement in how Bookmark
> displays the Type (added by commit 7c995264359824cc1aca40ca37201db5ed44e659).
> 
> Currently, bookmark-bmenu-list (C-x r l) displays a column named Type,
> which has a proper value for all handlers (e.g.: Eshell, VC, EWW etc)
> except for regular files, in which an empty string is displayed. I
> believe the current behavior could be improved by displaying a default
> value ("Files") to make the interface more uniform, explicit and to
> avoid confusion.
> 
> Steps:
> 1) emacs -Q
> 2) M-x eshell
> 3) C-x r m <RET>
> 4) C-x C-f ~/foo.bar
> 5) C-x r m <RET>
> 6) C-x r b
> 
> These steps will add two bookmarks (eshell and foo.bar) and will open
> the Bookmarks list. The Type column will have values "" and
> "Eshell". After the patch below is applied, the Type column will have
> values "File" and "Eshell".

Thanks.

Karl, any comments?

bug#59212: 29.0.50; [PATCH] Set default value of bookmark Type

[Stefan Kangas]

severity 59212 wishlist
thanks

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gabriel <gabriel376@hotmail.com>
>> Date: Sat, 12 Nov 2022 05:56:22 -0300
>>
>> Description:
>> Not a bug per se, but a suggestion for improvement in how Bookmark
>> displays the Type (added by commit 7c995264359824cc1aca40ca37201db5ed44e659).
>>
>> Currently, bookmark-bmenu-list (C-x r l) displays a column named Type,
>> which has a proper value for all handlers (e.g.: Eshell, VC, EWW etc)
>> except for regular files, in which an empty string is displayed. I
>> believe the current behavior could be improved by displaying a default
>> value ("Files") to make the interface more uniform, explicit and to
>> avoid confusion.
>>
>> Steps:
>> 1) emacs -Q
>> 2) M-x eshell
>> 3) C-x r m <RET>
>> 4) C-x C-f ~/foo.bar
>> 5) C-x r m <RET>
>> 6) C-x r b
>>
>> These steps will add two bookmarks (eshell and foo.bar) and will open
>> the Bookmarks list. The Type column will have values "" and
>> "Eshell". After the patch below is applied, the Type column will have
>> values "File" and "Eshell".
>
> Thanks.
>
> Karl, any comments?

I'm not Karl, but I've experimented with something similar before and I
found two things:

1. I had to add a type for Dired bookmarks (which are not Files) to
   avoid confusion.

2. Using a default name (I also used "Files", IIRC), just made my long
   list of bookmarks more busy with little benefit.  If anything, it
   made it harder to navigate, as it was harder to make out the
   "special" types of bookmarks.

So all in all, I was not a huge fan of the change.

If we want to add this, I suggest adding a defcustom so that users can
enable or disable this functionality to their liking (or change "Files"
into something else).  I might prefer to disable it, for example.

bug#59212: 29.0.50; [PATCH] Set default value of bookmark Type

[Gabriel]

Stefan Kangas <stefankangas@gmail.com> writes:
> I'm not Karl, but I've experimented with something similar before and I
> found two things:
>
> 1. I had to add a type for Dired bookmarks (which are not Files) to
>    avoid confusion.
>
> 2. Using a default name (I also used "Files", IIRC), just made my long
>    list of bookmarks more busy with little benefit.  If anything, it
>    made it harder to navigate, as it was harder to make out the
>    "special" types of bookmarks.
>
> So all in all, I was not a huge fan of the change.
>
> If we want to add this, I suggest adding a defcustom so that users can
> enable or disable this functionality to their liking (or change "Files"
> into something else).  I might prefer to disable it, for example.

1. Good point about Dired. Would be nice to have a Type for regular
files and another one for directories. Is there any approach to
differentiate files from directories that do not rely on IO calls? Some
kind of portable string-check, I guess. I am aware of 'file-directory-p'
and 'file-regular-p', but both will try to read FILENAME, which could
degrade performance in case of long Bookmark lists (and there is also
Tramp).

2. Thanks for the inputs. It's not clear for me what do you mean by
"more busy" or "harder to navigate". How do you normally navigate in
long Bookmark lists? What are the use-cases where a visual distinction
between empty and non-empty strings of the Type column is useful? What
about the distinction between Types Eshell, DocView and VC, where the
user has to read the contents of the Type column?

I am against adding a new defcustom for such trivial thing, Emacs has
already too much. I think this proposed change is a better default to
novices, and advanced users can add an advice around
bookmark-type-from-full-record. If we want to customize this behavior, I
propose something like a defcustom that offers an alist of
'((TYPE . DISPLAY-STRING)).

I also suggest to add a new 'type choice to bookmark-sort-flag, which I
can do in a new wishlist.

---
Gabriel