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-isearch-backward #2125

Closed
wants to merge 2 commits into from

Conversation

@andschwa
Copy link
Contributor

@andschwa andschwa commented Jul 8, 2019

This adds a swiper-isearch-backward implementation for #1172. Its behavior doesn't match isearch-backward-regexp perfectly, but it matches that of swiper-isearch. The fix for that discrepancy should be separate from this, and then fixed for both.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 9, 2019

Thanks for the PR. Looks like there's still a bit of work left. Let me know if it's ready for review.

@andschwa andschwa force-pushed the swiper-isearch-backward branch from 3004ac7 to d5e8714 Jul 10, 2019
@andschwa andschwa changed the title Swiper isearch backward Add swiper-isearch-backward Jul 10, 2019
@andschwa
Copy link
Contributor Author

@andschwa andschwa commented Jul 10, 2019

@abo-abo I took a different approach the second time around, actually reversing the logic of the search. I am not entirely happy with some of the code duplication, but I am happy with the behaviors. I think there is some refactoring to be done, but separately from this implementation.

Also, does my note in the tests make sense to you?

@andschwa andschwa force-pushed the swiper-isearch-backward branch from d5e8714 to 1d82836 Jul 12, 2019
@andschwa
Copy link
Contributor Author

@andschwa andschwa commented Jul 12, 2019

Cleaned up some of the implementation (reduced the duplication).

@andschwa
Copy link
Contributor Author

@andschwa andschwa commented Jul 13, 2019

@abo-abo Would you prefer turning this into the pair swiper-isearch-forward and swiper-isearch-backward? I kind of have an implementation of that as a work-in-progress though I gotta figure out a bug (probably due to my time away from Lisp, I keep trying to curry functions like in OCaml 😆).

@andschwa
Copy link
Contributor Author

@andschwa andschwa commented Jul 13, 2019

Oh yeah 😂 I can get my FSF copyright assignment done ASAP...like Monday I think.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 15, 2019

Thanks. The code looks OK. Let me know what the CA goes through.

andschwa added 2 commits Jul 16, 2019
- Sets the first candidate before the point instead of after the point
- Meant to be used on `C-r` like `swiper-isearch` on `C-s`
- This searches in reverse in comparison to `swiper-isearch` in order to
  handle the placement of the point properly.
- Note explains the discrepancy in behaviors.
@andschwa andschwa force-pushed the swiper-isearch-backward branch from 1d82836 to 22d391d Jul 16, 2019
@andschwa
Copy link
Contributor Author

@andschwa andschwa commented Jul 16, 2019

CA is in the mail. Forgot it had to be snail-mailed 😅

@abo-abo abo-abo closed this in f5508e5 Jul 17, 2019
abo-abo added a commit that referenced this issue Jul 17, 2019
* ivy-test.el (swiper-isearch-backward-backspace): Add test.

Fixes #2125
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jul 17, 2019

Thanks. I rewrote / simplified your code in the first commit (I didn't want to introduce more functions than necessary).

Then in the second commit I've added a backspace behavior that matches isearch-backward-regexp and a test for it. Please review and test.

@andschwa
Copy link
Contributor Author

@andschwa andschwa commented Jul 18, 2019

Thank you @abo-abo! I like your simplifications, good way to go about it.

@andschwa andschwa deleted the swiper-isearch-backward branch Jul 19, 2019
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
- Sets the first candidate before the point instead of after the point
- Meant to be used on `C-r` like `swiper-isearch` on `C-s`
- This searches in reverse in comparison to `swiper-isearch` in order to
  handle the placement of the point properly.

ivy-test.el (swiper-isearch-backward): Add new tests

- Note explains the discrepancy in behaviors.

Fixes abo-abo#2125
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
* ivy-test.el (swiper-isearch-backward-backspace): Add test.

Fixes abo-abo#2125
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

2 participants