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 #2663 #2781

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

Fix #2663 #2781

wants to merge 2 commits into from

Conversation

Nathoufresh
Copy link

All right after some testing I found out we just need to (setq history-add-new-input nil) and no modifications to ivy--update-history.

Ivy-read calls read-from-minibuffer which will add the selection to history if history-add-new-input is non-nil.

My issue is gone and the test are ok.

But now it will break some workflow like if someone prefer let's say find-file instead of counsel-find-file the history won't work. It's not really efficient but as a workaround instead of passing the real hist var to read-from-minibuffer we can pass a copy of it and so we can leave history-add-new-input as it is.

ivy.el Outdated Show resolved Hide resolved
@Nathoufresh
Copy link
Author

Not working with copy-sequence. I have no idea why

@basil-conto
Copy link
Collaborator

Not working with copy-sequence.

What isn't working exactly?

What about copy-alist?

@Nathoufresh
Copy link
Author

Most tests fails with copy-squence or copy-alist but everything passe with copy-tree.
Logs.

From the copy-tree's docs

If TREE is a cons cell, this recursively copies both its car and its cdr.
Contrast to ‘copy-sequence’, which copies only along the cdrs.

@basil-conto
Copy link
Collaborator

Most tests fails with copy-squence or copy-alist

That's expected, because hist is usually a symbol, so passing it directly to copy-sequence or copy-alist is wrong.

but everything passe with copy-tree.

That's because copy-tree returns its argument unchanged if it is an atom.

That means this PR is a no-op, so I'm wondering which issue it's trying to solve, and whether it's an issue at all.

@Nathoufresh
Copy link
Author

Nathoufresh commented Mar 25, 2021

When you invoke counsel-M-x and you type ^ev buf and select eval-buffer in the history there will be eval-buffer then ^ev buf which I find not really convenient. An easy way to fix it is to set history-add-new-input to nil so you'd have only ^ev buf added to the history. But if let's say I don't use counsel for a command then I'll have no history at all for that command. I'm just trying to find a way to remove the eval-buffer while keeping history-add-new-input to true.

I think when I created this PR I've lost myself a bit and yeah, my method is definitely not working...

@basil-conto
Copy link
Collaborator

When you invoke counsel-M-x and you type ^ev buf and select eval-buffer in the history there will be eval-buffer then ^ev buf which I find not really convenient.

I agree, but I'm not sure whether @abo-abo introduced this intentionally.

I'm just trying to find a way to remove the eval-buffer while keeping history-add-new-input to true.

What if some users prefer the opposite: to keep eval-buffer in the history instead of ^ev, for consistency and better integration with vanilla Emacs completion? (In fact I think this discussion has come up before.)

So I think this behaviour should be customisable somehow.

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

Successfully merging this pull request may close these issues.

2 participants