[PATCH] When deleting in bookmark menu, prompt for confirmation.

[PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

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

I wanted to run this by folks here to get opinions/review before 
committing.  It stems from a suggestion by Oliver Taylor in a 
thread on the Emacs Humanites mailing list (the thread is linked 
to from the commit message).

If you prefer to view this patch in the GitLab diff-browsing 
interface, it's here:

https://code.librehq.com/kfogel/emacs/-/commit/73b688abbb20ce48a7a5ea69e53e8a8bb6b279fc

Best regards,
-Karl


[-- Attachment #2: bookmark-menu-deletion-confirmation-patch.txt --]
[-- Type: text/plain, Size: 4263 bytes --]

[[[
In bookmark menu, offer prompt to confirm deletion

* src/emacs/lisp/bookmark.el
  (bookmark-menu-confirm-deletion): New defcustom.
  (bookmark-bmenu-execute-deletions): Conditionally confirm.
  (bookmark-delete-all): Add comment explaining why we don't
    use the new confirmation formula here.

(Note: the bulk of the code diff is just reindentation of an otherwise
unchanged `let' expression in `bookmark-bmenu-execute-deletions'.)

Thanks to Oliver Taylor for suggesting the new behavior:

  https://lists.gnu.org/archive/html/emacs-humanities/2021-02/msg00022.html
  From: Oliver Taylor
  Subject: Re: [emacs-humanities] Extending Emacs Bookmarks to Work with EWW
  To: Karl Fogel
  Cc: Stefan Kangas, Emacs-humanities mailing list
  Date: Wed, 3 Feb 2021 20:21:59 -0800
  Message-Id: <936D47EA-4D11-452B-8303-971B6386877B@me.com>
]]]

--- lisp/bookmark.el
+++ lisp/bookmark.el
@@ -121,6 +121,12 @@ bookmark-sort-flag
   :type 'boolean)
 
 
+(defcustom bookmark-menu-confirm-deletion t
+  "Non-nil means prompt for confirmation when executing the deletion
+of bookmarks marked for deletion in a bookmark menu buffer.  Nil
+means don't prompt for confirmation."
+  :type 'boolean)
+
 (defcustom bookmark-automatically-show-annotations t
   "Non-nil means show annotations when jumping to a bookmark."
   :type 'boolean)
@@ -1376,6 +1382,13 @@ bookmark-delete-all
 If optional argument NO-CONFIRM is non-nil, don't ask for
 confirmation."
   (interactive "P")
+  ;; We don't use `bookmark-menu-confirm-deletion' here because that
+  ;; variable is specifically to control confirmation prompting in a
+  ;; bookmark menu buffer, where the user has the marked-for-deletion
+  ;; bookmarks arrayed in front of them and might have accidentally
+  ;; hit the key that executes the deletions.  The UI situation here
+  ;; is quite different, by contrast: the user got to this point by a
+  ;; sequence of keystrokes unlikely to be typed by chance.
   (when (or no-confirm
             (yes-or-no-p "Permanently delete all bookmarks? "))
     (bookmark-maybe-load-default-file)
@@ -2142,30 +2155,35 @@ bookmark-bmenu-delete-all
 
 
 (defun bookmark-bmenu-execute-deletions ()
-  "Delete bookmarks flagged `D'."
+  "Delete bookmarks flagged `D'.
+If `bookmark-menu-confirm-deletion' is non-nil, prompt for
+confirmation first."
   (interactive nil bookmark-bmenu-mode)
-  (let ((reporter (make-progress-reporter "Deleting bookmarks..."))
-        (o-point  (point))
-        (o-str    (save-excursion
-                    (beginning-of-line)
-                    (unless (= (following-char) ?D)
-                      (buffer-substring
-                       (point)
-                       (progn (end-of-line) (point))))))
-        (o-col     (current-column)))
-    (goto-char (point-min))
-    (while (re-search-forward "^D" (point-max) t)
-      (bookmark-delete (bookmark-bmenu-bookmark) t)) ; pass BATCH arg
-    (bookmark-bmenu-list)
-    (if o-str
-        (progn
-          (goto-char (point-min))
-          (search-forward o-str)
-          (beginning-of-line)
-          (forward-char o-col))
-      (goto-char o-point))
-    (beginning-of-line)
-    (progress-reporter-done reporter)))
+  (if (and bookmark-menu-confirm-deletion
+           (not (yes-or-no-p "Delete selected bookmarks? ")))
+      (message "Bookmarks not deleted.")
+    (let ((reporter (make-progress-reporter "Deleting bookmarks..."))
+          (o-point  (point))
+          (o-str    (save-excursion
+                      (beginning-of-line)
+                      (unless (= (following-char) ?D)
+                        (buffer-substring
+                         (point)
+                         (progn (end-of-line) (point))))))
+          (o-col     (current-column)))
+      (goto-char (point-min))
+      (while (re-search-forward "^D" (point-max) t)
+        (bookmark-delete (bookmark-bmenu-bookmark) t)) ; pass BATCH arg
+      (bookmark-bmenu-list)
+      (if o-str
+          (progn
+            (goto-char (point-min))
+            (search-forward o-str)
+            (beginning-of-line)
+            (forward-char o-col))
+        (goto-char o-point))
+      (beginning-of-line)
+      (progress-reporter-done reporter))))
 
 
 (defun bookmark-bmenu-rename ()

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Lars Ingebrigtsen]

Karl Fogel <kfogel@red-bean.com> writes:

> I wanted to run this by folks here to get opinions/review before
> committing.  It stems from a suggestion by Oliver Taylor in a thread
> on the Emacs Humanites mailing list (the thread is linked to from the
> commit message).

[...]

>   (bookmark-menu-confirm-deletion): New defcustom.

In general, I think it's better to offer undo instead of prompting.
That's not always possible, but I think in the bookmark case, that
shouldn't be too difficult -- just register an undo action that inserts
the items that were removed, I think?

But if we want to do the prompt, just a tiny comment:

> +(defcustom bookmark-menu-confirm-deletion t
> +  "Non-nil means prompt for confirmation when executing the deletion
> +of bookmarks marked for deletion in a bookmark menu buffer.  Nil
> +means don't prompt for confirmation."
> +  :type 'boolean)

:version should be added, and the first line should be a complete
sentence.  (And "nil" shouldn't be capitalised, even if it starts the
sentence.) 

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Karl Fogel <kfogel@red-bean.com> writes:
>
>> I wanted to run this by folks here to get opinions/review before
>> committing.  It stems from a suggestion by Oliver Taylor in a thread
>> on the Emacs Humanites mailing list (the thread is linked to from the
>> commit message).
>
> [...]
>
>>   (bookmark-menu-confirm-deletion): New defcustom.
>
> In general, I think it's better to offer undo instead of prompting.
> That's not always possible, but I think in the bookmark case, that
> shouldn't be too difficult -- just register an undo action that inserts
> the items that were removed, I think?

I think a confirmation here is in order, as this is a dangerous
operation.  For the same reason, we should ask the user to disable the
prompt rather than the other way around, i.e. the default should be t.

Undoing is also fine, but it is less discoverable, as you would need to
know that an action can be undone to use it.  OTOH, we could call it out
in the major mode help.

Perhaps if we had undo, it would make more sense to have the
confirmation nil by default.  (But I do wonder if many users would
then bother setting that option to t.)

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Stefan Monnier]

>> In general, I think it's better to offer undo instead of prompting.
>> That's not always possible, but I think in the bookmark case, that
>> shouldn't be too difficult -- just register an undo action that inserts
>> the items that were removed, I think?

Sounds like a good idea, yes.

> I think a confirmation here is in order, as this is a dangerous
> operation.  For the same reason, we should ask the user to disable the
> prompt rather than the other way around, i.e. the default should be t.

I don't have an opinion, but having undo would be beneficial in any case
(i.e. regardless of the confirmation).


        Stefan

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Lars Ingebrigtsen]

Stefan Kangas <stefankangas@gmail.com> writes:

> Undoing is also fine, but it is less discoverable, as you would need to
> know that an action can be undone to use it.  OTOH, we could call it out
> in the major mode help.

It's not obvious that such an action is reversible, no.  But the
bookmark deletion command could just say "C-u to undo" at the end.

> Perhaps if we had undo, it would make more sense to have the
> confirmation nil by default.  (But I do wonder if many users would
> then bother setting that option to t.)

If we have undo, adding confirmation wouldn't be very useful, no.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

RE: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Drew Adams]

> > Undoing is also fine, but it is less discoverable, as you would need to
> > know that an action can be undone to use it.  OTOH, we could call it out
> > in the major mode help.
> 
> It's not obvious that such an action is reversible, no.  But the
> bookmark deletion command could just say "C-u to undo" at the end.

That doesn't really cut the mustard, IMO.

If a `D' flag was off the window, you won't even
know whether that file was deleted, so you won't
know whether you want to undo.  At best, you'll
see a message telling you how many files were
deleted.

Plus, just what does undo do?  Will a user be
assured that _all_ deletions were undone?  Undo
does things in packets.  A user can easily worry
that only some of the deletions were undone.

> > Perhaps if we had undo, it would make more sense to have the
> > confirmation nil by default.  (But I do wonder if many users would
> > then bother setting that option to t.)
> 
> If we have undo, adding confirmation wouldn't be very useful, no.

Undo is no substitute for prompting for confirmation, IMO.

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Date: Sun, 02 May 2021 22:43:03 -0500
> 
> I wanted to run this by folks here to get opinions/review before 
> committing.  It stems from a suggestion by Oliver Taylor in a 
> thread on the Emacs Humanites mailing list (the thread is linked 
> to from the commit message).

Isn't this a backward-incompatible change in behavior?  If it is, then
(a) I'd prefer the default to remain as it was before, and (b) the
change should be called out in NEWS.

Thanks.

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> I wanted to run this by folks here to get opinions/review before
>> committing.  It stems from a suggestion by Oliver Taylor in a
>> thread on the Emacs Humanites mailing list (the thread is linked
>> to from the commit message).
>
> Isn't this a backward-incompatible change in behavior?  If it is, then
> (a) I'd prefer the default to remain as it was before, and (b) the
> change should be called out in NEWS.

I don't see how backwards-compatibility is relevant here, but perhaps
you could provide more specifics.

IMO, we should do what we always do: enable this new feature by default
if it is generally useful, and disable it if it is not.  If someone does
not like the change, we already have an option in place to disable it.

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Mon, 3 May 2021 07:47:50 -0500
> Cc: emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Isn't this a backward-incompatible change in behavior?  If it is, then
> > (a) I'd prefer the default to remain as it was before, and (b) the
> > change should be called out in NEWS.
> 
> I don't see how backwards-compatibility is relevant here, but perhaps
> you could provide more specifics.

We are now asking for confirmation where previously no question was
asked, or am I missing something?

> IMO, we should do what we always do: enable this new feature by default
> if it is generally useful, and disable it if it is not.  If someone does
> not like the change, we already have an option in place to disable it.

Questions that pop where previously Emacs did something silently can
be annoying, which is why I'd prefer to turn this off by default.  If
someone wants the new behavior, they can enable it.

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> I don't see how backwards-compatibility is relevant here, but perhaps
>> you could provide more specifics.
>
> We are now asking for confirmation where previously no question was
> asked, or am I missing something?

My point is that backwards compatibility is not necessarily the most
important consideration in this case, and that such a stance would at
least need some elaborating.

AFAICT, we routinely make "backward incompatible" changes where we see
a need for it.  (But of course we do them with care and consideration.)

Luckily, you provided your reasoning below:

> Questions that pop where previously Emacs did something silently can
> be annoying, which is why I'd prefer to turn this off by default.  If
> someone wants the new behavior, they can enable it.

That's fair enough.  To me, the danger of accidentally deleting a
bookmarks still seems more important.  On the one hand, we have lived
with that for a long time, OTOH that is no reason not to try to do
better.

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Mon, 3 May 2021 10:43:29 -0500
> Cc: kfogel@red-bean.com, emacs-devel@gnu.org
> 
> > Questions that pop where previously Emacs did something silently can
> > be annoying, which is why I'd prefer to turn this off by default.  If
> > someone wants the new behavior, they can enable it.
> 
> That's fair enough.  To me, the danger of accidentally deleting a
> bookmarks still seems more important.  On the one hand, we have lived
> with that for a long time, OTOH that is no reason not to try to do
> better.

My point is that it is safer to start with the existing behavior, and
"do better" as an opt-in feature.  Later, if enough people like this
new behavior, we can make it the default.

Changing the default behavior is 100% justified only when the old
behavior is a clear bug, IMO.

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

On 03 May 2021, Stefan Kangas wrote:
>Eli Zaretskii <eliz@gnu.org> writes:
>
>>> I don't see how backwards-compatibility is relevant here, but 
>>> perhaps
>>> you could provide more specifics.
>>
>> We are now asking for confirmation where previously no question 
>> was
>> asked, or am I missing something?
>
>My point is that backwards compatibility is not necessarily the 
>most
>important consideration in this case, and that such a stance 
>would at
>least need some elaborating.
>
>AFAICT, we routinely make "backward incompatible" changes where 
>we see
>a need for it.  (But of course we do them with care and 
>consideration.)
>
>Luckily, you provided your reasoning below:
>
>> Questions that pop where previously Emacs did something 
>> silently can
>> be annoying, which is why I'd prefer to turn this off by 
>> default.  If
>> someone wants the new behavior, they can enable it.
>
>That's fair enough.  To me, the danger of accidentally deleting a
>bookmarks still seems more important.  On the one hand, we have 
>lived
>with that for a long time, OTOH that is no reason not to try to 
>do
>better.

I agree with Stefan's reasoning here.

Backward compatibility is a big deal in an API, but it's a much 
smaller deal in an interactive interface behavior.  Yes, users 
will be prompted in a place where they weren't prompted before, 
but the prompt is self-explanatory, and the old behavior was 
needlessly dangerous -- it's easy to type "x" accidentally and 
lose bookmarks marked for deletion before one had finalized the 
list.

If someone had suggested this when I was first implementing the 
Bookmark Menu, I think I would have incorporated the suggestion 
then and just had this behavior from the beginning.  The fact that 
it's a change in interactive behavior now is minor compared to the 
benefit.

I agree that it should get a NEWS entry, of course, and agree with 
Lars' suggestions of `:version' and some documentation tweaks. 

Regarding the question of using an undo-based method instead:

Lars Ingebrigtsen wrote:
>In general, I think it's better to offer undo instead of 
>prompting.
>That's not always possible, but I think in the bookmark case, 
>that
>shouldn't be too difficult -- just register an undo action that 
>inserts
>the items that were removed, I think?
 
I think that's not as good as the confirmation-prompt here, for 
two reasons.  One, the discoverability issue raised by Stefan 
Kangas in his reply; two, Bookmark Menu has a similar basic 
look-and-feel to Dired Mode, with which many users are probably 
already familiar, and Dired does exactly this kind of confirmation 
prompt by default for deleting files.  If Bookmark just behaves 
the way Dired does by default, that would be consistent with [at 
least some] users' expectations.

Best regards,
-Karl

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Mon, 03 May 2021 12:21:36 -0500
> 
> >That's fair enough.  To me, the danger of accidentally deleting a
> >bookmarks still seems more important.  On the one hand, we have
> >lived with that for a long time, OTOH that is no reason not to try
> >to do better.
> 
> I agree with Stefan's reasoning here.
> 
> Backward compatibility is a big deal in an API, but it's a much 
> smaller deal in an interactive interface behavior.  Yes, users 
> will be prompted in a place where they weren't prompted before, 
> but the prompt is self-explanatory, and the old behavior was 
> needlessly dangerous -- it's easy to type "x" accidentally and 
> lose bookmarks marked for deletion before one had finalized the 
> list.

Noted.  But we are not going to change the default behavior in
incompatible ways on my watch, sorry.

> If someone had suggested this when I was first implementing the 
> Bookmark Menu, I think I would have incorporated the suggestion 
> then and just had this behavior from the beginning.

That ship has sailed; we cannot change the past.  The sheer amount of
time that the current behavior was the default gives weight to it that
cannot be countermanded by reasoning.

> The fact that it's a change in interactive behavior now is minor
> compared to the benefit.

I understand that this is your opinion, but I've heard enough
complaints from veteran users about annoying behavior changes forced
upon them (and had my own share of such annoyances) that I'm firmly
against making such changes opt-out.

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Jim Porter]

On 5/3/2021 10:41 AM, Eli Zaretskii wrote:
>> From: Karl Fogel <kfogel@red-bean.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
>> Date: Mon, 03 May 2021 12:21:36 -0500
>>
>> Backward compatibility is a big deal in an API, but it's a much
>> smaller deal in an interactive interface behavior.  Yes, users
>> will be prompted in a place where they weren't prompted before,
>> but the prompt is self-explanatory, and the old behavior was
>> needlessly dangerous -- it's easy to type "x" accidentally and
>> lose bookmarks marked for deletion before one had finalized the
>> list.
> 
> Noted.  But we are not going to change the default behavior in
> incompatible ways on my watch, sorry.

Just a random musing: would there be any sense in adding a global option 
like `prefer-conservative-defaults' to Emacs 28 and mentioning it in the 
NEWS so that, come Emacs 29, developers could make modestly 
backwards-incompatible changes like this while keeping the old behavior 
for people with `prefer-conservative-defaults' set to t (which they 
hopefully set upon the release of Emacs 28)?

One downside I see right off the bat is that 
`prefer-conservative-defaults' == t would roughly mean "keep Emacs like 
it was in version 28", but a decade from now, someone might prefer Emacs 
38 and want conservative defaults starting then. I suppose 
`prefer-conservative-defaults' could specify a preferred version, and 
`defcustom' could allow for multiple default values defined by 
particular version ranges.

Either of these solutions would increase the maintenance burden somewhat 
for those variables (the second especially), but no one is *required* to 
make backwards-incompatible changes, so I'd hope this is only done when 
a developer truly believes that the new behavior is better, and is 
worried that existing users won't want to change.

This is somewhat academic for me, since I'm an existing user, and I 
always read the NEWS upon upgrading so I know what, if anything, I'd 
like to tweak. However, something like this could make it easier to 
improve Emacs in ways that would make it more attractive to a new user.

Like I said, it's just a thought...

- Jim

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Eli Zaretskii]

> From: Jim Porter <jporterbugs@gmail.com>
> Date: Mon, 3 May 2021 11:28:12 -0700
> 
> Just a random musing: would there be any sense in adding a global option 
> like `prefer-conservative-defaults' to Emacs 28 and mentioning it in the 
> NEWS so that, come Emacs 29, developers could make modestly 
> backwards-incompatible changes like this while keeping the old behavior 
> for people with `prefer-conservative-defaults' set to t (which they 
> hopefully set upon the release of Emacs 28)?

That requires that those users know about that option and turn it on
the moment they install the new Emacs.  I'm not sure I understand why
we should assume such an option will be known.

> One downside I see right off the bat is that 
> `prefer-conservative-defaults' == t would roughly mean "keep Emacs like 
> it was in version 28", but a decade from now, someone might prefer Emacs 
> 38 and want conservative defaults starting then. I suppose 
> `prefer-conservative-defaults' could specify a preferred version, and 
> `defcustom' could allow for multiple default values defined by 
> particular version ranges.

You make it sound as if defaults never change in Emacs.  That is
definitely not true: we do change them, after we receive feedback from
users who opt in to the new behavior.

Also, a behavior can be incompatible only where existing features are
considered.  New features cannot by definition behave incompatibly,
because there's nothing to compare them to.  So compatible behavior
does not mean that Emacs 38 will be the same as Emacs 28: it will have
many new features and packages that 28 never had.

> Either of these solutions would increase the maintenance burden somewhat 
> for those variables (the second especially), but no one is *required* to 
> make backwards-incompatible changes, so I'd hope this is only done when 
> a developer truly believes that the new behavior is better, and is 
> worried that existing users won't want to change.

I don't think I agree with that assessment.  Incompatible behaviors
are also introduced because each one of us has personal preferences.
So "truly believes" is many times in the eyes of the beholder.

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Jim Porter]

On Mon, May 3, 2021 at 11:45 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Jim Porter <jporterbugs@gmail.com>
> > Date: Mon, 3 May 2021 11:28:12 -0700
> >
> > One downside I see right off the bat is that
> > `prefer-conservative-defaults' == t would roughly mean "keep Emacs like
> > it was in version 28", but a decade from now, someone might prefer Emacs
> > 38 and want conservative defaults starting then. I suppose
> > `prefer-conservative-defaults' could specify a preferred version, and
> > `defcustom' could allow for multiple default values defined by
> > particular version ranges.
>
> You make it sound as if defaults never change in Emacs.  That is
> definitely not true: we do change them, after we receive feedback from
> users who opt in to the new behavior.

That's fair. So long as there's a path to change defaults if
maintainers are confident about it, then I have no concerns.

As for the feature itself, I'd probably opt in to it, since I only
lightly use bookmarks, and having it work more like dired would reduce
confusion for me. If some version of this patch merges, I'll be sure
to provide feedback if I notice any hiccups.

- Jim

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

On 03 May 2021, Eli Zaretskii wrote:
>Noted.  But we are not going to change the default behavior in
>incompatible ways on my watch, sorry.
>
>> If someone had suggested this when I was first implementing the 
>> Bookmark Menu, I think I would have incorporated the suggestion 
>> then and just had this behavior from the beginning.
>
>That ship has sailed; we cannot change the past.  The sheer 
>amount of
>time that the current behavior was the default gives weight to it 
>that
>cannot be countermanded by reasoning.
>
>> The fact that it's a change in interactive behavior now is 
>> minor
>> compared to the benefit.
>
>I understand that this is your opinion, but I've heard enough
>complaints from veteran users about annoying behavior changes 
>forced
>upon them (and had my own share of such annoyances) that I'm 
>firmly
>against making such changes opt-out.

Great -- that's a clear decision backed up by a perfectly 
reasonably argument, thank you. (I don't even disagree very 
strongly; there are good arguments both ways on this one.)

So, I'll change the patch so it defaults to no confirmation, and 
add an etc/NEWS entry, and then we can see how people like it. 
Revised patch coming soon.

Best regards,
-Karl

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

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

Revised patch attached, with the option now defaulting to nil 
(i.e., to the old behavior) as per discussion.  Review/comments 
welcome.

Lars, you wrote this regarding v1 of this patch:

>...the first line [of the doc string] should be a complete 
>sentence.

It actually was a complete sentence even in v1, but I think I know 
what you meant.  However, the "Non-nil means..." phrasing is found 
throughout Emacs -- I counted over 1000 places with this quick 
check:

  $ find lisp -name "*.el" | xargs grep '"Non-nil means'

(One can add '-C3' to grep to see that they are indeed 
`defcustom's and `defvar's.)

I guess that people are used to it, so I left that phrasing as-is.

Best regards,
-Karl


[-- Attachment #2: bookmark-menu-deletion-confirmation-patch-v2.txt --]
[-- Type: text/plain, Size: 4943 bytes --]

[[[
New option to confirm deletion in bookmark menu

* lisp/bookmark.el (bookmark-menu-confirm-deletion): New defcustom.
(bookmark-delete-all): Add comment explaining why we don't use the new
confirmation formula here.
(bookmark-bmenu-execute-deletions): Conditionally confirm deletion.
Note that the bulk of the code diff here is just reindentation of an
otherwise unchanged `let' expression.

* etc/NEWS: Announce the new option.

Thanks to Lars Ingebrigtsen and Eli Zaretskii for review, and thanks
to Oliver Taylor for suggesting the option in the first place:

  https://lists.gnu.org/archive/html/emacs-humanities/2021-02/msg00022.html
  From: Oliver Taylor
  Subject: Re: [emacs-humanities] Extending Emacs Bookmarks to Work with EWW
  To: Karl Fogel
  Cc: Stefan Kangas, Emacs-humanities mailing list
  Date: Wed, 3 Feb 2021 20:21:59 -0800
  Message-Id: <936D47EA-4D11-452B-8303-971B6386877B@me.com>
]]]

--- etc/NEWS
+++ etc/NEWS
@@ -276,6 +276,14 @@ commands.  The new keystrokes are 'C-x x g' ('revert-buffer'),
 ** Commands 'set-frame-width' and 'set-frame-height' can now get their
 input using the minibuffer.
 
+---
+** New user option 'bookmark-menu-confirm-deletion'
+In Bookmark Menu mode, Emacs by default does not prompt for
+confirmation when you type 'x' to execute the deletion of bookmarks
+that have been marked for deletion.  However, if this new option is
+non-nil then Emacs will require confirmation with 'yes-or-no-p' before
+deleting.
+
 \f
 * Editing Changes in Emacs 28.1
 
--- lisp/bookmark.el
+++ lisp/bookmark.el
@@ -121,6 +121,13 @@ bookmark-sort-flag
   :type 'boolean)
 
 
+(defcustom bookmark-menu-confirm-deletion nil
+  "Non-nil means prompt for confirmation when executing the deletion
+of bookmarks marked for deletion in a bookmark menu buffer; nil
+means don't prompt for confirmation."
+  :version "28.1"
+  :type 'boolean)
+
 (defcustom bookmark-automatically-show-annotations t
   "Non-nil means show annotations when jumping to a bookmark."
   :type 'boolean)
@@ -1376,6 +1383,13 @@ bookmark-delete-all
 If optional argument NO-CONFIRM is non-nil, don't ask for
 confirmation."
   (interactive "P")
+  ;; We don't use `bookmark-menu-confirm-deletion' here because that
+  ;; variable is specifically to control confirmation prompting in a
+  ;; bookmark menu buffer, where the user has the marked-for-deletion
+  ;; bookmarks arrayed in front of them and might have accidentally
+  ;; hit the key that executes the deletions.  The UI situation here
+  ;; is quite different, by contrast: the user got to this point by a
+  ;; sequence of keystrokes unlikely to be typed by chance.
   (when (or no-confirm
             (yes-or-no-p "Permanently delete all bookmarks? "))
     (bookmark-maybe-load-default-file)
@@ -2142,30 +2156,35 @@ bookmark-bmenu-delete-all
 
 
 (defun bookmark-bmenu-execute-deletions ()
-  "Delete bookmarks flagged `D'."
+  "Delete bookmarks flagged `D'.
+If `bookmark-menu-confirm-deletion' is non-nil, prompt for
+confirmation first."
   (interactive nil bookmark-bmenu-mode)
-  (let ((reporter (make-progress-reporter "Deleting bookmarks..."))
-        (o-point  (point))
-        (o-str    (save-excursion
-                    (beginning-of-line)
-                    (unless (= (following-char) ?D)
-                      (buffer-substring
-                       (point)
-                       (progn (end-of-line) (point))))))
-        (o-col     (current-column)))
-    (goto-char (point-min))
-    (while (re-search-forward "^D" (point-max) t)
-      (bookmark-delete (bookmark-bmenu-bookmark) t)) ; pass BATCH arg
-    (bookmark-bmenu-list)
-    (if o-str
-        (progn
-          (goto-char (point-min))
-          (search-forward o-str)
-          (beginning-of-line)
-          (forward-char o-col))
-      (goto-char o-point))
-    (beginning-of-line)
-    (progress-reporter-done reporter)))
+  (if (and bookmark-menu-confirm-deletion
+           (not (yes-or-no-p "Delete selected bookmarks? ")))
+      (message "Bookmarks not deleted.")
+    (let ((reporter (make-progress-reporter "Deleting bookmarks..."))
+          (o-point  (point))
+          (o-str    (save-excursion
+                      (beginning-of-line)
+                      (unless (= (following-char) ?D)
+                        (buffer-substring
+                         (point)
+                         (progn (end-of-line) (point))))))
+          (o-col     (current-column)))
+      (goto-char (point-min))
+      (while (re-search-forward "^D" (point-max) t)
+        (bookmark-delete (bookmark-bmenu-bookmark) t)) ; pass BATCH arg
+      (bookmark-bmenu-list)
+      (if o-str
+          (progn
+            (goto-char (point-min))
+            (search-forward o-str)
+            (beginning-of-line)
+            (forward-char o-col))
+        (goto-char o-point))
+      (beginning-of-line)
+      (progress-reporter-done reporter))))
 
 
 (defun bookmark-bmenu-rename ()

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Lars Ingebrigtsen]

Karl Fogel <kfogel@red-bean.com> writes:

> Revised patch attached, with the option now defaulting to nil (i.e.,
> to the old behavior) as per discussion.  Review/comments welcome.
>
> Lars, you wrote this regarding v1 of this patch:
>
>> ...the first line [of the doc string] should be a complete sentence.
>
> It actually was a complete sentence even in v1, but I think I know
> what you meant.  However, the "Non-nil means..." phrasing is found
> throughout Emacs -- I counted over 1000 places with this quick check:

What I meant was that the first line should be a complete sentence.  :-)

> +(defcustom bookmark-menu-confirm-deletion nil
> +  "Non-nil means prompt for confirmation when executing the deletion
> +of bookmarks marked for deletion in a bookmark menu buffer; nil
> +means don't prompt for confirmation."

This doc string is three lines long, and the first line isn't a complete
sentence -- but it should be.

In any case, as I said -- I don't think adding this user option makes a
lot of sense.  Instead bookmark should implement "undo" functionality.

And adding the option, but defaulting to nil, makes even less sense --
nobody is going to discover this option unless it defaults to t.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

On 05 May 2021, Lars Ingebrigtsen wrote:
>Karl Fogel <kfogel@red-bean.com> writes:
>
>> Revised patch attached, with the option now defaulting to nil 
>> (i.e.,
>> to the old behavior) as per discussion.  Review/comments 
>> welcome.
>>
>> Lars, you wrote this regarding v1 of this patch:
>>
>>> ...the first line [of the doc string] should be a complete 
>>> sentence.
>>
>> It actually was a complete sentence even in v1, but I think I 
>> know
>> what you meant.  However, the "Non-nil means..." phrasing is 
>> found
>> throughout Emacs -- I counted over 1000 places with this quick 
>> check:
>
>What I meant was that the first line should be a complete 
>sentence.  :-)

Ah, right!  I forgot that sometimes "line" means literally "line", 
right.  Ahem.  Sorry :-).  You said exactly what you meant, and I 
somehow read something different.

>In any case, as I said -- I don't think adding this user option 
>makes a
>lot of sense.  Instead bookmark should implement "undo" 
>functionality.

Hmm, I can understand, but... let me give you my best argument for 
why to do this instead of implementing undo:

First of all, I think this option is useful on its own, with or 
without undo.  It did come from a user request, after all, which 
was seconded (on Emacs Humanities) by at least one other person 
besides me.  Giving people the ability to prevent the bad thing 
from happening is not the same as giving them ability to recover 
if the bad thing happens.

Also, implementing undo functionality would open up a whole lot of 
questions.  If Bookmark Menu mode supports undo, do Bookmark 
operations in general support undo?  If so, which ones?  

And even just within Bookmark Menu, what else should be undo-able? 
Renaming?  What about retargeting?  And there's the usual bevy of 
undo-boundary questions: suppose someone does 'x' to execute 
deletions, then 's' to save the current state of the bookmarks 
list.  If they then undo, should it undo the deletions but not 
save the result, or should it also save?

Personally, I'm not prepared to go down this road; it's a lot of 
work and complexity.  The situation that needed addressing in 
Bookmark Menu was simply that it's too easy to accidentally hit 
'x' and delete things that you weren't ready to delete.  Solving 
this one problem, in the same way that Dired solves it, seems 
clean and useful to me.

>And adding the option, but defaulting to nil, makes even less 
>sense --
>nobody is going to discover this option unless it defaults to t.

Well, there is the etc/NEWS entry, but still I kind of agree with 
you.  I would have chosen the default-to-true route, though Eli's 
preference for the other way is perfectly reasonable; this isn't a 
One Right Way situation IMHO.

Discoverability problems would exist with undo, too, of course.

Best regards,
-Karl

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Stefan Monnier]

> Hmm, I can understand, but... let me give you my best argument for why to do
> this instead of implementing undo:

Makes a lot of sense to me, yes.
While the idea of "undo" was brought up in response to your proposal and
can indeed be seen as providing a similar functionality, I think the two
are mostly orthogonal.

> First of all, I think this option is useful on its own, with or without
> undo.

Agreed.  And similarly undo would be useful with or without such
a confirmation prompt.

> Also, implementing undo functionality would open up a whole lot
> of questions.

Yes, it's an interesting field.  I hope someone will tackle this problem.

> If Bookmark Menu mode supports undo, do Bookmark operations in
> general support undo?

I'd say yes, but in order to save the user the trouble of saying "undo
bookmark operation" (as opposed to undoing other things), I think it's
OK to limit oneself to supporting the `undo` command within the
bookmark-menu buffer.

I think to make it work well, we'd also want to make sure the bookmark
menu is always up-to-date with the bookmark data structure (i.e. any
external change to the bookmarks would be immediately reflected in a
bookmark menu if there is one (with a corresponding undo entry)).

> If so, which ones?  And even just within Bookmark
> Menu, what else should be undo-able?

I think it would make sense to make "everything" in there undoable.
We might even be able to support `undo-in-region` ;-)

>  And there's the usual bevy of undo-boundary questions: suppose someone does
>  'x' to execute deletions, then 's' to save the current state of the
>  bookmarks list.  If they then undo, should it undo the deletions but not
>  save the result, or should it also save?

If you think of the bookmark-menu as the buffer that visits the bookmark
file, then it would be natural that "save" is not part of the operations
that are recorded in the undo log (and the `**` in the modeline might
try and reflect the "saved" state like we do for normal files).


        Stefan

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

On 05 May 2021, Stefan Monnier wrote:
>> Hmm, I can understand, but... let me give you my best argument 
>> for why to do
>> this instead of implementing undo:
>
>Makes a lot of sense to me, yes.
>While the idea of "undo" was brought up in response to your 
>proposal and
>can indeed be seen as providing a similar functionality, I think 
>the two
>are mostly orthogonal.

Glad to hear that.  +1 to the rest of your analysis below, too, 
including that last part about how for undo purposes the Bookmark 
Menu buffer can be thought of as "visiting" the file in which 
bookmarks are saved.  (However, users are much less directly 
exposed to the bookmark file than they are to the regular kind of 
file that one visits, edits, and saves -- so the analogy fits 
mostly but perhaps not perfectly.  A person could in theory use 
bookmarks regularly without ever needing to be aware that the file 
even exists.)

I don't plan to implement the undo functionality myself.  It's a 
larger task than I want to take on.  Or rather, if I were going to 
find time to do something of that size in Emacs, there are other 
things I would pick before this.  However, it's good to have all 
these thoughts archived here, in case anyone else decides to do 
it. 

Best regards,
-Karl

>> First of all, I think this option is useful on its own, with or 
>> without
>> undo.
>
>Agreed.  And similarly undo would be useful with or without such
>a confirmation prompt.
>
>> Also, implementing undo functionality would open up a whole lot
>> of questions.
>
>Yes, it's an interesting field.  I hope someone will tackle this 
>problem.
>
>> If Bookmark Menu mode supports undo, do Bookmark operations in
>> general support undo?
>
>I'd say yes, but in order to save the user the trouble of saying 
>"undo
>bookmark operation" (as opposed to undoing other things), I think 
>it's
>OK to limit oneself to supporting the `undo` command within the
>bookmark-menu buffer.
>
>I think to make it work well, we'd also want to make sure the 
>bookmark
>menu is always up-to-date with the bookmark data structure 
>(i.e. any
>external change to the bookmarks would be immediately reflected 
>in a
>bookmark menu if there is one (with a corresponding undo entry)).
>
>> If so, which ones?  And even just within Bookmark
>> Menu, what else should be undo-able?
>
>I think it would make sense to make "everything" in there 
>undoable.
>We might even be able to support `undo-in-region` ;-)
>
>>  And there's the usual bevy of undo-boundary questions: suppose 
>>  someone does
>>  'x' to execute deletions, then 's' to save the current state 
>>  of the
>>  bookmarks list.  If they then undo, should it undo the 
>>  deletions but not
>>  save the result, or should it also save?
>
>If you think of the bookmark-menu as the buffer that visits the 
>bookmark
>file, then it would be natural that "save" is not part of the 
>operations
>that are recorded in the undo log (and the `**` in the modeline 
>might
>try and reflect the "saved" state like we do for normal files).
>
>
>        Stefan

RE: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Drew Adams]

> >> Hmm, I can understand, but... let me give you my best argument
> >> for why to do this instead of implementing undo:
> >
> > Makes a lot of sense to me, yes.
> > While the idea of "undo" was brought up in response to your
> > proposal and can indeed be seen as providing a similar
> > functionality, I think the two are mostly orthogonal.
> 
> Glad to hear that.  +1 to the rest of your analysis below, too,
> including that last part about how for undo purposes the Bookmark
> Menu buffer can be thought of as "visiting" the file in which
> bookmarks are saved.  (However, users are much less directly
> exposed to the bookmark file than they are to the regular kind of
> file that one visits, edits, and saves -- so the analogy fits
> mostly but perhaps not perfectly.  A person could in theory use
> bookmarks regularly without ever needing to be aware that the file
> even exists.)
> 
> I don't plan to implement the undo functionality myself.  It's a
> larger task than I want to take on.  Or rather, if I were going to
> find time to do something of that size in Emacs, there are other
> things I would pick before this.  However, it's good to have all
> these thoughts archived here, in case anyone else decides to do
> it.

FWIW, I'm not warm to the idea of undoing operations,
here.

Consider undo in WDired.  You can just as easily want
to think of WDired as "visiting" a directory, but
that's problematic.  It's visiting a presentation of
a directory listing.  (Actually, the listing can be of
several directories, or even of arbitrary files located
anywhere.)

We support undoing text changes to the buffer in Wired.
But WDired doesn't perform operations _on the files_
listed.  (Dired does.)  It's a view of some files -
in that sense it "visits" them or their directory.
But the analogy of a buffer visiting a file is poor.

The bookmark-list display buffer (bookmark "menu") is
more analogous to Dired than WDired.  How much undo
does Dired support?  Only undo to what are, in effect
annotations (and not done via `undo' but by specific
operations such as unmarking etc.).

We don't support undo for its operations on files and
dirs that are listed.  If you use `x' to delete all
flagged files, there's no undo for that.  (We have a
move-to-trash possibility, but nothing with `undo'.)

And for good reason, I think.

This can certainly be discussed/explored more, but a
priori I think that direction is generally misguided,
especially from a user point of view.

Take the buffer-list display as another example.
Do we offer undo as a way to undo operations on
buffers there?  Should we?  I think not.  Not in
general.  Undo The Hammer is not the best way to
undo some things.

The bookmark-list display is, like Dired, a view
of a listing of things.  And no, those things are
not the bookmarks in "the bookmark file".  They're
the bookmarks in the _current bookmark list_.

The bookmarks in that list can be loaded from any
number of bookmark files, and more bookmarks can
be added on the fly, so that the list corresponds
to no bookmark file.

The only thing you can say about it is that if you
_save_ the current bookmark list then, by default,
its bookmarks will replace those in a particular
bookmark file.

[On the other hand, at least with Bookmark+, you
can save the bookmark-list display itself - its
current state - in a file.  And you can restore
it from that file (or another).  So perhaps there's
room for being able to undo some textual changes
in the bookmark-list display - like using undo in
WDired.  But I don't see that as a rich mine of
interest, at least not a priori.]

___

As for offering undo of `x' deletions as a
_substitute_ for prompting for confirmation:
that's simply wrong, IMO.  There are several
reasons that undo either won't help or can cause
harm in this context.  They've already been
pointed out.

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

On 05 May 2021, Drew Adams wrote:
>The bookmarks in that list can be loaded from any
>number of bookmark files, and more bookmarks can
>be added on the fly, so that the list corresponds
>to no bookmark file.

That's a good point.  (I hadn't thought of that, probably because 
I never use bookmarks that way except occasionally when testing.)

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Matthias Meulien]

Drew Adams <drew.adams@oracle.com> writes:

> (...) The bookmarks in that list can be loaded from any number of
> bookmark files, and more bookmarks can be added on the fly, so that
> the list corresponds to no bookmark file.

Yes, you're right.

But if one press `s', the default bookmark file will be updated with
all bookmarks whatever their originating bookmark file!

From my pov and usage (one bookmark file for work projects and another
one, the default one, for personnal projects and common bookmarks like
download folder, Python documentation, etc.) this makes loading multiple
bookmark files deeply broken (but that's another discussion).
-- 
Matthias

RE: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Drew Adams]

> > (...) The bookmarks in that list can be loaded from any number of
> > bookmark files, and more bookmarks can be added on the fly, so that
> > the list corresponds to no bookmark file.
> 
> Yes, you're right.  But if one press `s', the default
> bookmark file will be updated with all bookmarks
> whatever their originating bookmark file!

Which is what I said:

  The only thing you can say about it is that if you
  _save_ the current bookmark list then, by default,
  its bookmarks will replace those in a particular
  bookmark file.

> From my pov and usage (one bookmark file for work
> projects and another one, the default one, for
> personnal projects and common bookmarks like
> download folder, Python documentation, etc.) this
> makes loading multiple bookmark files deeply broken
> (but that's another discussion).

I disagree.  But then, I use Bookmark+, which makes
using multiple bookmark files much easier and more
useful.

https://www.emacswiki.org/emacs/BookmarkPlus#UsingMultipleBookmarkFiles

Wrt your use case of using only two files: Just use
a prefix arg with `L', to switch to the last bookmark
file used.  (Or use `L RET', since the last one is
the default for switching.)

[IIRC, vanilla bookmark.el doesn't have a way to
switch to a different bookmark file interactively.
All it needs to do, to add that, is to let a prefix
arg for `bookmark-load' and `bookmark-bmenu-load'
provide non-nil arg OVERWRITE.  It provides a
prefix arg for saving, but not for loading.]

A particularly handy way to switch among multiple
bookmark files is to bookmark bookmark files!  Then
you just jump to such a bookmark to switch or load
(add) files.  Use bookmark names that tell you just
what each file is for.  Use such bookmarks to flip
between projects or contexts, or to combine/compose
them.

In menu-bar menu `Bookmark+' for your bookmark-list
display (`C-x r l'), submenu `Bookmark File' has
these items:

 _____________________________________________________
 Revert to Saved Bookmarks                  C-u g
 Empty Bookmark File...                     C-x x 0
 ___
 Load (Add) Bookmarks...                    l
 Switch to Bookmarks...                     L
 Switch to Last Bookmark File...            C-u L
 ___
 Load Bookmarks, Mark Those Loaded...
 Load Bookmarks, Mark Only Those Loaded...
 Load Marked Bookmark-File Bookmarks...     M-l
 ___
 Move Marked to Bookmark File...            Y > -
 Copy Marked to Bookmark File...            Y > +
 Copy Marked to New Bookmark File...        Y > 0
 Set Bookmark-File Bookmark from Marked...  C-u Y > 0
_____________________________________________________

You can toggle automatic saving of your current
bookmarks to the current bookmark file any time,
using `M-~'.  (Toggles option `bookmark-save-flag'.)
Turning auto-saving off is often what you want
to do if you've loaded multiple bookmark files.

You can also easily copy or move bookmarks from
one bookmark file to another.

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Matthias Meulien]

Drew Adams <drew.adams@oracle.com> writes:

> [IIRC, vanilla bookmark.el doesn't have a way to
> switch to a different bookmark file interactively.
> All it needs to do, to add that, is to let a prefix
> arg for `bookmark-load' and `bookmark-bmenu-load'
> provide non-nil arg OVERWRITE.  It provides a
> prefix arg for saving, but not for loading.]

I checked the implementation of `bookmark-bmenu-load' and
`bookmark-load' and learned that this is already implemented: One can
C-u l from a bookmark menu buffer to load bookmarks destructively and
change the path of the default bookmark file!

Should the docstring of `bookmark-bmenu-load' should be extended as
follows to make this feature easier to discover?

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 64b467adfa..766ddc8402 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -2063,7 +2063,10 @@ bookmark-bmenu-save
 
 
 (defun bookmark-bmenu-load ()
-  "Load the bookmark file and rebuild the bookmark menu-buffer."
+  "Load the bookmark file and rebuild the bookmark menu-buffer.
+With a prefix arg, existing bookmarks are destroyed and the
+loaded bookmark file is watched.
+"
   (interactive nil bookmark-bmenu-mode)
   (bookmark-bmenu-ensure-position)
   (save-excursion

-- 
Matthias

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Eli Zaretskii]

> From: Matthias Meulien <orontee@gmail.com>
> Date: Sat, 08 May 2021 22:46:00 +0200
> Cc: Karl Fogel <kfogel@red-bean.com>, Lars Ingebrigtsen <larsi@gnus.org>,
>  Stefan Monnier <monnier@iro.umontreal.ca>,
>  "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> 
> I checked the implementation of `bookmark-bmenu-load' and
> `bookmark-load' and learned that this is already implemented: One can
> C-u l from a bookmark menu buffer to load bookmarks destructively and
> change the path of the default bookmark file!
> 
> Should the docstring of `bookmark-bmenu-load' should be extended as
> follows to make this feature easier to discover?

Yes.  However, this works because bookmark-bmenu-load calls
bookmark-load via call-interactively, and the latter is sensitive to
the prefix argument.  So the doc string of bookmark-load should also
be improved by mentioning the effect of the prefix argument.

Thanks.

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

On 09 May 2021, Eli Zaretskii wrote:
>> Should the docstring of `bookmark-bmenu-load' should be 
>> extended as
>> follows to make this feature easier to discover?
>
>Yes.  However, this works because bookmark-bmenu-load calls
>bookmark-load via call-interactively, and the latter is sensitive 
>to
>the prefix argument.  So the doc string of bookmark-load should 
>also
>be improved by mentioning the effect of the prefix argument.

My inclination would be to do this by having `bookmark-bmenu-load' 
just refer readers to `bookmark-load', and then improving the 
latter's doc string.  That way if future changes are made to 
`bookmark-load's behavior, we wouldn't need to remember to update 
`bookmark-bmenu-load's doc string too.

Thoughts?

Best regards,
-Karl

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: Matthias Meulien <orontee@gmail.com>,  drew.adams@oracle.com,
>   larsi@gnus.org,  monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Sun, 09 May 2021 13:37:52 -0500
> 
> My inclination would be to do this by having `bookmark-bmenu-load' 
> just refer readers to `bookmark-load', and then improving the 
> latter's doc string.  That way if future changes are made to 
> `bookmark-load's behavior, we wouldn't need to remember to update 
> `bookmark-bmenu-load's doc string too.
> 
> Thoughts?

I'll have thoughts when I see the result ;-)

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

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

On 09 May 2021, Eli Zaretskii wrote:
>> From: Karl Fogel <kfogel@red-bean.com>
>> Cc: Matthias Meulien <orontee@gmail.com>, 
>> drew.adams@oracle.com,
>>   larsi@gnus.org,  monnier@iro.umontreal.ca, 
>>   emacs-devel@gnu.org
>> Date: Sun, 09 May 2021 13:37:52 -0500
>> 
>> My inclination would be to do this by having 
>> `bookmark-bmenu-load' 
>> just refer readers to `bookmark-load', and then improving the 
>> latter's doc string.  That way if future changes are made to 
>> `bookmark-load's behavior, we wouldn't need to remember to 
>> update 
>> `bookmark-bmenu-load's doc string too.
>> 
>> Thoughts?
>
>I'll have thoughts when I see the result ;-)

Heh, fair enough.  My inquiry was about the general practice of 
having one function's doc string referring out to another 
function's.  But it sounds like you have no objection to that, and 
neither do I.

If the attached doc patch looks sensible, I'll commit it.

Best regards,
-Karl


[-- Attachment #2: 0001-Improve-bookmark-bmenu-load-doc-string.patch --]
[-- Type: text/plain, Size: 1372 bytes --]

From e0dac0712bf2ee9cef1ab067dbc26c6a00a1f85b Mon Sep 17 00:00:00 2001
From: Karl Fogel <kfogel@red-bean.com>
Date: Tue, 25 May 2021 00:26:57 -0500
Subject: [PATCH] Improve `bookmark-bmenu-load' doc string

* lisp/bookmark.el (bookmark-bmenu-load): Refer reader to
  `bookmark-load' for more information.

As discussed in this thread:

  https://lists.gnu.org/archive/html/emacs-devel/2021-05/msg00389.html

  From: Karl Fogel
  To: Eli Zaretskii
  Cc: Matthias Meulien, Drew Adams, Lars Ingebrigtsen,
      Stefan Monnier, Emacs Devel
  Subject: Re: [External] : Re: [PATCH] When deleting in bookmark menu,
           prompt for confirmation.
  Date: Sun, 09 May 2021 13:37:52 -0500
  Message-ID: <87h7jboirj.fsf@red-bean.com>
---
 lisp/bookmark.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git lisp/bookmark.el lisp/bookmark.el
index 64b467adfa..e6a22f9bf0 100644
--- lisp/bookmark.el
+++ lisp/bookmark.el
@@ -2063,7 +2063,9 @@ bookmark-bmenu-save
 
 
 (defun bookmark-bmenu-load ()
-  "Load the bookmark file and rebuild the bookmark menu-buffer."
+  "Load the bookmark file and rebuild the bookmark menu-buffer.
+This invokes `bookmark-load' interactively, so see there for more
+information (such as how a prefix argument is handled)."
   (interactive nil bookmark-bmenu-mode)
   (bookmark-bmenu-ensure-position)
   (save-excursion
-- 
2.30.2

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: orontee@gmail.com,  drew.adams@oracle.com,  larsi@gnus.org,
>   monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Tue, 25 May 2021 00:38:22 -0500
> 
> >> Thoughts?
> >
> >I'll have thoughts when I see the result ;-)
> 
> Heh, fair enough.  My inquiry was about the general practice of 
> having one function's doc string referring out to another 
> function's.

It depends on what you need to say, thus my response above.

Given what you wrote, and what bookmark-load does with the prefix
argument, I think it is better to say that explicitly in
bookmark-bmenu-load's doc string, since you only need a single quite
simple sentence to say that, whereas the doc strong of bookmark-load
is quite long.

Thanks.

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

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

On 25 May 2021, Eli Zaretskii wrote:
>> From: Karl Fogel <kfogel@red-bean.com>
>> Cc: orontee@gmail.com,  drew.adams@oracle.com,  larsi@gnus.org,
>>   monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>> Date: Tue, 25 May 2021 00:38:22 -0500
>> 
>> >> Thoughts?
>> >
>> >I'll have thoughts when I see the result ;-)
>> 
>> Heh, fair enough.  My inquiry was about the general practice of 
>> having one function's doc string referring out to another 
>> function's.
>
>It depends on what you need to say, thus my response above.
>
>Given what you wrote, and what bookmark-load does with the prefix
>argument, I think it is better to say that explicitly in
>bookmark-bmenu-load's doc string, since you only need a single 
>quite
>simple sentence to say that, whereas the doc strong of 
>bookmark-load
>is quite long.

Well, now that I've done it, I think your way is an improvement -- 
although it turned out to be slightly more doc change than I 
expected.  Revised patch attached for review.

Best regards,
-Karl


[-- Attachment #2: 0001-Improve-some-doc-strings-in-bookmark.el.patch --]
[-- Type: text/plain, Size: 2410 bytes --]

From 5817bd661775f4f9ecb1ab0efc831a36fef44578 Mon Sep 17 00:00:00 2001
From: Karl Fogel <kfogel@red-bean.com>
Date: Tue, 25 May 2021 00:26:57 -0500
Subject: [PATCH] Improve some doc strings in bookmark.el

* lisp/bookmark.el (bookmark-bmenu-load): Describe prefix argument
  behavior.  Refer to related functions for more information.
  (bookmark-bmenu-save): Likewise refer to related functions.

As discussed in this thread:

  https://lists.gnu.org/archive/html/emacs-devel/2021-05/msg00389.html

  From: Karl Fogel
  To: Eli Zaretskii
  Cc: Matthias Meulien, Drew Adams, Lars Ingebrigtsen,
      Stefan Monnier, Emacs Devel
  Subject: Re: [External] : Re: [PATCH] When deleting in bookmark menu,
           prompt for confirmation.
  Date: Sun, 09 May 2021 13:37:52 -0500
  Message-ID: <87h7jboirj.fsf@red-bean.com>
---
 lisp/bookmark.el | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git lisp/bookmark.el lisp/bookmark.el
index 64b467adfa..31e41a9273 100644
--- lisp/bookmark.el
+++ lisp/bookmark.el
@@ -2054,7 +2054,10 @@ bookmark-bmenu-any-marks
 
 (defun bookmark-bmenu-save ()
   "Save the current list into a bookmark file.
-With a prefix arg, prompts for a file to save them in."
+With a prefix arg, prompts for a file to save them in.
+
+See also the related behaviors of `bookmark-load' and
+`bookmark-bmenu-load'."
   (interactive nil bookmark-bmenu-mode)
   (save-excursion
     (save-window-excursion
@@ -2063,7 +2066,19 @@ bookmark-bmenu-save
 
 
 (defun bookmark-bmenu-load ()
-  "Load the bookmark file and rebuild the bookmark menu-buffer."
+  "Load bookmarks from a file and rebuild the bookmark menu-buffer.
+Prompt for a file, with the default choice being the value of
+`bookmark-default-file'.
+
+With a prefix argument, replace the current ambient bookmarks
+(i.e., the ones in `bookmark-alist') with the ones from the selected
+file and make that file be the new value of `bookmark-default-file'.
+In other words, a prefix argument means \"switch over to the bookmark
+universe defined in the loaded file\".  Without a prefix argument,
+just add the loaded bookmarks into the current ambient set.
+
+See the documentation for `bookmark-load' for more details; see also
+the related behaviors of `bookmark-save' and `bookmark-bmenu-save'."
   (interactive nil bookmark-bmenu-mode)
   (bookmark-bmenu-ensure-position)
   (save-excursion
-- 
2.30.2

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: orontee@gmail.com,  drew.adams@oracle.com,  larsi@gnus.org,
>   monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Tue, 25 May 2021 15:24:44 -0500
> 
> >Given what you wrote, and what bookmark-load does with the prefix
> >argument, I think it is better to say that explicitly in
> >bookmark-bmenu-load's doc string, since you only need a single 
> >quite
> >simple sentence to say that, whereas the doc strong of 
> >bookmark-load
> >is quite long.
> 
> Well, now that I've done it, I think your way is an improvement -- 
> although it turned out to be slightly more doc change than I 
> expected.  Revised patch attached for review.

LGTM, thanks.

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

On 26 May 2021, Eli Zaretskii wrote:
>> From: Karl Fogel <kfogel@red-bean.com>
>> Cc: orontee@gmail.com,  drew.adams@oracle.com,  larsi@gnus.org,
>>   monnier@iro.umontreal.ca,  emacs-devel@gnu.org
>> Date: Tue, 25 May 2021 15:24:44 -0500
>> 
>> >Given what you wrote, and what bookmark-load does with the 
>> >prefix
>> >argument, I think it is better to say that explicitly in
>> >bookmark-bmenu-load's doc string, since you only need a single 
>> >quite
>> >simple sentence to say that, whereas the doc strong of 
>> >bookmark-load
>> >is quite long.
>> 
>> Well, now that I've done it, I think your way is an improvement 
>> -- 
>> although it turned out to be slightly more doc change than I 
>> expected.  Revised patch attached for review.
>
>LGTM, thanks.

Thanks for the review, Eli.  Committed (c4e8d1dbe2e) on master.

The reason it's 'master' instead of 'emacs-27' is that the patch 
doesn't apply cleanly on 'emacs-27', and the effort needed to 
adjust it didn't seem worth it to me.  (If these documentation 
updates were urgently needed by users, I'd make a different 
decision, but I don't think that's the case.)

Best regards,
-Karl

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

On 06 May 2021, Matthias Meulien wrote:
>>From my pov and usage (one bookmark file for work projects and 
>>another
>one, the default one, for personnal projects and common bookmarks 
>like
>download folder, Python documentation, etc.) this makes loading 
>multiple
>bookmark files deeply broken (but that's another discussion).

Agreed.  However, since we don't get many bug reports about it in 
practice, I'm not sure it's worth solving.  If anything, it might 
be an argument for removing the multiple-bookmark-files 
functionality and just admitting that everyone's just using the 
one default file :-).

RE: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Drew Adams]

> However, since we don't get many bug reports about it in
> practice, I'm not sure it's worth solving.  If anything, it might
> be an argument for removing the multiple-bookmark-files
> functionality and just admitting that everyone's just using the
> one default file :-).

That would really be too bad.

Dunno whether that would mean that Bookmark+ would
need to incorporate the pre-change vanilla code.
Maybe the Bookmark+ code already side-steps the
vanilla code sufficiently in this regard to not
be bitten by such an (unfortunate) change.

There really is no good reason not to allow
multiple bookmarks with the same name...

Re: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

On 07 May 2021, Drew Adams wrote:
>> However, since we don't get many bug reports about it in
>> practice, I'm not sure it's worth solving.  If anything, it 
>> might
>> be an argument for removing the multiple-bookmark-files
>> functionality and just admitting that everyone's just using the
>> one default file :-).
>
>That would really be too bad.

Don't worry, I'm not sure enough about these hypothetical usage 
patterns to actually make the change.  Whatever people are doing, 
it's not resulting in bug reports here, so I wouldn't go looking 
for trouble!

Best regards,
-Karl

>Dunno whether that would mean that Bookmark+ would
>need to incorporate the pre-change vanilla code.
>Maybe the Bookmark+ code already side-steps the
>vanilla code sufficiently in this regard to not
>be bitten by such an (unfortunate) change.
>
>There really is no good reason not to allow
>multiple bookmarks with the same name...

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Lars Ingebrigtsen]

Karl Fogel <kfogel@red-bean.com> writes:

> Giving people the ability to prevent the bad thing from happening
> is not the same as giving them ability to recover if the bad thing
> happens.

It's not the same -- giving them the ability to recover is better.  :-)
We're teaching users to hit "y" after a whole bunch of commands, and the
"y" becomes automatic.

> Also, implementing undo functionality would open up a whole lot of
> questions.

It does, but a thing like this doesn't have to be 100% to be useful.
Every new thing that's made undoable is progress.

> Well, there is the etc/NEWS entry, but still I kind of agree with you.
> I would have chosen the default-to-true route, though Eli's preference
> for the other way is perfectly reasonable; this isn't a One Right Way
> situation IMHO.

There isn't, but if it's worth having a query, then that query is worth
the most to new users -- more experienced users can then choose to
switch it off or on.  Adding this option, but defaulting it to nil
doesn't add much value, in my opinion.  Anybody can add a prompter to
any command by just doing:

(advice-add 'bookmark-bmenu-execute-deletions :before
	    (lambda () (unless (y-or-n-p "Really execute?") (error "Nope"))))

Perhaps we should just make a convenience function for people to add
prompters to any command they want to...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Stefan Monnier]

> Perhaps we should just make a convenience function for people to add
> prompters to any command they want to...

We incidentally have pretty much exactly that in the form of "disabled
commands".


        Stefan

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Perhaps we should just make a convenience function for people to add
>> prompters to any command they want to...
>
> We incidentally have pretty much exactly that in the form of "disabled
> commands".

That's true...  it's just that the prompting for "disabled commands" is
a bit excessive if what you really want is just to have a y/n prompt for
a command you consider dangerous. 

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Stefan Monnier]

Lars Ingebrigtsen [2021-05-07 10:40:07] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> Perhaps we should just make a convenience function for people to add
>>> prompters to any command they want to...
>> We incidentally have pretty much exactly that in the form of "disabled
>> commands".
> That's true...  it's just that the prompting for "disabled commands" is
> a bit excessive if what you really want is just to have a y/n prompt for
> a command you consider dangerous. 

Indeed, but maybe that can be adjusted ;-)


        Stefan

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> That's true...  it's just that the prompting for "disabled commands" is
>> a bit excessive if what you really want is just to have a y/n prompt for
>> a command you consider dangerous. 
>
> Indeed, but maybe that can be adjusted ;-)

Sure, we could extend the `disabled' machinery easily enough.  For
instance, we could allow arbitrary functions, and then recommend that
people say

(put 'some-command 'disabled 'y-or-n-p)

in their Emacs files to confirm before calling?  Should be pretty
trivial to implement.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Sure, we could extend the `disabled' machinery easily enough.  For
> instance, we could allow arbitrary functions, and then recommend that
> people say
>
> (put 'some-command 'disabled 'y-or-n-p)
>
> in their Emacs files to confirm before calling?  Should be pretty
> trivial to implement.

I'm wondering what the use-case is here.  If there are any commands
where something like this would be useful, shouldn't we just treat that
as a bug and add a prompt?

Or do you have any commands in mind where users might want to add a
prompt, but we would not?

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Stefan Monnier]

Stefan Kangas [2021-05-09 10:04:57] wrote:
> Lars Ingebrigtsen <larsi@gnus.org> writes:
>> Sure, we could extend the `disabled' machinery easily enough.  For
>> instance, we could allow arbitrary functions, and then recommend that
>> people say
>>
>> (put 'some-command 'disabled 'y-or-n-p)
>>
>> in their Emacs files to confirm before calling?  Should be pretty
>> trivial to implement.
>
> I'm wondering what the use-case is here.  If there are any commands
> where something like this would be useful, shouldn't we just treat that
> as a bug and add a prompt?
>
> Or do you have any commands in mind where users might want to add a
> prompt, but we would not?

I think what this is getting at is to have a standardized way to have
and control such prompts.  Whether it's done via symbol-properties,
inline code, advice-add, or younameit is "an implementation detail".


        Stefan

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Lars Ingebrigtsen]

Stefan Kangas <stefankangas@gmail.com> writes:

> Or do you have any commands in mind where users might want to add a
> prompt, but we would not?

This thread is an example -- we've added code for confirmation of this
specific command, but it's not enabled by default.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

On 06 May 2021, Lars Ingebrigtsen wrote:
>It's not the same -- giving them the ability to recover is 
>better.  :-)
>We're teaching users to hit "y" after a whole bunch of commands, 
>and the
>"y" becomes automatic.

Ah, that's not a problem here.

When a confirmation prompt is used to defend against 
*accidentally* performing an action that the user didn't intend to 
perform, then there is no habituation problem.

In other words, sure, the "yes" *should* be automatic: if what you 
intend to do is execute the deletion of marked bookmarks, then you 
will (habitually and unthinkingly) type 'x yes RET'.  No problem 
there.

But when you *didn't* intend to do that, and you had simply hit 
'x' by accident, then you're very unlikely to type 'yes RET' as 
the very next thing you do -- your brain isn't running that macro 
right now.  So you'll be surprised when you find yourself at a 
confirmation prompt, and you'll look at what's happening and type 
C-g or 'no' to back out of the situation.  The confirmation has 
served its purpose.

The "confirmation is bad because habituation" argument is classic, 
but it only applies to situations where the confirmation was 
intended to make someone think more carefully about an intended 
action -- that is indeed foolish, but it's not what we're doing 
here.  Confirmation whose purpose is merely to make sure that 
action was intended at all is not broken by habituation, but 
rather made stronger by habituation.

Best regards,
-Karl

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Date: Wed, 05 May 2021 00:24:07 -0500
> 
> Revised patch attached, with the option now defaulting to nil 
> (i.e., to the old behavior) as per discussion.  Review/comments 
> welcome.

Thanks, LGTM, modulo Lars's comments (with which I agree) about the
doc string.

Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Karl Fogel]

On 05 May 2021, Eli Zaretskii wrote:
>> From: Karl Fogel <kfogel@red-bean.com>
>> Date: Wed, 05 May 2021 00:24:07 -0500
>> 
>> Revised patch attached, with the option now defaulting to nil 
>> (i.e., to the old behavior) as per discussion.  Review/comments 
>> welcome.
>
>Thanks, LGTM, modulo Lars's comments (with which I agree) about 
>the
>doc string.

Committed (b8bdf6437712), with doc string fixed as per Lars.

Best regards,
-Karl

RE: [External] : Re: [PATCH] When deleting in bookmark menu, prompt for confirmation.

[Drew Adams]

FWIW, I agree with Karl (and others).  The strongest
arguments are: (1) Dired asks for confirmation, and
(2) it's easy to accidentally hit `x' with flags
(`D') present but not visible (e.g. scrolled off
window).

I haven't done this (yet) in Bookmark+, but it's a
good idea to do it.  If you add this option then I'll
update my code to respect it.
___

I'm not even sure that we should bother having an
option for _not_ prompting to confirm.  (And that's
rare for me. ;-))