Update relevant index in consult-history

[leungbk]

To do this, we change the shape of consult-mode-histories and define
consult--label-and-delete-dups.

This allows C-c C-x to work within eshell-mode.

If my Eshell history has

echo a
echo b
echo c

then after I select echo a with consult-history and press RET, then I can fetch echo b using C-c C-x, without having to start a new consult-history session or mashing M-p.

[minad]

This looks to complicated for my taste what you are doing here. But I agree that it is a good idea to do this.

[minad]

@leungbk After looking at your PR again I got my doubts if this is the correct thing to do: consult-history is meant to be random access - I am not sure if after every random access the history reference index should be adjusted. E.g. when browsing through the history (e.g. via next-history-element for the minibuffer), the changed position is used which may not be what a user expects.

Maybe it makes more sense to instead push the selected element to the beginning of the history, either removing the item at the old position or keeping it? This would also simplify things, since no index updating would be needed. Furthermore I wonder why the history is not updated automatically when executing the command, such that we would not have to do anything here. What do you think?

For now I added the consult--remove-dups function to the main branch and replaced all delete-dups and seq-uniq usages with it, since this is pretty independent from this PR.

[leungbk]

@leungbk After looking at your PR again I got my doubts if this is the correct thing to do: consult-history is meant to be random access - I am not sure if after every random access the history reference index should be adjusted. E.g. when browsing through the history (e.g. via next-history-element for the minibuffer), the changed position is used which may not be what a user expects.

comint-history-isearch-backward-regexp in core Emacs works in the same way as what this PR envisions for consult-history, by reassigning the index according to what the user has selected. If they want to start over with a fresh history index, then in comint/eshell they can submit a blank input and then mash M-p within the same shell buffer, or within the minibuffer they can C-g and start a new minibuffer session.

The closest analogue in eshell seems to be eshell-isearch-backward. This does not reassign the index; I thinkEshell's behavior is less intuitive.

If we look at the behavior of the comint/eshell equivalent of next-history-element, which would probably be *-next-matching-input, the behavior observed in comint/eshell with this PR presently aligns with your expectations.

If I have

echo a
echo b
echo c
echo a

in my history, then selecting echo a and pressing M-p (eshell-previous-matching-input-from-input) would actually fetch me the most recent echo a. This is because *-previous-matching-input-from-input contains a check (https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/eshell/em-hist.el#n892) that reassigns eshell-history-index to nil if the last-command was not *-previous/next-matching-input-from-input. So at least for comint and eshell, the index reassignments I have in this PR would be wiped out after calling a *-*-matching-input-from-input command.

Back when I wrote the equivalent of this PR for Counsel, I did the following to ensure that the index used by the first invocation of eshell-previous-matching-input-from-input corresponded to the recently-selected candidate:

https://github.com/abo-abo/swiper/blob/b68f1ec01e6000db83e20767271ac86dea5e5c95/counsel.el#L4817-L4824

I can't write any equivalent here since I don't think there's a way of retrieving the :caller of the last completing-read session (current-minibuffer-command introduced in Emacs 28 is bound only recursively, and is not going to tell us anything about the last minibuffer command if we're not presently in the minibuffer).

This is only tangentially related to what I sought to do with this PR though, which was to make the C-c C-x commands of comint and eshell work with consult-history. If you feel strongly about keeping the relevant index untouched so that the next-history-element-alikes will start from the beginning, all I'd need to do is to not reassign the index for minibuffer-history-position; I wouldn't need to prevent myself from modifying the comint/eshell indices since the *-previous-matching-input-from-input commands will reset their indices unless the last-command is a *-*-matching-input-from-input command.

Maybe it makes more sense to instead push the selected element to the beginning of the history, either removing the item at the old position or keeping it? This would also simplify things, since no index updating would be needed.

It's not clear to me how your suggestion of splicing the selected candidate without otherwise doing any index manipulation will give the *-get-next-from-history commands the relevant info on what must be selected after executing C-c C-x.
Without something like this PR, selecting something, submitting it to the prompt, and pressing C-c C-x won't be expected to yield the right successor input.

comint-history-isearch-backward-regexp already supports C-c C-x via the same kind of index reassignment that I am trying to implement in this PR.

Furthermore I wonder why the history is not updated automatically when executing the command, such that we would not have to do anything here. What do you think?

It is already updated in the way that you describe without this PR.

To assuage your concerns about next/previous-history-element for the minibuffer while still supporting the C-c C-x stuff for comint and eshell, I propose something like the following change to my PR within consult-history in place of the pcase statement there:

(when (memq index-variable '(eshell-history-index
                             comint-input-ring-index))
  (setf (symbol-value index-variable) candidate-index))

Again, the comint and eshell *-*-previous-matching-input-from-input commands wipe out any index reassignments I make, so that the modified index is used only by C-c C-x, and for the minibuffer, I simply won't adjust the index at all. This won't break any other stuff that the user adds to consult-mode-histories, it will only provide this extra behavior for comint and eshell.

What do you think?

[minad]

Oh, that's a long argument here and I must admit that I have a hard time to follow.

Could you explain to me without assuming any prior knowledge on how all these histories work and on how counsel works etc, why you would want to reassign the index, if the current behavior already pushes the last selected element to the top of the history? Could you explain what the purpose of this index is after all? Under which circumstances is this index shifted?

My point is that by changing the index, we are somehow destroying the reference point of the history and I feel this goes a bit against the spirit of having consult-history as a random access interface, since we also get this side-effect on top.

And then you say some things that there are effects which wipe out the index reassignment of this PR. Why are we doing this then?

[minad]

I am reading again reading your very first comment in this PR:

then after I select echo a with consult-history and press RET, then I can fetch echo b using C-c C-x, without having to start a new consult-history session or mashing M-p.

So you want to sequentially replay the history somehow from the last selected history item? Is this the idea? I think I originally misunderstood the purpose of this PR.

[leungbk]

So you want to sequentially replay the history somehow from the last selected history item? Is this the idea? I think I originally misunderstood the purpose of this PR.

Yes, that's the sole purpose of this PR.

My point is that by changing the index, we are somehow destroying the reference point of the history and I feel this goes a bit against the spirit of having consult-history as a random access interface, since we also get this side-effect on top.

The latest version of this PR assuages this concern.

• minibuffer-history-position is never modified
• Although comint/eshell's *-*-get-previous-matching-input-from-input will make use of their respective indices, they will check to ensure that last-command is a certain thing before using the existing values of the indices.
• C-c C-x in comint and eshell perform no such checks, so if we wish to support that command, we would adjust the indices ourselves.

[minad]

@leungbk Ok, I get this now. You select the index and then you press C-c C-x to execute the history step by step from this point. The index replay does not make much sense for the minibuffer though, therefore you removed it there?
I think, I agree with the change - but I still don't want comint and eshell indices hardcoded (I think you have that right now).

So I guess it should work like this:

• Allow the two possible variants to be configured in the mode history list: (mode history index) and (mode history)
• If the user configures no index, no index update happens. If an index is configured, the adjustment is supposed to happen. This means we can support the current behavior and the new behavior by configuring the variable, which is good.
• No index readjustment for the minibuffer should happen, since it is probably not useful?
• No advice stuff should be added to comint/eshell as you did for counsel. I would like to keep consult free of such hacks.

Could you please add a bit more documentation to the customizable variable and the command such that it is clear what is going on and how the user can configure the behavior? But besides that I think your PR is already along these lines?

[leungbk]

So I guess it should work like this:

* Allow the two possible variants to be configured in the mode history list: (mode history index) and (mode history)

* If the user configures no index, no index update happens. If an index is configured, the adjustment is supposed to happen. This means we can support the current behavior and the new behavior by configuring the variable, which is good.

* No index readjustment for the minibuffer should happen, since it is probably not useful?

* No advice stuff should be added to comint/eshell as you did for counsel. I would like to keep consult free of such hacks.

Done.

Could you please add a bit more documentation to the customizable variable and the command such that it is clear what is going on and how the user can configure the behavior? But besides that I think your PR is already along these lines?

Done.

[minad]

Great! Thanks! Why not do the same for the term-mode?

[leungbk]

Great! Thanks! Why not do the same for the term-mode?

I don't know how to anything in term-mode, and term-mode does not appear to have any C-c C-x command like what eshell and comint have, so I'm not aware of the benefits of adjusting term-input-ring-index within consult-history; do you have any in mind?

[minad]

I don't know how to anything in term-mode, and term-mode does not appear to have any C-c C-x command like what eshell and comint have, so I'm not aware of the benefits of adjusting term-input-ring-index within consult-history; do you have any in mind?

No, this is good enough for me. I just wanted to make sure that it is intentional.

[minad]

Are you finished here? Then I can pull this.

[leungbk]

Yeah, there was something I missed earlier, but it should be ready to go now. Thanks.

…

On Tue, Dec 22, 2020 at 11:33 AM Daniel Mendler ***@***.***> wrote: Are you finished here? Then I can pull this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#83 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG65GOXWYM7LOSNI6XNHFU3SWDX6XANCNFSM4VERP24Q> .

[minad]

@leungbk Now, I found another issue - while we are not sorting, we are deleting duplicates! I am once again doubtful if this is the right thing to do, since this will totally mess up the history replay and the user will wonder in the end what is going on since the candidate view does not match the real history.

Now I am not sure how to proceed. I see the following options:

1. Do not merge this - does it pull its weight if we have too many edge cases.
2. Use a universal argument to select the indexed behavior. In that case, no duplicates are removed.
3. Add another command consult-command-index which does the adjustment, consult-command does not adjust.

Note that I pushed a few minor adjustments to this branch.

[minad]

My problem with this whole thing is that the linear history with duplicates and completing-read selection does not work well together. Assume you have a long history and you want to select the starting point of a command sequence, then you basically have to search the starting point via scrolling, if the start command appears at multiple places and you only know depending on the context if this is the real starting point you are looking for.

[leungbk]

My problem with this whole thing is that the linear history with duplicates and completing-read selection does not work well together. Assume you have a long history and you want to select the starting point of a command sequence, then you basically have to search the starting point via scrolling, if the start command appears at multiple places and you only know depending on the context if this is the real starting point you are looking for.

I didn't address this before since I didn't think anyone would want to see duplicates. But for someone who does want to see them, something like your option 3) above seems fine to me if in the case where we keep duplicates, we also label each candidate with its index in the display to disambiguate between the duplicates we keep, similar to how consult-line labels each line with the actual line number.

[minad]

Well, if I want replay I want for sure see the duplicates - otherwise I really don't understand the point here. But as I argued, completing-read is not the best fit to select a starting point of a list of candidates, it suffers from the same problems as consult-line where the context is lost as soon as you narrow. I think consult-line is the only command where the context plays a role? As of now the consult history commands neglect the context since duplicates are deleted and no index adjustment is done.

[leungbk]

Well, if I want replay I want for sure see the duplicates - otherwise I really don't understand the point here.

If for a particular candidate, either

1. that candidate is not duplicated, or
2. that candidate is duplicated, but you want to replay from the most recent occurrence

then everything works as expected. In this case, we are satisfied.

In the other case, where the candidate is duplicated and you need to go back to an earlier occurrence, then your options are to

1. if the consult-history session stores duplicates, mash selectrum-next-candidate
2. if the consult-history session stores duplicates and allows the search string to match the index (not sure if I am fond of this), then type in the index to try and match
3. simply mash comint-previous-matching-input
4. manually look inside the file (if any) storing the history

I do not see a circumstance in which this PR, if it is adjusted to allow duplicate entries for comint and eshell, leaves the user worse off than they would be without it: even in the latter case, option 1 is not worse than the closest existing alternative, which is to type a search string into the comint prompt and mash comint-previous-matching-input-from-input (option 3).

But as I argued, completing-read is not the best fit to select a starting point of a list of candidates, it suffers from the same problems as consult-line where the context is lost as soon as you narrow.

I don't disagree that it's not the perfect interface, but in the happy case for comint and eshell, we are unequivocally better off, and in the other case, we are no worse off.

As of now the consult history commands neglect the context since duplicates are deleted and no index adjustment is done.

Since we're not doing any index adjustment for the minibuffer case, it might make sense to separate the portions relating to minibuffer history into a separate consult-minibuffer-history command.

[minad]

I do not see a circumstance in which this PR, if it is adjusted to allow duplicate entries for comint and eshell, leaves the user worse off than they would be without it: even in the latter case, option 1 is not worse than the closest existing alternative, which is to type a search string into the comint prompt and mash comint-previous-matching-input-from-input (option 3).

I don't disagree that it's not the perfect interface, but in the happy case for comint and eshell, we are unequivocally better off, and in the other case, we are no worse off.

Yes, I agree. But this basically holds for every feature you could possibly implement as long as it doesn't degrade existing features. Therefore this is not useful criterion on which one can decide if this is worth it or not. I am not saying no here, I am just stating some objections and I would prefer if we think a bit more about it - maybe we come up with something better? Generally I would like to avoid feature creep in this project. But the project is still young, so we have to find a way and some guidelines on what makes sense and what doesn't (also see the wishlist discussions).

Since we're not doing any index adjustment for the minibuffer case, it might make sense to separate the portions relating to minibuffer history into a separate consult-minibuffer-history command.

I would like to avoid this, since it is nice to have this context-dependent command and a single history keybinding. One could bind the minibuffer to the same command only in the minibuffer, though. But I don't see the minibuffer detection adding significantly to the complexity.

[minad]

@oantolin Do you have an opinion on the index adjustment, since the history command is originally based on your https://github.com/oantolin/completing-history?

[leungbk]

One option to show the relevant context for duplicates would be to offer opt-in previewing with a customizable amount of future entries to show.

echo a
echo b
[...]
echo a
echo z

With duplicate occurrences of echo a, then with previewing on, the preview action could, when the minibuffer is highlighting a candidate, scroll down to the prompt area in the comint/eshell buffer and display

echo a
echo b
# (more if the context defcustom is high)

or similarly for the other occurrence of echo a. Selecting the candidate with RET would simply insert echo a.

[oantolin]

@oantolin Do you have an opinion on the index adjustment, since the history command is originally based on your https://github.com/oantolin/completing-history?

Sorry, no opinion at all. This is functionality I've never used, never been aware existed and never wanted. 🤷

[minad]

@oantolin Okay, thank you!

[leungbk]

@minad What do you think about my suggestion above?

One option to show the relevant context for duplicates would be to offer opt-in previewing with a customizable amount of future entries to show.

echo a
echo b
[...]
echo a
echo z

With duplicate occurrences of echo a, then with previewing on, the preview action could, when the minibuffer is highlighting a candidate, scroll down to the prompt area in the comint/eshell buffer and display

echo a
echo b
# (more if the context defcustom is high)

or similarly for the other occurrence of echo a. Selecting the candidate with RET would simply insert echo a.

[minad]

@leungbk Yes, I've seen it, but it makes things more complicated. The same with consult-line if you show additional context, or I don't understand precisely where do you want the context to be shown.You want to show the context in the prompt?

[leungbk]

@leungbk Yes, I've seen it, but it makes things more complicated. The same with consult-line if you show additional context, or I don't understand precisely where do you want the context to be shown.You want to show the context in the prompt?

I would like to show the future context within the comint/eshell buffer as an opt-in preview action.

[minad]

I would like to show the future context within the comint/eshell buffer as an opt-in preview action.

I would find that a bit weird, but it is certainly a possibility - I am not sure though. I don't think the additional complexity is justified tbh. This feature has to pull its weight or we shouldn't add it.