Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

counsel.el: Add other window for Bookmarks #2902

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

Hugo-Heagren
Copy link
Contributor

@Hugo-Heagren Hugo-Heagren commented Aug 1, 2021

Add actions other window and other frame to counsel-bookmark, in line with similar actions for counsel-find-file and ivy-switch-buffer.

This is just for consistency with the other interfaces. I was looking for these actions in the bookmarks menu and couldn't find them, so here they are.

@Hugo-Heagren Hugo-Heagren changed the title counsel.el: Add other window' and other frame' for Bookmarks counsel.el: Add other window and other frame for Bookmarks Aug 1, 2021
Copy link
Collaborator

@basil-conto basil-conto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

counsel.el Outdated
@@ -2490,7 +2490,9 @@ By default `counsel-bookmark' opens a dired buffer for directories."

(ivy-set-actions
'counsel-bookmark
`(("d" bookmark-delete "delete")
`(("j" bookmark-jump-other-window "other window")
("f" bookmark-jump-other-frame "other frame")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor issues with the bookmark-jump-other-frame action:

  1. The command bookmark-jump-other-frame is new in Emacs 27, but Counsel tries to support Emacs versions 24.5 and later equally, if reasonably possible.
  2. Although f is indeed bound to an "other frame" action most of the time, for some commands, including crucially the related counsel-bookmarked-directory, it's unfortunately bound to a "find file" action instead.

So maybe we should add only the bookmark-jump-other-window action for now, to be conservative? WDYT?

Copy link
Contributor Author

@Hugo-Heagren Hugo-Heagren Aug 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command bookmark-jump-other-frame is new in Emacs 27, but Counsel tries to support Emacs versions 24.5 and later equally, if reasonably possible.

Well in theory it shouldn't be too difficult to just check if the function is defined and only insert the action if it is right? I'm not massively experienced in elisp, but I know it's not too difficult to check if a function is bound or not. If that was in the call to ivy-set-actions itself, then the check wouldn't slow down the actual execution of counsel-bookmark, so there wouldn't be any (significant) performance loss from the check.

Although f is indeed bound to an "other frame" action most of the time, for some commands, including crucially the related counsel-bookmarked-directory, it's unfortunately bound to a "find file" action instead.

So maybe we should add only the bookmark-jump-other-window action for now, to be conservative? WDYT?

Ah ok, so there's an issue even with the above. Personally I only ever work with one frame, so this would be fine for me. I don't know how much other people use these commands, but clearly I (as someone who doesn't even use frames) am the first person to write this feature, so it can't be high on everyone's agendas. I think only adding other-window is probably fine, I'll update.

Copy link
Collaborator

@basil-conto basil-conto Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well in theory it shouldn't be too difficult to just check if the function is defined and only insert the action if it is right?

Right, but it's not the most common in Emacs for key bindings to be conditional on the existence of a function (although there are examples of it, even within Ivy). More common would be to define a compatibility alias/shim that delegates to bookmark-jump-other-frame if it exists, and otherwise emulates (read: copies) its behaviour. That way the key always invokes the desired behaviour. I don't know whether that's worth the trouble in this case.

If that was in the call to ivy-set-actions itself, then the check wouldn't slow down the actual execution of counsel-bookmark, so there wouldn't be any (significant) performance loss from the check.

None of this is performance-critical, and testing whether a function exists is very cheap; there's no need to worry about performance here.

I don't know how much other people use these commands

FWIW, I'm on a tiling WM so I have pop-up-frames non-nil, which means each "other window" is usually displayed as an "other frame" anyway. IOW, bookmark-jump-other-window and bookmark-jump-other-frame do the same thing on my end.

I think only adding other-window is probably fine, I'll update.

Thanks.

@Hugo-Heagren Hugo-Heagren force-pushed the bookmark-other-actions branch from 2ebc8ee to 9b59393 Compare August 1, 2021 21:21
@Hugo-Heagren Hugo-Heagren changed the title counsel.el: Add other window and other frame for Bookmarks counsel.el: Add other window for Bookmarks Aug 2, 2021
* counsel.el (counsel-bookmark): Add "other window" action
bookmark-jump-other-window, in line with similar actions for
counsel-find-file and ivy-switch-buffer (PR abo-abo#2902).

Copyright-paperwork-exempt: yes
@basil-conto basil-conto force-pushed the bookmark-other-actions branch from 9b59393 to bde2176 Compare August 2, 2021 09:35
@basil-conto basil-conto merged commit bde2176 into abo-abo:master Aug 2, 2021
@basil-conto
Copy link
Collaborator

Thanks again! This should become available on GNU-devel ELPA within the next day: https://elpa.gnu.org/devel/counsel.html

@Hugo-Heagren Hugo-Heagren deleted the bookmark-other-actions branch August 2, 2021 11:37
@Hugo-Heagren Hugo-Heagren restored the bookmark-other-actions branch August 2, 2021 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants