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

Support C-c C-x shell inputs with counsel-shell commands #2234

Closed
wants to merge 1 commit into from

Conversation

leungbk
Copy link
Contributor

@leungbk leungbk commented Sep 15, 2019

In comint-mode and eshell-mode, if

> echo old
> echo new
> echo newer
[...]
> echo newest

is in your history, then M-p-ing all the way back to echo old, and RET C-c C-x executes echo old and inserts echo new, and RET C-c C-x again inserts echo newer.

I didn't write the appropriate adjustments to the new counsel-slime-repl-history, partly because slime doesn't have a C-c C-x-style command presently defined, and partly because I don't use slime that often.

@abo-abo
Copy link
Owner

abo-abo commented Oct 10, 2019

Thanks. I've merged the first two commits. As for the third one, I'd like ivy.el to have little to no knowledge of counsel.el. Let's figure out how to rewrite the third commit so that counsel.el hooks up via some new API, so that ivy.el doesn't have to know about symbols like counsel-esh-history.

@leungbk
Copy link
Contributor Author

leungbk commented Nov 18, 2019

OK, I've removed the counsel dependency.

@leungbk leungbk changed the title Support C-c C-x shell inputs with counsel-shell commands [WIP] Support C-c C-x shell inputs with counsel-shell commands Nov 18, 2019
@leungbk
Copy link
Contributor Author

leungbk commented Nov 18, 2019

Actually, maybe this shouldn't be merged yet. I recall some strange bug involving shell history that I never figured out. I'll need some time to get to the bottom of it.

@abo-abo
Copy link
Owner

abo-abo commented Nov 18, 2019

Thanks, let me know when it's ready.

@leungbk
Copy link
Contributor Author

leungbk commented Dec 18, 2019

I think I know what the issue is: it's a problem that occurs when eshell-hist-ignored-ups and comint-input-ignoredups are non-nil in Emacs' comint and eshell modes.

If

> echo a
> echo b
> echo b
> echo c

is in your history, then M-p-ing up to echo a and hitting RET and C-c C-x will execute echo a again and put echo b into the prompt as expected. But the second echo b doesn't get hopped over correctly with the RET C-c C-x chain when eshell-hist-ignoredups is t. If I don't have the duplicate echo b in this sequence, the inputs work exactly as they should; if eshell-hist-ignoredups is nil (the default), this also works.

I think this commit is ready for review now. I'm not sure why the CI fails on Emacs 26.3 alone, though.

@leungbk leungbk changed the title [WIP] Support C-c C-x shell inputs with counsel-shell commands Support C-c C-x shell inputs with counsel-shell commands Dec 18, 2019
@abo-abo
Copy link
Owner

abo-abo commented Dec 19, 2019

Thanks for the update. I propose to simplify the code like this:

(defun ivy--label-and-delete-dups (entries)
  "Label ENTRIES with history indices."
  (let ((ht (make-hash-table :test #'equal))
        (idx 0)
        entry accum)
    (while (setq entry (pop entries))
      (unless (gethash entry ht)
        (puthash entry t ht)
        (push `(,entry . ,idx) accum))
      (cl-incf idx))
    (nreverse accum)))

Regarding defadvice, I would prefer to avoid it. Can you try to fix it like this:

(counsel--browse-history
 eshell-history-ring
 :caller #'counsel-esh-history
 :index 'eshell-history-index)

(defun counsel--browse-history (ring &key caller index)
  "Use Ivy to navigate through RING."
  (setq ivy-completion-beg (point))
  (setq ivy-completion-end (point))
  (let ((ivy--index-to-fix index))
    (ivy-read "History: " (ivy-history-contents ring)
              :keymap ivy-reverse-i-search-map
              ;; the action should fix the index/ring so defadvice is not needed
              :action #'counsel--browse-history-action
              :caller caller)))

@leungbk leungbk force-pushed the counsel-shell-stuff branch 2 times, most recently from ce0b281 to 869d2f7 Compare Dec 21, 2019
@leungbk
Copy link
Contributor Author

leungbk commented Dec 21, 2019

OK, I fixed ivy--label-and-delete-dups.

(defun eshell-previous-matching-input-from-input (arg)
  "Search backwards through input history for match for current input.
\(Previous history elements are earlier commands.)
With prefix argument N, search for Nth previous match.
If N is negative, search forwards for the -Nth following match."
  (interactive "p")
  (if (not (memq last-command '(eshell-previous-matching-input-from-input
				eshell-next-matching-input-from-input)))
      ;; Starting a new search
      (setq eshell-matching-input-from-input-string
	    (buffer-substring (save-excursion (eshell-bol) (point))
			      (point))
	    eshell-history-index nil))
  (eshell-previous-matching-input
   (concat "^" (regexp-quote eshell-matching-input-from-input-string))
   arg))

The :action passed to ivy-read in this PR already adjusts eshell-history-index. The issue is that eshell-previous-matching-input-from-input, which is typically called interactively by the user, wipes eshell-history-index. A typical scenario is if I have

> echo a
> echo b
> echo a
> echo c

Without the advice, if I select echo a with counsel-eshell-history and then press <up> to execute eshell-previous-matching-input-from-input, because eshell-history-index gets cleared by the first part of this function, I will land on the exact same echo a the first time I press <up>.

I'm fine with simply not adding the advice at all, if you prefer; the only difference is that the user has to press <up> one more time in eshell-mode to get to an older occurrence of the string selected within the ivy prompt.

ivy.el (ivy-completion-in-region-action): Handle history pairs.
ivy.el (ivy-reverse-i-search): Update action to handle different type.
counsel.el (counsel-esh--index-last,
counsel-shell-history--index-last,
counsel-slime-repl-history--index-last): New variables.
counsel.el (counsel-set-eshell-history-index,
counsel-set-comint-history-index): New functions.
@leungbk
Copy link
Contributor Author

leungbk commented Jan 5, 2020

@abo-abo

@abo-abo abo-abo closed this in 69fedaa Jan 8, 2020
@abo-abo
Copy link
Owner

abo-abo commented Jan 8, 2020

Thanks.

@leungbk leungbk deleted the counsel-shell-stuff branch Jan 8, 2020
astoff pushed a commit to astoff/swiper that referenced this pull request Jan 1, 2021
astoff pushed a commit to astoff/swiper that referenced this pull request Jan 1, 2021
ivy.el (ivy-completion-in-region-action): Handle history pairs.
ivy.el (ivy-reverse-i-search): Update action to handle different type.
counsel.el (counsel-esh--index-last,
counsel-shell-history--index-last,
counsel-slime-repl-history--index-last): New variables.
counsel.el (counsel-set-eshell-history-index,
counsel-set-comint-history-index): New functions.

Fixes abo-abo#2234
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.

None yet

2 participants