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

Fix insert action for swiper-isearch #2963

Merged

Conversation

dustinpaluch
Copy link
Contributor

@dustinpaluch dustinpaluch commented Mar 23, 2022

swiper-isearch passes a buffer position to ivy--action-insert. The existing insert action throws an error. #2929
This change assumes numbers passed to ivy--action-insert are valid buffer positions in the current buffer, and converts them to a string containing the line that buffer position is on, allowing insert to do the right thing.

make compile - check for new compilation warnings ✅ 
make deps - install dependencies for testing ✅ 
make test - check for failing tests ✅ 
make checkdoc - check documentation guidelines ✅ 

@basil-conto basil-conto added the bug label Apr 3, 2022
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! This continues cleaning up the sad state of regressions between swiper and swiper-isearch that I also tried improving in #2913 and #2914.

This change assumes numbers passed to ivy--action-insert are valid buffer positions in the current buffer, and converts them to a string containing the line that buffer position is on, allowing insert to do the right thing.

Ivy is meant to be a low-level generic completion framework, and Swiper simply a client thereof. I think it's therefore better if Ivy remain as unaware of Swiper implementation details as possible, and this issue be fixed on the Swiper side instead.

That calls for a swiper-isearch override of the i (insert) action. Have a look at #2914 to see how I did this for the o (default) and w (copy) actions; you may be able to reuse some of the functions there. Thanks.

@basil-conto basil-conto linked an issue Apr 3, 2022 that may be closed by this pull request
@dustinpaluch
Copy link
Contributor Author

dustinpaluch commented Apr 6, 2022

Looking at the implementation of swiper-isearch-action-copy, it looks like it only copies the portion of the line that is the exact match of your swiper-isearch input, which makes sense.

So it would make sense for an insert action to do the same, but that's not really the feature I was trying to fix/implement.

My use case is that after finding a line via swiper-isearch, I want to insert that entire line where I started the search from. "You should use regular swiper then"... yes. But I use swiper-isearch as my default search function because I want both the minibuffer candidate list for browsing results as well as the ability to C-s to other matches on the same line. And I don't want to have to think about whether I want to do a full line insert before invoking either swiper or swiper-isearch. Nor do I have the keybinding real-estate to have both functions on easily invokable/high-traffic keys.

So I guess this is just an idiosyncrasy of my workflow, and I should just add my own custom action to swiper-isearch in my own config.

@dustinpaluch dustinpaluch force-pushed the fix/swiper-isearch-ivy-action-insert branch from adc9a65 to a5f3bc0 Compare April 7, 2022 17:50
@dustinpaluch
Copy link
Contributor Author

dustinpaluch commented Apr 7, 2022

@basil-conto , thanks for the feedback! Here's a new attempt mirroring your copy action, staying consistent with only inserting the match-string instead of the entire line which my first attempt did.

@abroekhof
Copy link

How's this look to get checked in? Would be nice to have this functionality!

@buhtz
Copy link

buhtz commented Jan 9, 2023

I assume my problem (#3006) depends on the problem that RP try to fix. Would be great if this could be merged.

* swiper.el (swiper-isearch-action-insert): New function modeled
after swiper-isearch-action-copy (abo-abo#2913, abo-abo#2914).
(swiper-isearch): Override the default 'insert' action with it.

Fixes abo-abo#2929.

Copyright-paperwork-exempt: yes
@basil-conto basil-conto force-pushed the fix/swiper-isearch-ivy-action-insert branch from a5f3bc0 to 72977e2 Compare March 26, 2023 12:48
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!

@basil-conto basil-conto merged commit 72977e2 into abo-abo:master Mar 26, 2023
basil-conto added a commit that referenced this pull request Mar 26, 2023
This takes into account swiper-goto-start-of-match and
swiper-isearch-backward, under which point precedes and does not
follow the candidate (#2913, #2914, #2929, #2963).

* swiper.el (swiper--isearch-candidate-string): New helper function.
(swiper-isearch-action-copy, swiper-isearch-action-insert): Use it.
@basil-conto
Copy link
Collaborator

Sorry for the delay in merging this.

Please have a look at my followup change for supporting swiper-goto-start-of-match and swiper-isearch-backward, and let me know if anything is amiss: b69f78f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ivy--action-insert throws wrong-type-argument via swiper-isearch
4 participants