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

Add option to choose whether the deleted region should be added to kill ring #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blue0513
Copy link

This package is very useful 🎉
I'll suggest one more feature: related #13 , #12


What's this PR

Add smart-hungry-delete-should-add-kill-ring method to choose whether deleted region should be added to kill ring or not.

Usage

Usage is as follows.

(use-package smart-hungry-delete
  :ensure t
  :init
  (smart-hungry-delete-add-default-hooks)
  (smart-hungry-delete-should-add-kill-ring nil) ;; or `t`

Copy link
Owner

@hrehfeld hrehfeld left a comment

Choose a reason for hiding this comment

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

OK, I suggest the following:

  1. create a new clean branch/PR
  2. use a custom variable smart-hungry-delete-kill-function that holds a function symbol to either kill-region or delete-region with a DOCSTRING like "A function symbol to kill the buffer text that smart-hungry-delete decides to delete or kill.\n\nThe function is called with two buffer positions BEG END as its only parameters and should remove the buffer text between these positions."
  3. just use that function in smart-hungry-delete-char

I think that's much cleaner, do you agree? Or am I missing something?

(Oh, and I'm sorry it takes so long for me to get back to the PRs 😬 )

@@ -61,6 +61,12 @@
:safe t
)

(defcustom smart-hungry-delete-add-kill-ring t
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, I don't want to be annoying, but can we rename this to smart-hungry-delete-kill-ring-save just like kill-ring-save? Or were you inspired by some other existing variable/function name? There's also avy-kill-ring-save-region and org-export-copy-to-kill-ring.

@@ -124,6 +130,14 @@ completely deleted."
(add-hook 'nxml-mode-hook 'smart-hungry-delete-default-sgml-mode-common-hook)
(add-hook 'text-mode-hook 'smart-hungry-delete-default-text-mode-hook))

;;;###autoload
(defun smart-hungry-delete-should-add-kill-ring (should-kill)
Copy link
Owner

Choose a reason for hiding this comment

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

common elisp convention is to postfix predicate functions with -p: e.g. -any-p, ring-p, org-url-p.

How about smart-hungry-delete-kill-ring-save-p?

Copy link
Owner

Choose a reason for hiding this comment

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

Wait, this isn't a predicate. Hmm.

([remap delete-backward-char] . smart-hungry-delete-backward-char)
([remap delete-char] . smart-hungry-delete-forward-char))
:init (smart-hungry-delete-add-default-hooks))
([remap delete-backward-char] . smart-hungry-delete-backward-char)
Copy link
Owner

Choose a reason for hiding this comment

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

awesome!

Copy link
Owner

Choose a reason for hiding this comment

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

Can you provide these in a separate PR?

@@ -124,6 +130,14 @@ completely deleted."
(add-hook 'nxml-mode-hook 'smart-hungry-delete-default-sgml-mode-common-hook)
(add-hook 'text-mode-hook 'smart-hungry-delete-default-text-mode-hook))

;;;###autoload
(defun smart-hungry-delete-should-add-kill-ring (should-kill)
"Choose deleted region should be added to kill ring or not by `SHOULD-KILL`."
Copy link
Owner

Choose a reason for hiding this comment

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

"Should the deleted region be added to the KILL-RING?"

"Choose deleted region should be added to kill ring or not by `SHOULD-KILL`."
(interactive)
(setq smart-hungry-delete-add-kill-ring should-kill)
(put #'smart-hungry-delete-backward-char 'delete-selection (if smart-hungry-delete-add-kill-ring 'kill nil))
Copy link
Owner

Choose a reason for hiding this comment

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

Why are you putting stuff into the fsymbol here? It's never used, is it?

@blue0513
Copy link
Author

Thx for the review!! 🤝
These days, unfortunately I'm a bit busy. So I'll respond later 🙇

@hrehfeld
Copy link
Owner

Sure, I'm happy to review when you find the time, thanks for submitting! <3

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.

2 participants