bug#12504: 24.2.50; `bookmark-rename' and `bookmark-maybe-historicize-string'

bug#12504: 24.2.50; `bookmark-rename' and `bookmark-maybe-historicize-string'

[Drew Adams]

Should `bookmark-rename' call `bookmark-maybe-historicize-string', as it
does now?  I am not sure this is a bug, but I would like to raise the
question.
 
What is the reason for that macro call by `bookmark-rename'?  The macro
pushes the STRING arg to `bookmark-history' for non-interactive use.
 
Why would we do that for the old bookmark name, when you rename a
bookmark?  Is it because we want to let you access that old name later,
so you can rename other bookmarks that might have the old name as a
prefix?
 
That's about all I can think of.  But with such a rationale, why don't
we do that only when `bookmark-rename' is called interactively?
Instead, we do it only when the function is NOT called interactively.
 
It seems to me that Lisp code should be able to use `bookmark-rename'
without adding the old name to the history.  Renaming a bookmark should
do only that, I think.
 
Is there a bug here?  If not, what is the rationale?
 
The doc string for `bookmark-rename' offers this rationale:
 
 Put STRING into the bookmark prompt history, if caller non-interactive.
 We need this because sometimes bookmark functions are invoked from
 menus, so `completing-read' never gets a chance to set `bookmark-history'.
 
(Such a rationale really should be a comment, not part of the doc
string, BTW.)
 
OK, it is true that `bookmark-rename' is used in a menu.  But what's
done does not seem the best way to handle the problem cited.  If it were,
then presumably we would be doing that kind of thing all over the place,
not just in bookmark.el.
 
And the implementation is overkill for that rationale.  It presumes that
every non-interactive call to `bookmark-rename' should update the
history.
 
I think there is a bug here, but if not I'd like to understand this
better.  Thx.
 

In GNU Emacs 24.2.50.1 (i386-mingw-nt5.1.2600)
 of 2012-09-17 on MARVIN
Bzr revision: 110062 cyd@gnu.org-20120917054104-r93rtwkrtva73ewe
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured using:
 `configure --with-gcc (4.7) --no-opt --enable-checking --cflags
 -ID:/devel/emacs/libs/libXpm-3.5.8/include
 -ID:/devel/emacs/libs/libXpm-3.5.8/src
 -ID:/devel/emacs/libs/libpng-dev_1.4.3-1/include
 -ID:/devel/emacs/libs/zlib-dev_1.2.5-2/include
 -ID:/devel/emacs/libs/giflib-4.1.4-1/include
 -ID:/devel/emacs/libs/jpeg-6b-4/include
 -ID:/devel/emacs/libs/tiff-3.8.2-1/include
 -ID:/devel/emacs/libs/gnutls-3.0.9/include
 -ID:/devel/emacs/libs/libiconv-1.13.1-1-dev/include
 -ID:/devel/emacs/libs/libxml2-2.7.8/include/libxml2'
 

bug#12504: `bookmark-rename' and `bookmark-maybe-historicize-string'

[Karl Fogel]

I agree there is a bug, or maybe a buglet, here, for the reasons you
describe, but I'm not sure how to solve it.

Does invoking functions through a menu result in an environment where
`called-interactively-p' returns non-nil?  In that case, the premise
behind `bookmark-maybe-historicize-string' is all wrong anyway, and the
macro should be rewritten to:

  `(when (called-interactively-p 'interactive)
     (setq bookmark-history (cons ,string bookmark-history))))

The issue is larger than just `bookmark-rename', obviously.

By the way, your guess is right: it's useful (I think) to have the old
name in the history for `bookmark-rename', because someone may want to
use it or a variant of it in another bookmark soon.  History is cheap
that way: it's better to have a little junk than to *not* have the thing
the user needs when they need it.

Let's tackle the larger issue with `bookmark-maybe-historicize-string',
and then figure out whether `bookmark-rename' is doing the right thing.

-Karl

bug#12504: `bookmark-rename' and `bookmark-maybe-historicize-string'

[Drew Adams]

> I agree there is a bug, or maybe a buglet, here, for the reasons you
> describe, but I'm not sure how to solve it.
> 
> Does invoking functions through a menu result in an environment where
> `called-interactively-p' returns non-nil?  In that case, the premise
> behind `bookmark-maybe-historicize-string' is all wrong 
> anyway, and the macro should be rewritten to:
> 
>   `(when (called-interactively-p 'interactive)
>      (setq bookmark-history (cons ,string bookmark-history))))

I think the problem that the code comment refers to, namely that invoking it
from a menu will not add it to the history, is a real problem, albeit a minor
one.  And as you say, it has nothing to do with bookmarks in particular.

I would suggest removing this workaround to try to make it DTRT for bookmark.el
menu commands, and just kick the question up to emacs-devel.  There might be a
good general answer.  In any case, it is a general question.  And I don't see a
crying need for bookmark renaming etc. to handle this specially.

There was some discussion on emacs-devel a few years back about optionally
(i.e., a user could choose) adding commands invoked using menus to the command
history (which is a bit different, but which could cover this case as well).

I implemented that in Icicles, with user option
`icicle-menu-items-to-history-flag':

 "Non-nil means to add menu-item commands to the command history.
  This history is `extended-command-history'."

FWIW, I do that by adding/removing this function to/from `pre-command-hook',
according to the option value:

(defun icicle-add-menu-item-to-cmd-history ()
  "Add `this-command' to command history, if it is a menu item.
Menu items that are not associated with a command symbol are ignored.
Used on `pre-command-hook'."
  (condition-case nil ; Just in case, since this is on `pre-command-hook'.
      (when (and (> (length (this-command-keys-vector)) 0)
                 (equal '(menu-bar) (elt (this-command-keys-vector) 0))
                 ;; Exclude uninterned symbols such as `menu-function-356'.
                 (symbolp this-command)
                 (intern-soft this-command))
        (pushnew (symbol-name this-command) extended-command-history))
    (error nil)))

> The issue is larger than just `bookmark-rename', obviously.

Yup.  I don't think bookmark.el should try to deal with it.  How important is it
for menu access to add such stuff to the history?

> By the way, your guess is right: it's useful (I think) to have the old
> name in the history for `bookmark-rename', because someone may want to
> use it or a variant of it in another bookmark soon.  History is cheap
> that way: it's better to have a little junk than to *not* 
> have the thing the user needs when they need it.

I agree about the usefulness, but I think it should be done only when
`bookmark-rename' is invoked interactively.

(In my code, there is, in some contexts, some automatic bookmark renaming, and
it makes no sense in such non-interactive calls to add the old names to the
history.  Anyway, that's my problem, but for bookmark.el I still don't think it
helps to add to the history unless invoked interactively.)

> Let's tackle the larger issue with 
> `bookmark-maybe-historicize-string',

See above.  Any solution to satisfy this need should, in any case, not be adding
to the history "if caller non-interactive".  That's a bad workaround if the goal
is to add to the history when the user uses the menu: non-interactively is not
the same thing as interactively-using-a-menu.

> and then figure out whether `bookmark-rename' is doing the 
> right thing.

See above for my opinion.

HTH - Drew

bug#12504: `bookmark-rename' and `bookmark-maybe-historicize-string'

[Karl Fogel]

"Drew Adams" <drew.adams@oracle.com> writes:
>I agree about the usefulness, but I think it should be done only when
>`bookmark-rename' is invoked interactively.

When I say "interactively", I'm including both minibuffer command
and menu command -- anything the user did interactively, that is.

>...  That's a bad workaround if the goal is to add to the history when
>the user uses the menu: non-interactively is not the same thing as
>interactively-using-a-menu.

Sure, completely agree.  We're on the same page, conceptually; the code
just needs fixing, that's all :-).  I'll try to do it, but am busy for a
while again.  Sorry for the delay.

bug#12504: `bookmark-rename' and `bookmark-maybe-historicize-string'

[Drew Adams]

> >I agree about the usefulness, but I think it should be done only when
> >`bookmark-rename' is invoked interactively.
> 
> When I say "interactively", I'm including both minibuffer command
> and menu command -- anything the user did interactively, that is.

Yes, that's what I meant here, too.

But that is not what the code does, I believe.  It adds the name to the history
even when bookmark-rename is invoked non-interactively.

> >...  That's a bad workaround if the goal is to add to the 
> > history when the user uses the menu: non-interactively is not
> > the same thing as interactively-using-a-menu.
> 
> Sure, completely agree.  We're on the same page, 
> conceptually; the code just needs fixing, that's all :-).
> I'll try to do it, but am busy for a while again.

Thanks.  But again, this really is not specific to bookmark commands.  A more
general solution should be found, IMO.

For bookmark.el, I would be happy if we did not try to add menu-invoked commands
to the histry, and we just waited for a general solution.

But I would also be happy if you implement a bookmark.el solution that does what
we mentioned above: add to the history only when invoked by the user,
interactively (menu or keys).

bug#12504: `bookmark-rename' and `bookmark-maybe-historicize-string'

[Karl Fogel]

"Drew Adams" <drew.adams@oracle.com> writes:
>Thanks.  But again, this really is not specific to bookmark commands.  A more
>general solution should be found, IMO.
>
>For bookmark.el, I would be happy if we did not try to add menu-invoked
>commands to the histry, and we just waited for a general solution.
>
>But I would also be happy if you implement a bookmark.el solution that
>does what we mentioned above: add to the history only when invoked by
>the user, interactively (menu or keys).

Understood -- and thank you for laying out the options so clearly!

bug#12504: 24.2.50; `bookmark-rename' and `bookmark-maybe-historicize-string'

[Lars Ingebrigtsen]

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

> Does invoking functions through a menu result in an environment where
> `called-interactively-p' returns non-nil?  In that case, the premise
> behind `bookmark-maybe-historicize-string' is all wrong anyway, and the
> macro should be rewritten to:
>
>   `(when (called-interactively-p 'interactive)
>      (setq bookmark-history (cons ,string bookmark-history))))

The doc string is misleading -- this isn't about normal menus, but
functions like this:

(defun bookmark-bmenu-rename ()
  "Rename bookmark on current line.  Prompts for a new name."
  (interactive nil bookmark-bmenu-mode)
  (let ((bmrk (bookmark-bmenu-bookmark))
        (thispoint (point)))
    (bookmark-rename bmrk)
    (goto-char thispoint)))

So I've now updated the doc string.

> By the way, your guess is right: it's useful (I think) to have the old
> name in the history for `bookmark-rename', because someone may want to
> use it or a variant of it in another bookmark soon.  History is cheap
> that way: it's better to have a little junk than to *not* have the thing
> the user needs when they need it.

So I think this is working as designed, and I'm therefore closing this
bug report.

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

bug#12504: 24.2.50; `bookmark-rename' and `bookmark-maybe-historicize-string'

[Karl Fogel]

On 04 Dec 2021, Lars Ingebrigtsen wrote:
>Karl Fogel <kfogel@red-bean.com> writes:
>The doc string is misleading -- this isn't about normal menus, 
>but
>functions like this:
>
>(defun bookmark-bmenu-rename ()
>  "Rename bookmark on current line.  Prompts for a new name."
>  (interactive nil bookmark-bmenu-mode)
>  (let ((bmrk (bookmark-bmenu-bookmark))
>        (thispoint (point)))
>    (bookmark-rename bmrk)
>    (goto-char thispoint)))
>
>So I've now updated the doc string.

Thank you, and...

>So I think this is working as designed, and I'm therefore closing 
>this
>bug report.

...agreed.

Best regards,
-Karl