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 (counsel-mark-ring): Improve #1328

Closed
wants to merge 1 commit into from

Conversation

basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Nov 29, 2017

Changelog

(counsel--pad): Remove unnecessary defun.
(counsel-mark-ring): Support narrowing and ivy-immediate-done.
Fix line number formatting and mark-ring uniquifying and sorting.
Remove redundant call to Emacs 25.1 macro save-mark-and-excursion.
(counsel-mode-map): Replace pop-mark with pop-to-mark-command.

Question before merging

pop-to-mark-command has no default global binding. This means that the user has to explicitly bind a global pop-to-mark-command key in order for the rebinding in counsel-mode-map to counsel-mark-ring to work. Isn't overriding an explicit user key binding The Wrong Thing? Even if deemed acceptable, is there any point in rebinding an unbound command?

(counsel--pad): Remove unnecessary defun.
(counsel-mark-ring): Support narrowing and ivy-immediate-done.
Fix line number formatting and mark-ring uniquifying and sorting.
Remove redundant call to Emacs 25.1 macro save-mark-and-excursion.
(counsel-mode-map): Replace pop-mark with pop-to-mark-command.
@abo-abo
Copy link
Owner

abo-abo commented Nov 29, 2017

Thanks.

Isn't overriding an explicit user key binding The Wrong Thing?

I think this was specifically requested by a user as a feature, i.e. enable counsel-mode and have it do all the rebinds possible.

@basil-conto
Copy link
Collaborator Author

I think this was specifically requested by a user as a feature, i.e. enable counsel-mode and have it do all the rebinds possible.

Fair enough, thanks.

@basil-conto basil-conto deleted the counsel-mark-ring branch November 29, 2017 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants