Emacs 28 bug preventing bookmark deletion

[tpeacock19]

I'm on the master branch of emacs and have noticed I'm unable to delete Burly bookmarks. I've tracked it to this commit in emacs from May of this year https://github.com/emacs-mirror/emacs/commit/ab6cb65cb2b6d11a7b690dfcea8d98611290fad9.

bookmark-delete calls bookmark--unfontify which expects the bookmark to have a filename and position value in the alist. When this the filename is nil it matches with non-file buffers and errors out when evaluating (overlays-at pos).

(defun bookmark--unfontify (bm)
  "Remove a bookmark's colorized overlay.
BM is a bookmark as returned from function `bookmark-get-bookmark'.
See user option `bookmark-fontify'."
  (let ((filename (assq 'filename bm))
        (pos      (assq 'position bm))
        overlays found temp)
    (when filename (setq filename (expand-file-name (cdr filename))))
    (when pos (setq pos (cdr pos)))
    (dolist (buf (buffer-list))
      (with-current-buffer buf
        (when (equal filename buffer-file-name)
          (setq overlays (overlays-at pos))
          (while (and (not found) (setq temp (pop overlays)))
            (when (eq 'bookmark (overlay-get temp 'category))
              (delete-overlay (setq found temp)))))))))

I've made a quick workaround by adding empty strings for the filename value of burly bookmarks. I'm not sure if this should be addressed in Burly or with the Emacs itself, but figured I would bring to your attention.

diff --git a/burly.el b/burly.el
index a18555b..3662b23 100644
--- a/burly.el
+++ b/burly.el
@@ -165,7 +165,8 @@ a project."
    (list (completing-read "Save Burly bookmark: " (burly-bookmark-names)
                           nil nil burly-bookmark-prefix)))
   (let ((record (list (cons 'url (burly-frames-url))
-                      (cons 'handler #'burly-bookmark-handler))))
+                      (cons 'handler #'burly-bookmark-handler)
+                      (cons 'filename ""))))
     (bookmark-store name record nil)))

 ;;;###autoload
@@ -175,7 +176,8 @@ a project."
    (list (completing-read "Save Burly bookmark: " (burly-bookmark-names)
                           nil nil burly-bookmark-prefix)))
   (let ((record (list (cons 'url (burly-windows-url))
-                      (cons 'handler #'burly-bookmark-handler))))
+                      (cons 'handler #'burly-bookmark-handler)
+                      (cons 'filename ""))))
     (bookmark-store name record nil)))

 ;;;###autoload

[alphapapa]

I guess you mean that you're unable to delete them in the list-bookmarks buffer, right? If you just call M-x bookmark-delete, does it work?

I guess this is a question of the bookmarks API: are bookmarks required to have a non-nil filename now? ISTM that nil should be an acceptable value, because there may not be a filename (even for non-Burly bookmarks, e.g. a bookmarked, non-file-backed buffer).

So this seems like a bug in Emacs to me. It should probably be a simple fix relative to the change that introduced it. Would you mind reporting the bug to Emacs?

Edit: Looking at the code in that function, it looks like a simple oversight to me that should be fixed in that function. It probably also affects some bookmarks other than ones created by Burly.

[tpeacock19]

I believe actually believe that bookmarks may be expected to always have a position? If the filename is nil then it calls (overlays-at pos) and that is the function that returns a (wrong-type-argument integer-or-marker-p nil) error.

I'm not familiar enough with all types of bookmarks to determine if they should or should not have a position; though I agree on the filename.

[alphapapa]

I'm guessing that there is no formal definition of what bookmark fields are allowed to have nil values. And I'm guessing that, in the wild, anything goes, anyway. So I think these fontification functions added in that commit need to handle nil values for these fields. It's probably just a simple oversight that they don't already.

[tpeacock19]

I understand, I'll submit a report shortly.

[alphapapa]

Thanks. Please let me know the link so I can follow along.

[tpeacock19]

The link is bug#49341

[alphapapa]

Thanks. I see from the subsequently linked https://debbugs.gnu.org/cgi/bugreport.cgi?bug=48179 that this problem was expected, but whose responsibility it is to fix it seems to be in dispute. Hopefully it is fixed before Emacs 28 is released, but there's plenty of time before then.

[alphapapa]

Since that confirms that there is no bug here, I'm closing this issue, but feel free to follow up with more information.

[tpeacock19]

This has been fixed on the master branch https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-07/msg00244.html

[alphapapa]

Great news. Thanks for taking the initiative on this. It's nice to get these fixed ahead of time rather than finding out after Emacs releases. :)