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

Remove with-ivy-window in actions #3049

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ywwry66
Copy link
Contributor

@ywwry66 ywwry66 commented Jun 5, 2024

The with-ivy-window wrapper became redundant in actions after 55a90c9. Its usage was subsequently replaced by (select-window (ivy--get-window ivy-last)) in 1c09e99. Its obselote status was also confirmed by abo-abo in #1727 (comment).

This PR removes all with-ivy-window usages in actions in swiper.el and counsel.el, which cleans up the code and ensures the fix in 1c09e99 is correctly applied to all actions.

`with-ivy-window` wrapper is legacy for actions.
@ywwry66
Copy link
Contributor Author

ywwry66 commented Jun 5, 2024

This PR only contains repeated removal of with-ivy-window, so I assume it could potentially be treated as legally insignificant?

A regular series of repeated changes, such as renaming a symbol, is not legally significant even if the symbol has to be renamed in many places

@ywwry66
Copy link
Contributor Author

ywwry66 commented Jun 5, 2024

One thing this PR does not cover is the outdated description of the usage of with-ivy-window in Section 7.3 of the user manual. But those changes would probably require the copyright assignment.

@basil-conto
Copy link
Collaborator

basil-conto commented Jun 9, 2024

Its usage was subsequently replaced by (select-window (ivy--get-window ivy-last)) in 1c09e99.

Cross-referencing #639 and #665 for posterity.

@basil-conto
Copy link
Collaborator

Its usage was subsequently replaced by (select-window (ivy--get-window ivy-last)) in 1c09e99.

That commit says:

So now we use only the first part of `with-ivy-window', which
is (select-window (ivy--get-window ivy-last)).

But in fact, the first part of with-ivy-window is (select-window ... 'norecord), i.e. do not record this temporary window switch.

I'm inclined to bring back the norecord argument. Do you foresee any issues with that?

Either way, thanks for the careful analysis so far! As you can tell I'm still double checking the history and details of this part of the code.

@basil-conto
Copy link
Collaborator

This PR only contains repeated removal of with-ivy-window, so I assume it could potentially be treated as legally insignificant?

Yes, that's my understanding.

@basil-conto
Copy link
Collaborator

basil-conto commented Jun 9, 2024

One thing this PR does not cover is the outdated description of the usage of with-ivy-window in Section 7.3 of the user manual.

I can take care of these changes...

But those changes would probably require the copyright assignment.

...unless you prefer to get started on the CA process?

@basil-conto
Copy link
Collaborator

basil-conto commented Jun 9, 2024

This PR removes all with-ivy-window usages in actions in swiper.el

What about these two?

swiper/swiper.el

Lines 239 to 245 in 2a25a6f

(ivy-exit-with-action
(lambda (_)
(with-ivy-window
(move-beginning-of-line 1)
(let ((inhibit-read-only t))
(perform-replace from to
t t nil))))))

swiper/swiper.el

Lines 1514 to 1517 in 2a25a6f

(defun swiper-isearch-action (x)
"Move to X for `swiper-isearch'."
(if (setq x (swiper--isearch-candidate-pos x))
(with-ivy-window

@ywwry66
Copy link
Contributor Author

ywwry66 commented Jun 9, 2024

I'm inclined to bring back the norecord argument. Do you foresee any issues with that?

I don't see potential issues from my perspective. Maybe you can push a separate commit to this PR?

...unless you prefer to get started on the CA process?

I think I would avoid the lengthy paperwork unless absolutely necessary. So I will leave this part to you, thanks!

What about these two?

These were missed. Thanks for the quick review and I will update the PR.

@ywwry66
Copy link
Contributor Author

ywwry66 commented Jun 9, 2024

This usage might still be necessary because of 3656dfe?

swiper/swiper.el

Lines 239 to 245 in 2a25a6f

(ivy-exit-with-action
(lambda (_)
(with-ivy-window
(move-beginning-of-line 1)
(let ((inhibit-read-only t))
(perform-replace from to
t t nil))))))

`with-ivy-window` wrapper is legacy for actions.
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