[PATCH] open bookmark in other frame

[PATCH] open bookmark in other frame

[Pierre-Yves Luyten]

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

Hello,

people sometimes use several frames, so i would like to make opening 
bookmark one shot. Attached patch adds two funcs, 
"bookmark-jump-other-frame" and the equivalent from the bookmarks menu, 
"bookmark-bmenu-other-frame"

Regards
Pierre-Yves

[-- Attachment #2: 0001-lisp-bookmark.el-other-frame.patch --]
[-- Type: text/x-patch, Size: 2820 bytes --]

From bdcf094e15a1a51a1f137e407cf36bd79b42947d Mon Sep 17 00:00:00 2001
From: Pierre-Yves Luyten <py@luyten.fr>
Date: Wed, 10 Oct 2018 21:48:07 +0200
Subject: [PATCH] * lisp/bookmark.el : other-frame

bookmark-jump-other-frame
bookmark-bmenu-other-frame
---
 lisp/bookmark.el | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 58a279473d..3973e3b80e 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -1124,6 +1124,13 @@ DISPLAY-FUNC would be `switch-to-buffer-other-window'."
                                    bookmark-current-bookmark)))
   (bookmark-jump bookmark 'switch-to-buffer-other-window))
 
+(defun bookmark-jump-other-frame (bookmark)
+  "Jump to BOOKMARK in another frame.  See `bookmark-jump' for more."
+  (interactive
+   (list (bookmark-completing-read "Jump to bookmark (in another frame)"
+                                   bookmark-current-bookmark)))
+  (bookmark-jump bookmark 'view-buffer-other-frame))
+
 
 (defun bookmark-jump-noselect (bookmark)
   "Return the location pointed to by BOOKMARK (see `bookmark-jump').
@@ -1567,6 +1574,7 @@ unique numeric suffixes \"<2>\", \"<3>\", etc."
     (define-key map "\C-c\C-c" 'bookmark-bmenu-this-window)
     (define-key map "f" 'bookmark-bmenu-this-window)
     (define-key map "\C-m" 'bookmark-bmenu-this-window)
+    (define-key map "F" 'bookmark-bmenu-other-frame)
     (define-key map "o" 'bookmark-bmenu-other-window)
     (define-key map "\C-o" 'bookmark-bmenu-switch-other-window)
     (define-key map "s" 'bookmark-bmenu-save)
@@ -1702,6 +1710,8 @@ Bookmark names preceded by a \"*\" have annotations.
 \\[bookmark-bmenu-this-window] -- select this bookmark in place of the bookmark menu buffer.
 \\[bookmark-bmenu-other-window] -- select this bookmark in another window,
   so the bookmark menu bookmark remains visible in its window.
+\\[bookmark-bmenu-other-frame] -- select this bookmark in another frame,
+  so the bookmark menu bookmark remains visible in its window.
 \\[bookmark-bmenu-switch-other-window] -- switch the other window to this bookmark.
 \\[bookmark-bmenu-rename] -- rename this bookmark (prompts for new name).
 \\[bookmark-bmenu-relocate] -- relocate this bookmark's file (prompts for new file).
@@ -1970,6 +1980,11 @@ With a prefix arg, prompts for a file to save them in."
   (let ((bookmark (bookmark-bmenu-bookmark)))
     (bookmark--jump-via bookmark 'switch-to-buffer-other-window)))
 
+(defun bookmark-bmenu-other-frame ()
+  "Select this line's bookmark in other frame, leaving bookmark menu visible."
+  (interactive)
+  (let ((bookmark (bookmark-bmenu-bookmark)))
+    (bookmark--jump-via bookmark 'view-buffer-other-frame)))
 
 (defun bookmark-bmenu-switch-other-window ()
   "Make the other window select this line's bookmark.
-- 
2.19.0

Re: [PATCH] open bookmark in other frame

[Marcin Borkowski]

On 2018-10-10, at 22:14, Pierre-Yves Luyten <py@luyten.fr> wrote:

> Hello,
>
> people sometimes use several frames, so i would like to make opening 
> bookmark one shot. Attached patch adds two funcs, 
> "bookmark-jump-other-frame" and the equivalent from the bookmarks menu, 
> "bookmark-bmenu-other-frame"

Great idea!

How about a default keybinding?

Best,

-- 
Marcin Borkowski
http://mbork.pl

Re: [PATCH] open bookmark in other frame

[Karl Fogel]

Marcin Borkowski <mbork@mbork.pl> writes:
>> people sometimes use several frames, so i would like to make opening 
>> bookmark one shot. Attached patch adds two funcs, 
>> "bookmark-jump-other-frame" and the equivalent from the bookmarks menu, 
>> "bookmark-bmenu-other-frame"
>
>Great idea!
>
>How about a default keybinding?

I also like this patch.  (Marcin, I think Pierre-Yves did include a keybinding: "F" in the bookmark-map -- unless you meant something else by "default keybinding"?)

Pierre-Yves, have you read the CONTRIBUTE file at the top of the Emacs source tree (in particular the bit at the top, that points to https://www.gnu.org/software/emacs/manual/html_node/emacs/Contributing.html)?  While a plain 'git diff' is fine, 'git format-patch ...' might make it easier to preserve attribution in the most git-like manner.  However, we'll be able to merge it either way.

Also, that way we get your commit message (log message).  You can look at the existing commit history for examples of commit messages, and the CONTRIBUTE file has some guidance as well.

Best regards,
-Karl

Re: [PATCH] open bookmark in other frame

[Pierre-Yves Luyten]

Le 2018-10-10 23:35, Karl Fogel a écrit :
> Marcin Borkowski <mbork@mbork.pl> writes:

>> 
>> How about a default keybinding?
> 
> (Marcin, I think Pierre-Yves did include a
> keybinding: "F" in the bookmark-map -- unless you meant something else
> by "default keybinding"?)
> 

As discussed, i will change the "F" to 5 in bookmarks list. But i had no 
plan to include a keybinding outside of menu list. As of today i can see 
below bindings.

+ bookmark-jump (C-x r b)
+ bookmark-set (C-x r m)
+ bookmark-bmenu-list (C-x r l)

What i learned writing the patch is the existence of "bookmark-map" 
which is not bound by default and offers all interfactive bookmark 
related functions. So, i will amend patch to add the "other-frame" in 
this specific map, but i will not add in Ctl-x map.

Regards
Pierre-Yves

RE: [PATCH] open bookmark in other frame

[Drew Adams]

> people sometimes use several frames, so i would like to make opening
> bookmark one shot. Attached patch adds two funcs,
> "bookmark-jump-other-frame" and the equivalent from the bookmarks menu,
> "bookmark-bmenu-other-frame"

My suggestions in this regard, FWIW:

1. Don't use `view-buffer-other-frame'.
    Select the buffer, and not just read-only. Jumping to a bookmark
    typically puts you at its location (hence select), and the buffer is
    typically not put in a read-only mode. IOW, do the equivalent of
    this, or similar:

    (let ((pop-up-frames  t)) (bookmark-jump-other-window bookmark)

2. Don't use `F' as the key binding in the bookmark-list buffer.
    `F' is more often used for files than for frames. Maybe use `5'.
 
    (I use `J 5' in Bookmark+. `J' is a prefix for the jump commands
    in the bookmark-list buffer. The `5' is from `C-x 5' bindings for
    other-frame. I bind the command to `C-x 5 B' and `C-x j 5' globally.
    `C-x j' is a global prefix key for bookmark jump commands.)

Re: [PATCH] open bookmark in other frame

[Karl Fogel]

Drew Adams <drew.adams@oracle.com> writes:
>My suggestions in this regard, FWIW:
>
>1. Don't use `view-buffer-other-frame'.
>    Select the buffer, and not just read-only. Jumping to a bookmark
>    typically puts you at its location (hence select), and the buffer is
>    typically not put in a read-only mode. IOW, do the equivalent of
>    this, or similar:
>
>    (let ((pop-up-frames  t)) (bookmark-jump-other-window bookmark)

Agreed -- there's no reason it should be read-only.  Jumping to a bookmark doesn't normally cause the destination to be read-only, so there's no reason that should happen just because we're in a new frame.

(I haven't studied `pop-up-frames' enough to know whether Drew's back-of-the-envelope solution above is the best way, but presumably a little more research would lead to the best way.)

>2. Don't use `F' as the key binding in the bookmark-list buffer.
>    `F' is more often used for files than for frames. Maybe use `5'.
> 
>    (I use `J 5' in Bookmark+. `J' is a prefix for the jump commands
>    in the bookmark-list buffer. The `5' is from `C-x 5' bindings for
>    other-frame. I bind the command to `C-x 5 B' and `C-x j 5' globally.
>    `C-x j' is a global prefix key for bookmark jump commands.)

I don't have a strong opinion here, but my fingers also lean slightly toward "5" because of `C-x 5 b', for what it's worth.

Thanks for noticing that `view-buffer-other-frame' leads to read-onlyness, Drew, and for the "5" idea.

Best regards,
-Karl

Re: [PATCH] open bookmark in other frame

[Pierre-Yves Luyten]

Le 2018-10-11 09:19, Karl Fogel a écrit :
> Drew Adams <drew.adams@oracle.com> writes:
>> My suggestions in this regard, FWIW:
>> 
>> 1. Don't use `view-buffer-other-frame'.
>> (...)
> 
> Agreed -- there's no reason it should be read-only.  Jumping to a
> bookmark doesn't normally cause the destination to be read-only, so
> there's no reason that should happen just because we're in a new
> frame.

noted i will review this

> 
>> 2. Don't use `F' as the key binding in the bookmark-list buffer.
>>    `F' is more often used for files than for frames. Maybe use `5'.
>> 
>> (...)
> 
> I don't have a strong opinion here, but my fingers also lean slightly
> toward "5" because of `C-x 5 b', for what it's worth.
> 

now you suggest 5 it looks like a nice way to go.

I will review patch in the upcoming days to solve these two points and 
improve commit message

Regards
Pierre-Yves

RE: [PATCH] open bookmark in other frame

[Drew Adams]

> > IOW, do the equivalent of this, or similar:
> >    (let ((pop-up-frames  t)) (bookmark-jump-other-window bookmark)
> 
> (I haven't studied `pop-up-frames' enough to know whether Drew's back-of-
> the-envelope solution above is the best way, but presumably a little more
> research would lead to the best way.)

Non-nil `pop-up-frames' just makes "other-window" commands
use "other-frame".

I wrote "the equivalent of this, or similar", because someone
will perhaps say that there is a more "modern" way to do the
same thing, involving a `display-buffer' ACTION or something.

I still keep things simple in my own code, partly for backward
compatibility. The above code DTRT, but someone might
prefer a different formulation that does the same thing, or
similar.

Re: [PATCH] open bookmark in other frame

[Pierre-Yves Luyten]

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

On 10/11/18 12:06 AM, Drew Adams wrote:
>> people sometimes use several frames, so i would like to make opening
>> bookmark one shot. Attached patch adds two funcs,
>> "bookmark-jump-other-frame" and the equivalent from the bookmarks menu,
>> "bookmark-bmenu-other-frame"
> 
> My suggestions in this regard, FWIW:
> 
> 1. Don't use `view-buffer-other-frame'.
>      Select the buffer, and not just read-only. Jumping to a bookmark
>      typically puts you at its location (hence select), and the buffer is
>      typically not put in a read-only mode. IOW, do the equivalent of
>      this, or similar:
> 
>      (let ((pop-up-frames  t)) (bookmark-jump-other-window bookmark)
> 
> 2. Don't use `F' as the key binding in the bookmark-list buffer.
>      `F' is more often used for files than for frames. Maybe use `5'.
>   
>      (I use `J 5' in Bookmark+. `J' is a prefix for the jump commands
>      in the bookmark-list buffer. The `5' is from `C-x 5' bindings for
>      other-frame. I bind the command to `C-x 5 B' and `C-x j 5' globally.
>      `C-x j' is a global prefix key for bookmark jump commands.)
> 

So here is the new version of the patch
1. Use pop-up-frames variable to avoid a read-only mode on new frame.
    I also had to use (other-frame 1) to ensure new frame is raised.

2. Use "5" binding both in bookmark-map, and bookmark-bmenu-mode-map

3. Split in two commits, one per new function
    I tried to respect the conventions on the log.

The patches are generated using git format-patch.

Regards
Pierre-Yves

[-- Attachment #2: 0001-lisp-bookmark.el-bookmark-jump-other-frame-new-funct.patch --]
[-- Type: text/x-patch, Size: 1621 bytes --]

From fb477d9f3bfc39f18788aa735dc70f542b513377 Mon Sep 17 00:00:00 2001
From: Pierre-Yves Luyten <py@luyten.fr>
Date: Thu, 11 Oct 2018 23:40:32 +0200
Subject: [PATCH 1/2] * lisp/bookmark.el (bookmark-jump-other-frame): new
 function

  Add the new function bookmark-jump-other-frame
  Bind to bookmark-map
---
 lisp/bookmark.el | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 58a279473d..5ee5248b97 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -209,6 +209,7 @@ A non-nil value may result in truncated bookmark names."
     (define-key map "j" 'bookmark-jump)
     (define-key map "g" 'bookmark-jump) ;"g"o
     (define-key map "o" 'bookmark-jump-other-window)
+    (define-key map "5" 'bookmark-jump-other-frame)
     (define-key map "i" 'bookmark-insert)
     (define-key map "e" 'edit-bookmarks)
     (define-key map "f" 'bookmark-insert-location) ;"f"ind
@@ -1124,6 +1125,14 @@ DISPLAY-FUNC would be `switch-to-buffer-other-window'."
                                    bookmark-current-bookmark)))
   (bookmark-jump bookmark 'switch-to-buffer-other-window))
 
+(defun bookmark-jump-other-frame (bookmark)
+  "Jump to BOOKMARK in another frame.  See `bookmark-jump' for more."
+  (interactive
+   (list (bookmark-completing-read "Jump to bookmark (in another frame)"
+                                   bookmark-current-bookmark)))
+  (let ((pop-up-frames t))
+    (bookmark-jump bookmark 'display-buffer))
+  (other-frame 1))
 
 (defun bookmark-jump-noselect (bookmark)
   "Return the location pointed to by BOOKMARK (see `bookmark-jump').
-- 
2.19.0


[-- Attachment #3: 0002-lisp-bookmark.el-bookmark-bmenu-other-frame-new-func.patch --]
[-- Type: text/x-patch, Size: 2192 bytes --]

From d059307d9776cb316816eecd01b6115a0335f857 Mon Sep 17 00:00:00 2001
From: Pierre-Yves Luyten <py@luyten.fr>
Date: Thu, 11 Oct 2018 23:41:44 +0200
Subject: [PATCH 2/2] * lisp/bookmark.el (bookmark-bmenu-other-frame): new
 function

  Add bookmark-bmenu-other-frame function
  Document it
  Bind function to bookmark-bmenu-mode-map
---
 lisp/bookmark.el | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 5ee5248b97..e89252960c 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -1570,6 +1570,7 @@ unique numeric suffixes \"<2>\", \"<3>\", etc."
     (set-keymap-parent map special-mode-map)
     (define-key map "v" 'bookmark-bmenu-select)
     (define-key map "w" 'bookmark-bmenu-locate)
+    (define-key map "5" 'bookmark-bmenu-other-frame)
     (define-key map "2" 'bookmark-bmenu-2-window)
     (define-key map "1" 'bookmark-bmenu-1-window)
     (define-key map "j" 'bookmark-bmenu-this-window)
@@ -1711,6 +1712,8 @@ Bookmark names preceded by a \"*\" have annotations.
 \\[bookmark-bmenu-this-window] -- select this bookmark in place of the bookmark menu buffer.
 \\[bookmark-bmenu-other-window] -- select this bookmark in another window,
   so the bookmark menu bookmark remains visible in its window.
+\\[bookmark-bmenu-other-frame] -- select this bookmark in another frame,
+  so the bookmark menu bookmark remains visible in its window.
 \\[bookmark-bmenu-switch-other-window] -- switch the other window to this bookmark.
 \\[bookmark-bmenu-rename] -- rename this bookmark (prompts for new name).
 \\[bookmark-bmenu-relocate] -- relocate this bookmark's file (prompts for new file).
@@ -1979,6 +1982,12 @@ With a prefix arg, prompts for a file to save them in."
   (let ((bookmark (bookmark-bmenu-bookmark)))
     (bookmark--jump-via bookmark 'switch-to-buffer-other-window)))
 
+(defun bookmark-bmenu-other-frame ()
+  "Select this line's bookmark in other frame, leaving bookmark menu visible."
+  (interactive)
+  (let ((pop-up-frames t))
+    (bookmark-jump bookmark 'display-buffer))
+  (other-frame 1))
 
 (defun bookmark-bmenu-switch-other-window ()
   "Make the other window select this line's bookmark.
-- 
2.19.0

RE: [PATCH] open bookmark in other frame

[Drew Adams]

> > My suggestions in this regard, FWIW:
> >
> > 1. Don't use `view-buffer-other-frame'.
> >      Select the buffer, and not just read-only. Jumping to a bookmark
> >      typically puts you at its location (hence select), and the buffer is
> >      typically not put in a read-only mode. IOW, do the equivalent of
> >      this, or similar:
> >
> >      (let ((pop-up-frames  t)) (bookmark-jump-other-window bookmark)
> >
> > 2. Don't use `F' as the key binding in the bookmark-list buffer.
> >      `F' is more often used for files than for frames. Maybe use `5'.
> >
> >      (I use `J 5' in Bookmark+. `J' is a prefix for the jump commands
> >      in the bookmark-list buffer. The `5' is from `C-x 5' bindings for
> >      other-frame. I bind the command to `C-x 5 B' and `C-x j 5' globally.
> >      `C-x j' is a global prefix key for bookmark jump commands.)
> 
> So here is the new version of the patch
> 1. Use pop-up-frames variable to avoid a read-only mode on new frame.
>     I also had to use (other-frame 1) to ensure new frame is raised.

1. `pop-up-frames' has nothing to do with read-only. It just makes an "other-window" function use another frame instead of another window. If you use `bookmark-jump-other-window' instead of `bookmark-jump' then you don't need to also use `(other-frame 1)'. See the code I sent.

2. I don't think you need to include this in the doc string:

    , so the bookmark menu bookmark remains visible in its window.

That text is present for the other-window case, to emphasize that the same window is not reused. (It's not really needed there either, but it can help.) If we say that the bookmark is selected in another frame then it's pretty clear that we don't reuse the original window.

HTH.

Re: [PATCH] open bookmark in other frame

[Pierre-Yves Luyten]

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

On 10/12/18 12:04 AM, Drew Adams wrote
>>
>> So here is the new version of the patch
>> 1. Use pop-up-frames variable to avoid a read-only mode on new frame.
>>      I also had to use (other-frame 1) to ensure new frame is raised.
> 
> 1. `pop-up-frames' has nothing to do with read-only. 

Sure, i meant i now use pop-up-frames instead of 
view-buffer-other-frame, which was the issue.

> If you use `bookmark-jump-other-window' instead of `bookmark-jump' then you don't need to also use `(other-frame 1)'. See the code I sent.

Great, so i attach a new version of the patch that get rid of 
(other-frame 1) and still works. Thanks for the help.


> 2. I don't think you need to include this in the doc string:
> 
>      , so the bookmark menu bookmark remains visible in its window.
> 

I agree this might be too obvious. So i also removed useless verbiage in 
this version.

Regards
Pierre-Yves

[-- Attachment #2: 0001-lisp-bookmark.el-bookmark-jump-other-frame-new-funct.patch --]
[-- Type: text/x-patch, Size: 1579 bytes --]

From 9c3f6774413f9c9316eceafde98f1829e5c06dbd Mon Sep 17 00:00:00 2001
From: Pierre-Yves Luyten <py@luyten.fr>
Date: Fri, 12 Oct 2018 21:32:45 +0200
Subject: [PATCH 1/2] * lisp/bookmark.el (bookmark-jump-other-frame): new
 function

 Add bookmark-jump-other-frame
 Bind to bookmark-map
---
 lisp/bookmark.el | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 58a279473d..9d55c4aada 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -209,6 +209,7 @@ A non-nil value may result in truncated bookmark names."
     (define-key map "j" 'bookmark-jump)
     (define-key map "g" 'bookmark-jump) ;"g"o
     (define-key map "o" 'bookmark-jump-other-window)
+    (define-key map "5" 'bookmark-jump-other-frame)
     (define-key map "i" 'bookmark-insert)
     (define-key map "e" 'edit-bookmarks)
     (define-key map "f" 'bookmark-insert-location) ;"f"ind
@@ -1124,6 +1125,13 @@ DISPLAY-FUNC would be `switch-to-buffer-other-window'."
                                    bookmark-current-bookmark)))
   (bookmark-jump bookmark 'switch-to-buffer-other-window))
 
+(defun bookmark-jump-other-frame (bookmark)
+  "Jump to BOOKMARK in another frame.  See `bookmark-jump' for more."
+  (interactive
+   (list (bookmark-completing-read "Jump to bookmark (in another frame)"
+                                   bookmark-current-bookmark)))
+  (let ((pop-up-frames t))
+    (bookmark-jump-other-window bookmark)))
 
 (defun bookmark-jump-noselect (bookmark)
   "Return the location pointed to by BOOKMARK (see `bookmark-jump').
-- 
2.19.0


[-- Attachment #3: 0002-lisp-bookmark.el-bookmark-bmenu-other-frame-new-func.patch --]
[-- Type: text/x-patch, Size: 2082 bytes --]

From 31ede454672c727c079b5576053e52639ba94fcc Mon Sep 17 00:00:00 2001
From: Pierre-Yves Luyten <py@luyten.fr>
Date: Fri, 12 Oct 2018 21:35:45 +0200
Subject: [PATCH 2/2] * lisp/bookmark.el (bookmark-bmenu-other-frame):new
 function

 Add bookmark-bmenu-other-frame
 Bind to bookmark-bmenu-mode-map
---
 lisp/bookmark.el | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 9d55c4aada..7f73c22267 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -1569,6 +1569,7 @@ unique numeric suffixes \"<2>\", \"<3>\", etc."
     (set-keymap-parent map special-mode-map)
     (define-key map "v" 'bookmark-bmenu-select)
     (define-key map "w" 'bookmark-bmenu-locate)
+    (define-key map "5" 'bookmark-bmenu-other-frame)
     (define-key map "2" 'bookmark-bmenu-2-window)
     (define-key map "1" 'bookmark-bmenu-1-window)
     (define-key map "j" 'bookmark-bmenu-this-window)
@@ -1710,6 +1711,7 @@ Bookmark names preceded by a \"*\" have annotations.
 \\[bookmark-bmenu-this-window] -- select this bookmark in place of the bookmark menu buffer.
 \\[bookmark-bmenu-other-window] -- select this bookmark in another window,
   so the bookmark menu bookmark remains visible in its window.
+\\[bookmark-bmenu-other-frame] -- select this bookmark in another frame.
 \\[bookmark-bmenu-switch-other-window] -- switch the other window to this bookmark.
 \\[bookmark-bmenu-rename] -- rename this bookmark (prompts for new name).
 \\[bookmark-bmenu-relocate] -- relocate this bookmark's file (prompts for new file).
@@ -1979,6 +1981,13 @@ With a prefix arg, prompts for a file to save them in."
     (bookmark--jump-via bookmark 'switch-to-buffer-other-window)))
 
 
+(defun bookmark-bmenu-other-frame ()
+  "Select this line's bookmark in other frame."
+  (interactive)
+  (let  ((bookmark (bookmark-bmenu-bookmark))
+         (pop-up-frames t))
+    (bookmark-jump-other-window bookmark)))
+
 (defun bookmark-bmenu-switch-other-window ()
   "Make the other window select this line's bookmark.
 The current window remains selected."
-- 
2.19.0

Re: [PATCH] open bookmark in other frame

[Karl Fogel]

Pierre-Yves Luyten <py@luyten.fr> writes:
>On 10/12/18 12:04 AM, Drew Adams wrote
>>>
>>> So here is the new version of the patch
>>> 1. Use pop-up-frames variable to avoid a read-only mode on new frame.
>>>      I also had to use (other-frame 1) to ensure new frame is raised.
>>
>> 1. `pop-up-frames' has nothing to do with read-only. 
>
>Sure, i meant i now use pop-up-frames instead of
>view-buffer-other-frame, which was the issue.
>
>> If you use `bookmark-jump-other-window' instead of `bookmark-jump' then you don't need to also use `(other-frame 1)'. See the code I sent.
>
>Great, so i attach a new version of the patch that get rid of
>(other-frame 1) and still works. Thanks for the help.
>
>
>> 2. I don't think you need to include this in the doc string:
>>
>>      , so the bookmark menu bookmark remains visible in its window.
>
>I agree this might be too obvious. So i also removed useless verbiage
>in this version.

Thanks, Pierre-Yves.  Would you mind combining this into one patch, since this is conceptually one change, and using the commit message guidelines as given in CONTRIBUTE (see the section "** Commit messages")?  It would be easier to review that way, and would provide a cleaner audit trail from mailing list post through to commit.

Best regards,
-Karl

>>From 9c3f6774413f9c9316eceafde98f1829e5c06dbd Mon Sep 17 00:00:00 2001
>From: Pierre-Yves Luyten <py@luyten.fr>
>Date: Fri, 12 Oct 2018 21:32:45 +0200
>Subject: [PATCH 1/2] * lisp/bookmark.el (bookmark-jump-other-frame): new
> function
>
> Add bookmark-jump-other-frame
> Bind to bookmark-map
>---
> lisp/bookmark.el | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
>diff --git a/lisp/bookmark.el b/lisp/bookmark.el
>index 58a279473d..9d55c4aada 100644
>--- a/lisp/bookmark.el
>+++ b/lisp/bookmark.el
>@@ -209,6 +209,7 @@ A non-nil value may result in truncated bookmark names."
>     (define-key map "j" 'bookmark-jump)
>     (define-key map "g" 'bookmark-jump) ;"g"o
>     (define-key map "o" 'bookmark-jump-other-window)
>+    (define-key map "5" 'bookmark-jump-other-frame)
>     (define-key map "i" 'bookmark-insert)
>     (define-key map "e" 'edit-bookmarks)
>     (define-key map "f" 'bookmark-insert-location) ;"f"ind
>@@ -1124,6 +1125,13 @@ DISPLAY-FUNC would be `switch-to-buffer-other-window'."
>                                    bookmark-current-bookmark)))
>   (bookmark-jump bookmark 'switch-to-buffer-other-window))
> 
>+(defun bookmark-jump-other-frame (bookmark)
>+  "Jump to BOOKMARK in another frame.  See `bookmark-jump' for more."
>+  (interactive
>+   (list (bookmark-completing-read "Jump to bookmark (in another frame)"
>+                                   bookmark-current-bookmark)))
>+  (let ((pop-up-frames t))
>+    (bookmark-jump-other-window bookmark)))
> 
> (defun bookmark-jump-noselect (bookmark)
>   "Return the location pointed to by BOOKMARK (see `bookmark-jump').
>-- 
>2.19.0
>
>
>>From 31ede454672c727c079b5576053e52639ba94fcc Mon Sep 17 00:00:00 2001
>From: Pierre-Yves Luyten <py@luyten.fr>
>Date: Fri, 12 Oct 2018 21:35:45 +0200
>Subject: [PATCH 2/2] * lisp/bookmark.el (bookmark-bmenu-other-frame):new
> function
>
> Add bookmark-bmenu-other-frame
> Bind to bookmark-bmenu-mode-map
>---
> lisp/bookmark.el | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>diff --git a/lisp/bookmark.el b/lisp/bookmark.el
>index 9d55c4aada..7f73c22267 100644
>--- a/lisp/bookmark.el
>+++ b/lisp/bookmark.el
>@@ -1569,6 +1569,7 @@ unique numeric suffixes \"<2>\", \"<3>\", etc."
>     (set-keymap-parent map special-mode-map)
>     (define-key map "v" 'bookmark-bmenu-select)
>     (define-key map "w" 'bookmark-bmenu-locate)
>+    (define-key map "5" 'bookmark-bmenu-other-frame)
>     (define-key map "2" 'bookmark-bmenu-2-window)
>     (define-key map "1" 'bookmark-bmenu-1-window)
>     (define-key map "j" 'bookmark-bmenu-this-window)
>@@ -1710,6 +1711,7 @@ Bookmark names preceded by a \"*\" have annotations.
> \\[bookmark-bmenu-this-window] -- select this bookmark in place of the bookmark menu buffer.
> \\[bookmark-bmenu-other-window] -- select this bookmark in another window,
>   so the bookmark menu bookmark remains visible in its window.
>+\\[bookmark-bmenu-other-frame] -- select this bookmark in another frame.
> \\[bookmark-bmenu-switch-other-window] -- switch the other window to this bookmark.
> \\[bookmark-bmenu-rename] -- rename this bookmark (prompts for new name).
> \\[bookmark-bmenu-relocate] -- relocate this bookmark's file (prompts for new file).
>@@ -1979,6 +1981,13 @@ With a prefix arg, prompts for a file to save them in."
>     (bookmark--jump-via bookmark 'switch-to-buffer-other-window)))
> 
> 
>+(defun bookmark-bmenu-other-frame ()
>+  "Select this line's bookmark in other frame."
>+  (interactive)
>+  (let  ((bookmark (bookmark-bmenu-bookmark))
>+         (pop-up-frames t))
>+    (bookmark-jump-other-window bookmark)))
>+
> (defun bookmark-bmenu-switch-other-window ()
>   "Make the other window select this line's bookmark.
> The current window remains selected."

Re: [PATCH] open bookmark in other frame

[Pierre-Yves Luyten]

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

On 10/12/18 11:23 PM, Karl Fogel wrote:
> Would you mind combining this into one patch, since this is conceptually one change, and using the commit message guidelines as given in CONTRIBUTE (see the section "** Commit messages")?

Hello, here is the change in one single patch, and a few changes to 
commit message.

Regards
Pierre-Yves

[-- Attachment #2: 0001-lisp-bookmark.el-add-functions-to-open-in-other-fram.patch --]
[-- Type: text/x-patch, Size: 3241 bytes --]

From f9a85bbe25370d663b881c6dcf055182a2b6d9bc Mon Sep 17 00:00:00 2001
From: Pierre-Yves Luyten <py@luyten.fr>
Date: Sat, 13 Oct 2018 22:06:41 +0200
Subject: [PATCH] * lisp/bookmark.el: add functions to open in other frame

(bookmark-jump-other-frame): New function.
Bind in bookmark-map.
(bookmark-bmenu-other-frame): New function.
Bind in bookmark-bmenu-mode-map.
---
 lisp/bookmark.el | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 58a279473d..7f73c22267 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -209,6 +209,7 @@ A non-nil value may result in truncated bookmark names."
     (define-key map "j" 'bookmark-jump)
     (define-key map "g" 'bookmark-jump) ;"g"o
     (define-key map "o" 'bookmark-jump-other-window)
+    (define-key map "5" 'bookmark-jump-other-frame)
     (define-key map "i" 'bookmark-insert)
     (define-key map "e" 'edit-bookmarks)
     (define-key map "f" 'bookmark-insert-location) ;"f"ind
@@ -1124,6 +1125,13 @@ DISPLAY-FUNC would be `switch-to-buffer-other-window'."
                                    bookmark-current-bookmark)))
   (bookmark-jump bookmark 'switch-to-buffer-other-window))
 
+(defun bookmark-jump-other-frame (bookmark)
+  "Jump to BOOKMARK in another frame.  See `bookmark-jump' for more."
+  (interactive
+   (list (bookmark-completing-read "Jump to bookmark (in another frame)"
+                                   bookmark-current-bookmark)))
+  (let ((pop-up-frames t))
+    (bookmark-jump-other-window bookmark)))
 
 (defun bookmark-jump-noselect (bookmark)
   "Return the location pointed to by BOOKMARK (see `bookmark-jump').
@@ -1561,6 +1569,7 @@ unique numeric suffixes \"<2>\", \"<3>\", etc."
     (set-keymap-parent map special-mode-map)
     (define-key map "v" 'bookmark-bmenu-select)
     (define-key map "w" 'bookmark-bmenu-locate)
+    (define-key map "5" 'bookmark-bmenu-other-frame)
     (define-key map "2" 'bookmark-bmenu-2-window)
     (define-key map "1" 'bookmark-bmenu-1-window)
     (define-key map "j" 'bookmark-bmenu-this-window)
@@ -1702,6 +1711,7 @@ Bookmark names preceded by a \"*\" have annotations.
 \\[bookmark-bmenu-this-window] -- select this bookmark in place of the bookmark menu buffer.
 \\[bookmark-bmenu-other-window] -- select this bookmark in another window,
   so the bookmark menu bookmark remains visible in its window.
+\\[bookmark-bmenu-other-frame] -- select this bookmark in another frame.
 \\[bookmark-bmenu-switch-other-window] -- switch the other window to this bookmark.
 \\[bookmark-bmenu-rename] -- rename this bookmark (prompts for new name).
 \\[bookmark-bmenu-relocate] -- relocate this bookmark's file (prompts for new file).
@@ -1971,6 +1981,13 @@ With a prefix arg, prompts for a file to save them in."
     (bookmark--jump-via bookmark 'switch-to-buffer-other-window)))
 
 
+(defun bookmark-bmenu-other-frame ()
+  "Select this line's bookmark in other frame."
+  (interactive)
+  (let  ((bookmark (bookmark-bmenu-bookmark))
+         (pop-up-frames t))
+    (bookmark-jump-other-window bookmark)))
+
 (defun bookmark-bmenu-switch-other-window ()
   "Make the other window select this line's bookmark.
 The current window remains selected."
-- 
2.19.0

Re: [PATCH] open bookmark in other frame

[Karl Fogel]

Pierre-Yves Luyten <py@luyten.fr> writes:
>On 10/12/18 11:23 PM, Karl Fogel wrote:
>> Would you mind combining this into one patch, since this is conceptually one change, and using the commit message guidelines as given in CONTRIBUTE (see the section "** Commit messages")?
>
>Hello, here is the change in one single patch, and a few changes to
>commit message.

Thanks, Pierre.  I'll review & test this as soon as possible!

Did you produce your patch using 'git format-patch'?  We can apply it in either case, but if you produce it that way (and then I use 'git am' to apply), I think that will preserve your commit info and authorship info in the most accurate way possible.  The CONTRIBUTE file covers this.

Best regards,
-Karl

Re: [PATCH] open bookmark in other frame

[Pierre-Yves Luyten]

Le 2018-10-16 04:09, Karl Fogel a écrit :
> 
> Did you produce your patch using 'git format-patch'?

Yes. I just do not remember if i directly invoked git in command line or 
using magit.

Regards
Pierre-Yves

Re: [PATCH] open bookmark in other frame

[Karl Fogel]

I wrote:
>Thanks, Pierre.  I'll review & test this as soon as possible!

Oops, my apologies -- meant that to be "Thanks, Pierre-Yves."

Best regards,
-Karl

Re: [PATCH] open bookmark in other frame

[Karl Fogel]

Pierre-Yves Luyten <py@luyten.fr> writes:
>Hello, here is the change in one single patch, and a few changes to
>commit message.

Hi, Pierre-Yves.        

It's always a good idea to test changes in a freshly-built and freshly-started Emacs.  In this case, because there was no ";;;###autoload" marker before `bookmark-jump-other-frame', invoking that function in a fresh Emacs would generate an error.  For example, for me `bookmark-map' is bound to `C-c B', so when I did `C-c B 5' (in a new Emacs) I got an error.

The fix is simple -- this change on top of your patch:

  --- lisp/bookmark.el
  +++ lisp/bookmark.el
  @@ -1125,6 +1125,7 @@ bookmark-jump-other-window
                                      bookmark-current-bookmark)))
     (bookmark-jump bookmark 'switch-to-buffer-other-window))
   
  +;;;###autoload
   (defun bookmark-jump-other-frame (bookmark)
     "Jump to BOOKMARK in another frame.  See `bookmark-jump' for more."
     (interactive

Obviously, I could just add that ";;;###autoload" line myself commit your patch.  But I thought you might want a chance to update the patch and re-post it, so that you'd have the experience of trying the change before and after.  Either way is fine with me; let me know what you'd like to do.

(If you're not familiar with what the autoload system does, see section 16.5 "Autoload" in the Info pages.)

Best regards,
-Karl

Re: [PATCH] open bookmark in other frame

[Pierre-Yves Luyten]

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

On 02/11/2018 19:55, Karl Fogel wrote:
> 
> It's always a good idea to test changes in a freshly-built and freshly-started Emacs.  In this case, because there was no ";;;###autoload" marker before `bookmark-jump-other-frame', invoking that function in a fresh Emacs would generate an error.
Hi,

Oops i used to just work with eval-buffer on the lisp i needed, and i 
can see now this is not the right workflow for testing a patch.

So now i rather build Emacs to test patch properly, and obviously as you 
wrote adding the autoload magic comment fixed the issue. So here is the 
patch with this.

Regards
Pierre-Yves

[-- Attachment #2: 0001-lisp-bookmark.el-add-functions-to-open-in-other-fram.patch --]
[-- Type: text/x-patch, Size: 3100 bytes --]

From 0f2d53a9646db6e054b73e7c97dfbfa9f7c05e0e Mon Sep 17 00:00:00 2001
From: Pierre-Yves Luyten <py@luyten.fr>
Date: Sat, 13 Oct 2018 22:06:41 +0200
Subject: [PATCH] * lisp/bookmark.el: add functions to open in other frame

(bookmark-jump-other-frame): New function.
Bind in bookmark-map.
(bookmark-bmenu-other-frame): New function.
Bind in bookmark-bmenu-mode-map.
---
 lisp/bookmark.el | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 58a279473d..15a841e208 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -209,6 +209,7 @@ bookmark-map
     (define-key map "j" 'bookmark-jump)
     (define-key map "g" 'bookmark-jump) ;"g"o
     (define-key map "o" 'bookmark-jump-other-window)
+    (define-key map "5" 'bookmark-jump-other-frame)
     (define-key map "i" 'bookmark-insert)
     (define-key map "e" 'edit-bookmarks)
     (define-key map "f" 'bookmark-insert-location) ;"f"ind
@@ -1124,6 +1125,14 @@ bookmark-jump-other-window
                                    bookmark-current-bookmark)))
   (bookmark-jump bookmark 'switch-to-buffer-other-window))
 
+;;;###autoload
+(defun bookmark-jump-other-frame (bookmark)
+  "Jump to BOOKMARK in another frame.  See `bookmark-jump' for more."
+  (interactive
+   (list (bookmark-completing-read "Jump to bookmark (in another frame)"
+                                   bookmark-current-bookmark)))
+  (let ((pop-up-frames t))
+    (bookmark-jump-other-window bookmark)))
 
 (defun bookmark-jump-noselect (bookmark)
   "Return the location pointed to by BOOKMARK (see `bookmark-jump').
@@ -1561,6 +1570,7 @@ bookmark-bmenu-mode-map
     (set-keymap-parent map special-mode-map)
     (define-key map "v" 'bookmark-bmenu-select)
     (define-key map "w" 'bookmark-bmenu-locate)
+    (define-key map "5" 'bookmark-bmenu-other-frame)
     (define-key map "2" 'bookmark-bmenu-2-window)
     (define-key map "1" 'bookmark-bmenu-1-window)
     (define-key map "j" 'bookmark-bmenu-this-window)
@@ -1702,6 +1712,7 @@ bookmark-bmenu-mode
 \\[bookmark-bmenu-this-window] -- select this bookmark in place of the bookmark menu buffer.
 \\[bookmark-bmenu-other-window] -- select this bookmark in another window,
   so the bookmark menu bookmark remains visible in its window.
+\\[bookmark-bmenu-other-frame] -- select this bookmark in another frame.
 \\[bookmark-bmenu-switch-other-window] -- switch the other window to this bookmark.
 \\[bookmark-bmenu-rename] -- rename this bookmark (prompts for new name).
 \\[bookmark-bmenu-relocate] -- relocate this bookmark's file (prompts for new file).
@@ -1971,6 +1982,13 @@ bookmark-bmenu-other-window
     (bookmark--jump-via bookmark 'switch-to-buffer-other-window)))
 
 
+(defun bookmark-bmenu-other-frame ()
+  "Select this line's bookmark in other frame."
+  (interactive)
+  (let  ((bookmark (bookmark-bmenu-bookmark))
+         (pop-up-frames t))
+    (bookmark-jump-other-window bookmark)))
+
 (defun bookmark-bmenu-switch-other-window ()
   "Make the other window select this line's bookmark.
 The current window remains selected."
-- 
2.19.1

Re: [PATCH] open bookmark in other frame

[Karl Fogel]

Pierre-Yves Luyten <py@luyten.fr> writes:
>Oops i used to just work with eval-buffer on the lisp i needed, and i
>can see now this is not the right workflow for testing a patch.
>
>So now i rather build Emacs to test patch properly, and obviously as
>you wrote adding the autoload magic comment fixed the issue. So here
>is the patch with this.

Thanks, Pierre-Yves.  I've applied the patch and committed it now.  As it's a new feature, not a bug fix, it went to the master branch, so it'll be in the next major release of Emacs.

I modified the log message a bit.  I changed it to match the style recommended in CONTRIBUTING (that is, to put a summary line at the top before giving a ChangeLog-style entry below that), and I added a line at the bottom noting that I had applied the patch, because otherwise there would be no way to trace the full audit trail.

By the way, I realize that when you wrote the log message you were probably just following the examples you saw from looking at existing log messages.  Some developers are still using an older log message format, but the CONTRIBUTING file is clear about the modern format, so I though this change should provide an up-to-date example for whoever the next contributor is :-).

Here's the commit:

  | commit f3345dee4b40293547d10963c6cb242a62e424ba
  | Author: Pierre-Yves Luyten <py@luyten.fr>
  | Date:   Sat Oct 13 22:06:41 2018 +0200
  | 
  |     Add functions to open a bookmark in another frame
  |     
  |     * lisp/bookmark.el (bookmark-jump-other-frame): New function.
  |     Bind in bookmark-map.
  |     (bookmark-bmenu-other-frame): New function.
  |     Bind in bookmark-bmenu-mode-map.
  |     
  |     Patch applied by Karl Fogel.

Thank you for the improvement to Emacs!

Best regards,
-Karl

Re: [PATCH] open bookmark in other frame

[Stephen Leake]

Pierre-Yves Luyten <py@luyten.fr> writes:

> Hello,
>
> people sometimes use several frames, so i would like to make opening
> bookmark one shot. Attached patch adds two funcs,
> "bookmark-jump-other-frame" and the equivalent from the bookmarks
> menu, "bookmark-bmenu-other-frame"

The prefered solution to "open <x> in other frame" is to use the Gnu
ELPA package "other-frame-window".

-- 
-- Stephe