[PATCH] open bookmark in other frame

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] open bookmark in other frame

Pierre-Yves Luyten
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

0001-lisp-bookmark.el-other-frame.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Marcin Borkowski-3

On 2018-10-10, at 22:14, Pierre-Yves Luyten <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Karl Fogel-2
Marcin Borkowski <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

RE: [PATCH] open bookmark in other frame

Drew Adams
In reply to this post by Pierre-Yves Luyten
> 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.)

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Karl Fogel-2
Drew Adams <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Pierre-Yves Luyten
Le 2018-10-11 09:19, Karl Fogel a écrit :

> Drew Adams <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Pierre-Yves Luyten
In reply to this post by Karl Fogel-2
Le 2018-10-10 23:35, Karl Fogel a écrit :
> Marcin Borkowski <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

RE: [PATCH] open bookmark in other frame

Drew Adams
In reply to this post by Karl Fogel-2
> > 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.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Pierre-Yves Luyten
In reply to this post by Drew Adams
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

0001-lisp-bookmark.el-bookmark-jump-other-frame-new-funct.patch (1K) Download Attachment
0002-lisp-bookmark.el-bookmark-bmenu-other-frame-new-func.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Pierre-Yves Luyten
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

0001-lisp-bookmark.el-bookmark-jump-other-frame-new-funct.patch (1K) Download Attachment
0002-lisp-bookmark.el-bookmark-bmenu-other-frame-new-func.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Karl Fogel-2
Pierre-Yves Luyten <[hidden email]> 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 <[hidden email]>
>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 <[hidden email]>
>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."

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Stephen Leake-3
In reply to this post by Pierre-Yves Luyten
Pierre-Yves Luyten <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Pierre-Yves Luyten
In reply to this post by Karl Fogel-2
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

0001-lisp-bookmark.el-add-functions-to-open-in-other-fram.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Karl Fogel-2
Pierre-Yves Luyten <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Karl Fogel-2
In reply to this post by Pierre-Yves Luyten
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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] open bookmark in other frame

Pierre-Yves Luyten
In reply to this post by Karl Fogel-2
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