Selectrum: x-group-function grouping breaks selection of indexed item with custom org source!

[minad]

See https://github.com/minad/consult/issues/240. cc @leungbk @clemera

[oantolin]

Weird, I cannot reproduce the issue here. The number skips the header lines, as I'd expect, and C-u 4 RET does select the candidate displayed next to the number 4.

[leungbk]

Weird, I cannot reproduce the issue here. The number skips the header lines, as I'd expect, and C-u 4 RET does select the candidate displayed next to the number 4.

If you're on Linux/Mac, could you save the following file as test-selectrum.nix and load a nix-shell from there to test? This seems to consistently reproduce the issue for me.

# 1. Download nix: https://nixos.org/download.html
# 2. $ nix-shell test-selectrum.nix
# 3. $ [within the nix-shell] emacs -q -l .emacs.d/selectrum-index-init.el

{ pkgs ? (import <nixpkgs> {
    overlays = [
      (import (builtins.fetchTarball {
        url = https://github.com/nix-community/emacs-overlay/archive/bb9fb65a68e18b4e136a0355155a0d03ce651729.tar.gz;
      }))
    ];
  })
}:

with pkgs;
let myEmacs =
  (emacsPackagesGen emacsUnstable).emacsWithPackages (epkgs: (with epkgs; [
    use-package
    consult
    selectrum
  ]));
in
mkShell {
  name = "emacs-shell";
  buildInputs = [
    myEmacs
  ];
}

[minad]

If you're on Linux/Mac, could you save the following file as test-selectrum.nix and load a nix-shell from there to test? This seems to consistently reproduce the issue for me.

No, this is asking too much. I am not a nix user. Sorry.

Does this overlay include the newest versions?

[leungbk]

No, this is asking too much. I am not a nix user. Sorry.

I also observe the indexing issue when using Straight to fetch the latest Consult and Selectrum revs.

Does this overlay include the newest versions?

Yes:

https://github.com/nix-community/emacs-overlay//blob/bb9fb65a68e18b4e136a0355155a0d03ce651729/repos/melpa/recipes-archive-melpa.json#L92811
https://github.com/nix-community/emacs-overlay//blob/bb9fb65a68e18b4e136a0355155a0d03ce651729/repos/melpa/recipes-archive-melpa.json#L15444

[oantolin]

So, @minad, you can't reproduce the issue either?

[minad]

I cannot reproduce it, when pressing C-3 RET etc, Selectrum returns the correct entry. I tested with selectrum-show-indices=t and selectrum-group-format=non-nil/nil.

[minad]

Could this be a prescient issue? @leungbk Could you please show the minimal configuration you are using?

[minad]

@oantolin Luckily, I just created https://github.com/raxod502/selectrum/pull/475.

EDIT: No, I cannot reproduce. Neither with Prescient nor with Orderless.

[minad]

Okay, now I can reproduce. The problem happens only after one adds this custom org source and selects a buffer from this source. Weird.

[minad]

The org source is broken. It was wrong in the wiki. See https://github.com/minad/consult/wiki#org-buffers. But the grouping really breaks the ordering, which should not happen. I have to investigate!

[minad]

Ah I think I know what the problem is. It is this feature of selectrum to move the default candidate. This breaks my grouping assumption. Probably the easiest solution for now is to disable the default candidate moving if grouping is enabled. Alternatively move the whole group containing the default candidate, then it will also work, but it is more complicated. Ping @clemera, what do you think?

EDIT: Quick workaround:
~ elisp
(setq consult-config '((consult-buffer :default-top nil)))
~

[minad]

This is not the first time I got confused by the movement of the default candidate. I got confused before why the sorting is wrong when I did some debugging. I am starting to believe that this is a actually a misfeature.

[clemera]

What about moving the default candidate right after the preprocessing?

[minad]

@clemera Sounds like a good idea - moving the candidate before doing the grouping! Does that always work or will this break other assumptions? Why is the candidate moved late currently? It is moved after refinement/filtering.

[minad]

@clemera The critical code is this:

~ elisp
(when (and selectrum-move-default-candidate
selectrum--default-candidate)
(setq-local selectrum--refined-candidates
(selectrum--move-to-front-destructive
selectrum--default-candidate
selectrum--refined-candidates)))
(setq-local selectrum--refined-candidates
(selectrum--move-to-front-destructive
;; Make sure matching dirnames are sorted first.
(if (and minibuffer-completing-file-name
(member (file-name-as-directory input)
selectrum--refined-candidates))
(file-name-as-directory input)
input)
selectrum--refined-candidates))
~

There is this movement based on the input. Why is that there? This movement can only be made in refinement, not before. We don't have input during preprocessing.

[clemera]

The movement based on input is so that when you exactly match a candidate you can select it. It can be annoying if you provide the exact input and need to scroll.

[minad]

Hmm, okay. I guess there these proper fixes:

1. Perform the grouping after the refinement and not during preprocessing. This is a bit more costly, but grouping is generally cheap.
2. Perform some whole group movement, instead of only moving the default candidate. I don't know how to do this yet. Essentially equivalent to option 1?
3. Do not perform the grouping during display, but use the same singleton hack as in icomplete-vertical (@oantolin knows what I talk about). But this could hack groups into two pieces.
4. If the default candidate has been moved, only perform grouping on the cdr of the candidates. Mostly equivalent to 3, but the default candidate would not get a group header.

[clemera]

If it is fast enough 1 sounds most attractive to me.

[minad]

There is one downside of 1 - then the grouping function has the final say on the ordering. This means it could again decide to move the group containing the default candidate around, such that it is not at the top. This breaks the assumption of selectrum, which is that the candidate index is 0 if move-default-candidate=t. The consult--group-candidates function does not do that, but it is generally allowed, that the group function decides the order of the groups.

[clemera]

Okay, that wouldn't be nice. Maybe we should better follow your first suggestion and just avoid changing the ordering when any grouping is active.

[minad]

Okay, this actually sounds like the best solution. But it would also disallow the movement of the "input" candidate movement as you have it above. Generally any movement of candidates after preprocessing is disallowed if grouping is used. I am sorry that I missed this :(

[minad]

But the assumption with index 0 is used here - we could simply remove this code and then it falls down to cl-position. By doing that option 1 would work.

~ elisp
(selectrum-move-default-candidate
0)
(t
(or (cl-position selectrum--default-candidate
selectrum--refined-candidates
:key #'selectrum--get-full
:test #'equal)
0))))
~

[clemera]

Seems like a great solution!

[minad]

Alternative proposal, based on proposal 1 with minor optimization:

1. Remove the assumption of index=0, when selectrum-move-default-candidate
2. Move the default candidate during preprocessing, group afterwards (now index=0 can be violated)
3. Move the input candidate during refinement, but only if it is member of the list, group afterwards (this is basically a fixup pass). But then once again the input candidate could not be at the top and then you would have to scroll, which is not good.

If one wants to avoid scrolling for the "input candidate", there is no solution except proposal 3 or 4.

[clemera]

Hm, what about selecting the prompt in case the input is an exact match instead of moving the candidate around?

[clemera]

But maybe that could be confusing during input, it would flash the candidate the moment you input an exact match.

[minad]

Hm, what about selecting the prompt in case the input is an exact match instead of moving the candidate around?

If we could do that, it would be great. I don't know how this works.

Then we could do the following:

1. Remove the assumption of index=0, when selectrum-move-default-candidate
2. Move the default candidate during preprocessing, group afterwards (now index=0 can be violated)
3. Use prompt match for input candidate, and do not move it around.
4. Require that refinement never moves candidates.

[minad]

One other idea, based on proposal 3 - instead of using the prompt selection. Allow groups to be split, such that you get for example two --buffer-- headers, if the input candidate is moved. By moving the default candidate after preprocessing before grouping would at least ensure that it does not happen for the default candidate. This would violate the index=0 assumption, but that is okay.

Could you elaborate a bit more why this prompt selection would not work nicely?

[minad]

Hmm, okay. I guess it makes sense to go with option 3 actually. The user of completing-read and x-group-function has to ensure that the default candidate is always in the first group. If this holds we don't have a problem. The only question is what to do about the input movement. This could still lead to a group split, but I guess it would also be rare since then the input string must match candidates in other groups which happen to appear before the group containing the actual exact match candidate. I think option 3 is the best, it requires the least assumptions. This is the same I am doing in icomplete-vertical btw, it is robust.

[clemera]

It could highlight the candidate in the middle of input, if you proceed it gets deselected again maybe it wouldn't be too bad but it would also be problematic with the special handling we do for dirs in file completions. But now that you reconsider 3. this might be irrelevant anyway, I don't know what the singleton hack is about so I dismissed it right away ;)

[minad]

It just means calling the group function for each displayed candidate separately. This requires single element "singleton" lists, containing the candidate. But this is actually not bad, since we only call it for a handful of candidates, namely the ones being displayed.

[clemera]

Okay, thanks for explaining, now I also see why this could lead to split groups but as you say that would be rare.

[minad]

Fix PR is incoming...