[PATCH] Add user option to disable location in bookmarks

[PATCH] Add user option to disable location in bookmarks

[Jamie Beardslee]

[-- Attachment #1.1: Type: text/plain, Size: 238 bytes --]

Hi,

I've just started using save-place-mode, and found that bookmarks really
get in the way.  I figure there are probably a lot of people that use
bookmarks just to save some common files, rather than locations within a
file.

--
Jamie


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Add-user-option-to-disable-location-in-bookmarks.patch --]
[-- Type: text/x-patch, Size: 2584 bytes --]

From 17db59cbb7e060bed4c181f459b80cb67c1163b5 Mon Sep 17 00:00:00 2001
From: Jamie Beardslee <beardsleejamie@gmail.com>
Date: Sun, 21 Jun 2020 06:59:12 +1200
Subject: [PATCH] Add user option to disable location in bookmarks

When `bookmark-use-location' set to nil, bookmark-default-handler
won't try to jump to the right location in the file.

This way bookmarks can be used to just save regularly used files, and
save-place (or similar) can be relied on to, well, save the place.
---
 lisp/bookmark.el | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 5bb1698171..3687a0b20f 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -170,6 +170,10 @@ bookmark-menu-heading
   "Face used to highlight the heading in bookmark menu buffers."
   :version "22.1")
 
+(defcustom bookmark-use-location t
+  "Whether to open the specific location of bookmarked files.
+See also `save-place-mode'."
+  :type 'boolean)
 
 ;;; No user-serviceable parts beyond this point.
 
@@ -1224,16 +1228,17 @@ bookmark-default-handler
       ((and buf (get-buffer buf)))
       (t ;; If not, raise error.
        (signal 'bookmark-error-no-filename (list 'stringp file)))))
-    (if place (goto-char place))
-    ;; Go searching forward first.  Then, if forward-str exists and
-    ;; was found in the file, we can search backward for behind-str.
-    ;; Rationale is that if text was inserted between the two in the
-    ;; file, it's better to be put before it so you can read it,
-    ;; rather than after and remain perhaps unaware of the changes.
-    (when (and forward-str (search-forward forward-str (point-max) t))
-      (goto-char (match-beginning 0)))
-    (when (and behind-str (search-backward behind-str (point-min) t))
-      (goto-char (match-end 0)))
+    (when bookmark-use-location
+      (if place (goto-char place))
+      ;; Go searching forward first.  Then, if forward-str exists and
+      ;; was found in the file, we can search backward for behind-str.
+      ;; Rationale is that if text was inserted between the two in the
+      ;; file, it's better to be put before it so you can read it,
+      ;; rather than after and remain perhaps unaware of the changes.
+      (when (and forward-str (search-forward forward-str (point-max) t))
+        (goto-char (match-beginning 0)))
+      (when (and behind-str (search-backward behind-str (point-min) t))
+        (goto-char (match-end 0))))
     nil))
 
 ;;;###autoload
-- 
2.27.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 519 bytes --]

RE: [PATCH] Add user option to disable location in bookmarks

[Drew Adams]

> I've just started using save-place-mode, and found that bookmarks really
> get in the way.  I figure there are probably a lot of people that use
> bookmarks just to save some common files, rather than locations within a
> file.

(FYI: Half of your message ended up as an attachment.  At least that's how my email client, Outlook, delivered it.  Maybe a problem at my end; dunno.)

You don't specify what the actual problem is.  How does
a bookmark interfere with save-place?  Maybe provide a
step-by-step recipe to show the problem, starting from
`emacs -Q' (no init file).

Just what behavior are you looking for?  Do you not want
to open the file at all?  (If so, what do you want the
bookmark to do with the file, instead of opening it?)

If you want the bookmark to open the file, where do you
want the cursor to be in it - at what position?

You can create a bookmark to (almost) anything - a file
is just one possibility.  And a bookmark can do (almost)
anything with the thing it targets.

A bookmark can target a file in any way you want,
including (but not limited to) opening it at a given
position.  You can easily bookmark a file so that the
bookmark opens it with the cursor at position 1, for
example.  (And you need not visit a file in order to
bookmark it.)

Finally, a bookmark need not have any `location' field.

In sum, it's not clear to me: (1) what problem you want
to report, (2) why you think the solution is to not
include a `location' field, and (3) why you (apparently)
think that a bookmark requires a `location' field.

Re: [PATCH] Add user option to disable location in bookmarks

[Jamie Beardslee]

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

> You don't specify what the actual problem is.  How does a bookmark
> interfere with save-place?  Maybe provide a step-by-step recipe to
> show the problem, starting from `emacs -Q' (no init file).

‘bookmark-set’ saves the position--by which I mean point and context
strings--with no option (that I can find) of not saving it.  When using
‘bookmark-jump’, point will always be moved to that position, ignoring
the position from save-place.

> Just what behavior are you looking for?  Do you not want to open the
> file at all?  (If so, what do you want the bookmark to do with the
> file, instead of opening it?)
>
> If you want the bookmark to open the file, where do you want the
> cursor to be in it - at what position?

My patch allows the user to choose whether or not the bookmark record
should be in control of the position.

> A bookmark can target a file in any way you want, including (but not
> limited to) opening it at a given position.  You can easily bookmark a
> file so that the bookmark opens it with the cursor at position 1, for
> example.  (And you need not visit a file in order to bookmark it.)
>
> Finally, a bookmark need not have any `location' field.

I understand this.  I think it would be useful to have a simple
accessible way to effectively disable the point manipulation part of
bookmarks.  It’s not exactly convenient to edit the bookmark file by
hand every time a bookmark is created just to disable this feature.

--
Jamie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 519 bytes --]

Re: [PATCH] Add user option to disable location in bookmarks

[Stefan Monnier]

> ‘bookmark-set’ saves the position--by which I mean point and context
> strings--with no option (that I can find) of not saving it.
> When using ‘bookmark-jump’, point will always be moved to that
> position, ignoring the position from save-place.

Yes, that's on purpose.

> My patch allows the user to choose whether or not the bookmark record
> should be in control of the position.

I think it makes sense, but I think it shouldn't be a global option.
Maybe it should be a setting saved with the bookmark (i.e. when saving
the bookmark you could specify not to save the location) or otherwise
specified when jumping.

Maybe a prefix command which causes the subsequent `bookmark-jump` to
ignore the location info (and it could also cause a subsequent
`bookmark-set` not to save the location)?


        Stefan

RE: [PATCH] Add user option to disable location in bookmarks

[Drew Adams]

> > You don't specify what the actual problem is.  How does a bookmark
> > interfere with save-place?  Maybe provide a step-by-step recipe to
> > show the problem, starting from `emacs -Q' (no init file).
> 
> ‘bookmark-set’ saves the position--by which I mean point and context
> strings--with no option (that I can find) of not saving it.  When using
> ‘bookmark-jump’, point will always be moved to that position, ignoring
> the position from save-place.

That's not a recipe.

A bookmark can do anything.  It can even make use of
save-place or any save-place info.

You can create whatever kind of bookmark you need, if
you need something special.  See
`bookmark-make-record-function', and see how it's used
in the Elisp sources to create different kinds of
bookmarks.

`bookmark-set' uses `bookmark-make-record', which calls
the function value of `bookmark-make-record-function'.

It's only the default value of variable
`bookmark-make-record-function' that does what you say.

If the default `bookmark-make-record-function' doesn't
do what's needed for save-place, then maybe save-place
should define its own type of bookmark.

Or probably all it needs to do is `add-hook' for
`bookmark-after-jump-hook', just as it does for
`find-file-hook', `dired-initial-position-hook', and
`kill-emacs-hook'.  IOW, after jumping to the position
that you bookmarked, move to the save-place position.

It's you who bookmarked a given position.  And likely
you do sometimes want to return to that position.  But
if there's a more recent position in that file, which
save-place has saved for you, then just use that one,
via `bookmark-after-jump-hook'.

(And again, a bookmark need not have a location.  If
you use the default make-record function then yes,
the bookmark saves a position.)

> > Just what behavior are you looking for?  Do you not want to open the
> > file at all?  (If so, what do you want the bookmark to do with the
> > file, instead of opening it?)
> >
> > If you want the bookmark to open the file, where do you want the
> > cursor to be in it - at what position?
> 
> My patch allows the user to choose whether or not the bookmark record
> should be in control of the position.

The bookmark record has already chosen that.  Just
use a different kind of record.  Why have an option
that acts in a blanket manner on all bookmarks, to
ignore their position?

If this is about save-place then save-place should
do the right thing.  See above.  If there's an option
to be added in that regard, it's a save-place option:
`save-place-ignore-bookmark-position'.  The hook
function (see above) can move to the save-place place
if the option is non-nil, and just do nothing if it's
nil.

> > A bookmark can target a file in any way you want, including (but not
> > limited to) opening it at a given position.  You can easily bookmark a
> > file so that the bookmark opens it with the cursor at position 1, for
> > example.  (And you need not visit a file in order to bookmark it.)
> >
> > Finally, a bookmark need not have any `location' field.
> 
> I understand this.  I think it would be useful to have a simple
> accessible way to effectively disable the point manipulation part of
> bookmarks.  It’s not exactly convenient to edit the bookmark file by
> hand every time a bookmark is created just to disable this feature.

No one suggested that you edit a bookmark file.

Your cure isn't localized to the ill.  If this is a
good option to give users, to let save-place override
bookmark locations, just like it does for Dired and
`find-file', then that option belongs to save-place.

RE: [PATCH] Add user option to disable location in bookmarks

[Drew Adams]

> I think it makes sense, but I think it shouldn't be a global option.
> Maybe it should be a setting saved with the bookmark (i.e. when saving
> the bookmark you could specify not to save the location) or otherwise
> specified when jumping.
> 
> Maybe a prefix command which causes the subsequent `bookmark-jump` to
> ignore the location info (and it could also cause a subsequent
> `bookmark-set` not to save the location)?

Please, no.  Overkill, overoverkill.

Just have save-place do what it does for
`find-file' and `dired': move to the save-place
place after the function does its thing.

`bookmark-after-jump-hook'.

Re: [PATCH] Add user option to disable location in bookmarks

[Jamie Beardslee]

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

>> My patch allows the user to choose whether or not the bookmark record
>> should be in control of the position.
>
> The bookmark record has already chosen that.  Just use a different
> kind of record.  Why have an option that acts in a blanket manner on
> all bookmarks, to ignore their position?

I don't see the problem here, surely some users would want to ignore
bookmarks' positions.  There isn't a similar built-in library that
doesn't save position so I feel it should be an option provided by
bookmark.el without the user needing to define their own handler.

> If this is about save-place then save-place should do the right thing.
> See above.  If there's an option to be added in that regard, it's a
> save-place option: `save-place-ignore-bookmark-position'.  The hook
> function (see above) can move to the save-place place if the option is
> non-nil, and just do nothing if it's nil.

Okay, well I've attached that patch here.  I understand this is skipping
over one of the best features of bookmarks, but I still think it's a
valuable option even without the use of save-place.

--
Jamie


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Allow-save-place-to-override-the-position-when-using.patch --]
[-- Type: text/x-patch, Size: 1797 bytes --]

From c4378882ef88fafcd04017d1942f673658b04dd1 Mon Sep 17 00:00:00 2001
From: Jamie Beardslee <beardsleejamie@gmail.com>
Date: Sun, 21 Jun 2020 11:59:44 +1200
Subject: [PATCH] Allow save-place to override the position when using
 bookmarks.

When the user option `save-place-override-bookmark' is non-nil,
save-place will go to the last position rather than the position
specified in the bookmark record.
---
 lisp/saveplace.el | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lisp/saveplace.el b/lisp/saveplace.el
index 46738ab03d..f32c1ad753 100644
--- a/lisp/saveplace.el
+++ b/lisp/saveplace.el
@@ -118,12 +118,22 @@ save-place-ignore-files-regexp
   :version "24.1"
   :type 'regexp)
 
+(defcustom save-place-override-bookmark nil
+  "Whether `save-place' should override bookmarks.
+
+Normally, a bookmark is used to save a specific point in a buffer
+or file.  If this is non-nil, `save-place' will provide the
+position instead."
+  :type 'boolean)
+
 (declare-function dired-current-directory "dired" (&optional localp))
 
 (defun save-place--setup-hooks (add)
   (cond
    (add
     (add-hook 'find-file-hook #'save-place-find-file-hook t)
+    (eval-after-load "bookmark"
+      '(add-hook 'bookmark-after-jump-hook #'save-place-bookmark-hook t))
     (add-hook 'dired-initial-position-hook #'save-place-dired-hook)
     (unless noninteractive
       (add-hook 'kill-emacs-hook #'save-place-kill-emacs-hook))
@@ -334,6 +344,10 @@ save-place-find-file-hook
           ;; and make sure it will be saved again for later
           (setq save-place-mode t)))))
 
+(defun save-place-bookmark-hook ()
+  (when save-place-override-bookmark
+    (save-place-find-file-hook)))
+
 (declare-function dired-goto-file "dired" (file))
 
 (defun save-place-dired-hook ()
-- 
2.27.0

RE: [PATCH] Add user option to disable location in bookmarks

[Drew Adams]

> >> My patch allows the user to choose whether or not the bookmark record
> >> should be in control of the position.
> >
> > The bookmark record has already chosen that.  Just use a different
> > kind of record.  Why have an option that acts in a blanket manner on
> > all bookmarks, to ignore their position?
> 
> I don't see the problem here, surely some users would want to ignore
> bookmarks' positions.

Positions of all bookmarks, of any type?
Maybe there are such users.  But my point was
that such a shotgun approach isn't needed, to
solve the specific save-place problem.  Just
do it for save-place (and optionally, per a
user option).

> There isn't a similar built-in library that
> doesn't save position so I feel it should be an option provided by
> bookmark.el without the user needing to define their own handler.
> 
> > If this is about save-place then save-place should do the right thing.
> > See above.  If there's an option to be added in that regard, it's a
> > save-place option: `save-place-ignore-bookmark-position'.  The hook
> > function (see above) can move to the save-place place if the option is
> > non-nil, and just do nothing if it's nil.
> 
> Okay, well I've attached that patch here.  I understand this is skipping
> over one of the best features of bookmarks, but I still think it's a
> valuable option even without the use of save-place.

Yes, that's the idea.  (Didn't try it, but a
priori LGTM.)

Re: [PATCH] Add user option to disable location in bookmarks

[Yuri Khan]

On Sun, 21 Jun 2020 at 06:34, Drew Adams <drew.adams@oracle.com> wrote:

> If this is about save-place then save-place should
> do the right thing.  See above.  If there's an option
> to be added in that regard, it's a save-place option:
> `save-place-ignore-bookmark-position'.  The hook
> function (see above) can move to the save-place place
> if the option is non-nil, and just do nothing if it's
> nil.

I think you are too quick in punting the change to ‘save-place’.

I think it’s a matter of user expectations.

You expect that a bookmark targets a particular location in the file.
(There are bookmarks that do not target a file; let’s ignore them for
now.) Activating the bookmark visits that file (if not already
visited), displays that file’s buffer, and jumps to the bookmarked
location (adjusting it by searching for context if necessary).

Jamie expects that a bookmark targets the file in its entirety.
Activating the bookmark then should visit the file (if not already
visited) and display the file’s buffer. If the file had to be
re-visited, save-place kicks in and restores the point and scroll
position to the values saved when the file’s buffer was killed. If
save-place is not active, the point and scroll position should remain
whatever they end up by default (top of buffer?).

I dare say both expectations are valid for files. Therefore, the fix
should allow the user to avoid saving a location in bookmarks
targeting files, perhaps by introducing a setting like
‘bookmark-set-save-location’, boolean, default t. Other bookmark types
would then decide if it makes sense to honor that setting, and how.
E.g. an Info buffer technically contains a whole Info manual but,
through narrowing, makes an appearance of displaying only a single
Info page; it would make sense to bookmark the page but not the exact
line and context.

Re: [PATCH] Add user option to disable location in bookmarks

[Basil L. Contovounesios]

Jamie Beardslee <beardsleejamie@gmail.com> writes:

>  (defun save-place--setup-hooks (add)
>    (cond
>     (add
>      (add-hook 'find-file-hook #'save-place-find-file-hook t)
> +    (eval-after-load "bookmark"
> +      '(add-hook 'bookmark-after-jump-hook #'save-place-bookmark-hook t))

with-eval-after-load is preferred to eval-after-load, but is it
necessary?  add-hook knows how to deal with as of yet undefined hook
variables.

>      (add-hook 'dired-initial-position-hook #'save-place-dired-hook)
>      (unless noninteractive
>        (add-hook 'kill-emacs-hook #'save-place-kill-emacs-hook))
> @@ -334,6 +344,10 @@ save-place-find-file-hook
>            ;; and make sure it will be saved again for later
>            (setq save-place-mode t)))))

-- 
Basil

RE: [PATCH] Add user option to disable location in bookmarks

[Drew Adams]

> > If this is about save-place then save-place should
> > do the right thing.  See above.  If there's an option
> > to be added in that regard, it's a save-place option:
> > `save-place-ignore-bookmark-position'.  The hook
> > function (see above) can move to the save-place place
> > if the option is non-nil, and just do nothing if it's
> > nil.
> 
> I think you are too quick in punting the change to ‘save-place’.
>
> I think it’s a matter of user expectations.

Of course it is.  User expectations, common use
cases, and individual preferences.

The _general_ expectation is realized in the default
make-record function.  And in the default jump-to
behavior.  That's why they are defaults.  But they're
only defaults.

> You expect that a bookmark targets a particular location in the file.

No.  I don't expect anything particular from a given
bookmark, other than what it advertises.

Emacs users, in general, expect, by default, what
the default behavior provides.  That's why it's the
default.

> (There are bookmarks that do not target a file;
> let’s ignore them for now.)

First, not targeting a file is not the same thing
as not targeting a particular location in a file.

But if you really mean not targeting a position
in a file, why ignore the case of bookmarks that
don't target such locations, if that's just what
this user's looking for?

If you as a particular user, or a particular mode
or library author, want all bookmarks used in some
context - or even all bookmarks - to not go to a
specific file position then why create bookmarks
that do that?  Why use the _default_ make-record
function to create bookmarks for that use case?

That's the first point.  There's no requirement to
use the default make-record function, if you want
a custom behavior.

> Activating the bookmark visits that file (if not already
> visited), displays that file’s buffer, and jumps to the bookmarked
> location (adjusting it by searching for context if necessary).

By default, yes.  That's the default behavior for
jumping to a bookmark, including a file-visiting
bookmark. 

> Jamie expects that a bookmark targets the file in its entirety.
> Activating the bookmark then should visit the file (if not already
> visited) and display the file’s buffer.

I understand that.  (Visiting a file does put the
cursor at _some_ position - bob by default.)

> If the file had to be
> re-visited, save-place kicks in and restores the point and scroll
> position to the values saved when the file’s buffer was killed. If
> save-place is not active, the point and scroll position should remain
> whatever they end up by default (top of buffer?).

Yes, bob, by default, when first visiting a file.

> I dare say both expectations are valid for files.

All kinds of expectations are valid.  Whatever a
user wants and is realizable is valid.  No one's
suggested that Jamie's preferred behavior is invalid.

> Therefore, the fix should allow the user to avoid
> saving a location in bookmarks targeting files,

That doesn't follow.  A user should be able to jump
to a file bookmark and end up at the last save-place
location in that file.  That says nothing about what
gets saved in a bookmark record.

Is the need to avoid saving a location in bookmarks?
Or is the need to always visit a file bookmark at
bob (position 1)?  Or is the need to always (or only
sometimes perhaps) let a hook function adjust the
position upon visiting?

I think the need described is just to have jumping
to a file bookmark go to save-place's recorded
location within the file.

> perhaps by introducing a setting like
> ‘bookmark-set-save-location’, boolean, default t.

saveplace.el affects only file (and Dired) visits,
right?  So only file (and possibly Dired) bookmarks
present the user with this need.

> Other bookmark types would then decide if it makes
> sense to honor that setting, and how.

You mean they would _have_ to do that - decide.  Or
else they would just get their `location' settings
ignored whenever someone wanted save-place to ignore
`location' for file bookmarks.

So _every_ kind of bookmark would now need to add
logic to deal with this blanket, all-bookmarks option.

Unless, that is, for some reason some particular kind
of bookmark really wanted to make its `location' be
ignored whenever the option is enabled.  In which case,
it probably would have just not included a `location'
setting, or it too would have already felt the need for
such an option.

> E.g. an Info buffer technically contains a whole Info manual but,
> through narrowing, makes an appearance of displaying only a single
> Info page; it would make sense to bookmark the page but not the exact
> line and context.

Would it?  If necessarily so, then Info bookmarks
wouldn't have a `location' setting, would they?

Now, you can say that we could add such a blanket
user option, which makes all bookmarks, of all
kinds, have their `location' setting ignored.

And we could compensate for that blanket treatment
by providing a separate option that lists bookmark
types (or buffers or files or whatever) to exclude
from the option behavior.

Or we could incorporate that into the same option,
by making it (1) a list of type, buffer, files, or
whatever to exclude; (2) t or `all', meaning include
all; or (3) nil, meaning always respect `location'.
_____

Or we can just satisfy the need, which is for
save-place, by having save-place do what it does
for visiting files generally: invoke its hook,
already defined, for visiting a file,
`save-place-find-file-hook'.  That hook function
(which shouldn't be named `*-hook', BTW) does just
what's wanted.

(And perhaps Jamie will want to do similarly for
Dired bookmarks, using `save-place-dired-hook'.)

And the place to use such a save-place hook
function for bookmark jumping, is
`bookmark-after-jump-hook', just as the place to
use the same function for `find-file' visiting is
`find-file-hook'.

Completely comparable, IMO.