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 swiper-region and swiper-isearch-region #2027

Closed
wants to merge 4 commits into from
Closed

Conversation

@masasam
Copy link
Contributor

@masasam masasam commented Apr 15, 2019

I think this is a useful function in the swiper so please import it if you like.

swiper-isearch-region
If region is selected, swiper-isearch with the keyword selected in region.
If the region isn't selected, swiper-isearch.

swiper-region
If region is selected, swiper with the keyword selected in region.
If the region isn't selected, swiper.

swiper.el Outdated
"If region is selected, `swiper' with the keyword selected in region.
If the region isn't selected, `swiper'."
(interactive)
(if (region-active-p)
Copy link
Collaborator

@basil-conto basil-conto Apr 16, 2019

Region-aware commands should usually use use-region-p instead.

Copy link
Contributor Author

@masasam masasam Apr 18, 2019

@basil-conto Thank you for review.
Fix to use-region-p.

swiper.el Outdated
If the region isn't selected, `swiper'."
(interactive)
(if (region-active-p)
(progn (setq mark-active nil)
Copy link
Collaborator

@basil-conto basil-conto Apr 16, 2019

You should usually call the function deactivate-mark or set the variable deactivate-mark instead of manipulating mark-active directly; see (elisp) The Mark for details.

Copy link
Contributor Author

@masasam masasam Apr 18, 2019

Fix to deactivate-mark.

swiper.el Outdated
(interactive)
(if (region-active-p)
(progn (setq mark-active nil)
(swiper (buffer-substring
Copy link
Collaborator

@basil-conto basil-conto Apr 16, 2019

Should this be buffer-substring-no-properties instead?

Copy link
Contributor Author

@masasam masasam Apr 18, 2019

Fix to buffer-substring-no-properties.

swiper.el Outdated
(progn (setq mark-active nil)
(swiper (buffer-substring
(region-beginning) (region-end))))
(swiper)))
Copy link
Collaborator

@basil-conto basil-conto Apr 16, 2019

Tip: if you invert the if condition, there is no need for the progn:

(if (not ...)
    (swiper)
  ...)

Copy link
Contributor Author

@masasam masasam Apr 18, 2019

Fix to no progn.

swiper.el Outdated
@@ -503,6 +503,17 @@ When non-nil, INITIAL-INPUT is the initial search pattern."
(interactive)
(swiper--ivy (swiper--candidates) initial-input))

;;;###autoload
(defun swiper-region-or-not ()
Copy link
Collaborator

@basil-conto basil-conto Apr 16, 2019

Why not just call it swiper-region? Or add this logic to swiper itself, based on a new user option?

Copy link
Contributor Author

@masasam masasam Apr 18, 2019

Fix to swiper-region and swiper-isearch-region.

swiper.el Outdated
(interactive)
(if (region-active-p)
(progn (setq mark-active nil)
(swiper (buffer-substring
Copy link
Collaborator

@basil-conto basil-conto Apr 16, 2019

Please follow the project's dir-locals-file settings and indent with spaces, not tabs.

Copy link
Contributor Author

@masasam masasam Apr 18, 2019

Fix indent with speces.

@masasam masasam changed the title Add swiper-region-or-not and swiper-isearch-region-or-not Add swiper-region and swiper-isearch-region Apr 18, 2019
@abo-abo abo-abo closed this in 9e513d0 May 7, 2019
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 7, 2019

Thanks, merged partially. Please review the two referenced commits.

@masasam
Copy link
Contributor Author

@masasam masasam commented May 7, 2019

@abo-abo Thank you for the partial merge!

@masasam masasam deleted the region branch May 7, 2019
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants