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

Handle minibuffer completion table, predicate and metadata #95

Merged

Conversation

clemera
Copy link
Collaborator

@clemera clemera commented May 20, 2020

This will allow us to access the metadata for purposes as discussed in #15 and #82 and allows other packages and commands to access those variables (see #94). With this patch we can retrieve the metadata like:

(completion-metadata input
                     minibuffer-completion-table
                     minibuffer-completion-predicate)

We could also decide to only pass the metadata instead (which would probably reduce garbage collection when exiting the minibuffer I guess). But the metadata can depend on the current input and the predicate passed to the completion table. Having metadata dependent on the input or predicate sounds like a crazy idea and I don't know if it actually happens in practice.

@clemera clemera changed the title [#94] Pass minibuffer completion table and predicate where appropriate Pass minibuffer completion table and predicate where appropriate May 20, 2020
@clemera
Copy link
Collaborator Author

clemera commented May 20, 2020

This PR fails with CI for emacs-master but I can't check why because I don't want to give them read and write access for all my repos which they apparently require to show me the results for some reason.

@clemera clemera changed the title Pass minibuffer completion table and predicate where appropriate Pass minibuffer completion table and predicate May 21, 2020
@clemera clemera force-pushed the handle-minibuffer-completion-variables branch 2 times, most recently from de64ebb to 428328c Compare May 21, 2020 08:30
@clemera clemera force-pushed the handle-minibuffer-completion-variables branch from 428328c to 850ef7a Compare May 21, 2020 08:31
@raxod502
Copy link
Member

image

You should also be able to test locally, make docker VERSION=master.

@clemera
Copy link
Collaborator Author

clemera commented May 24, 2020

Thanks, it's fixed now.

@raxod502 raxod502 added the waiting on response Needs more info or follow-up, will be closed after 90 days if no response label May 25, 2020
@raxod502
Copy link
Member

See #94 (comment).

Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, seems fine. Could you add a changelog entry?

selectrum.el Outdated
For MINIBUFFER-COMPLETION-TABLE and
MINIBUFFER-COMPLETION-PREDICATE see `minibuffer-completion-table'
and `minibuffer-completion-predicate'. They are used for internal
purposes and compatibility to Emacs completion API."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably explicitly mention that passing the keyword arguments will dynamically bind the corresponding variables, as otherwise you have to be extremely familiar with binding semantics and the implementation of cl-defun to guess that it has this effect.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, have done it.

@clemera
Copy link
Collaborator Author

clemera commented May 27, 2020

The embark package has made a lot of progress and can be used with selectrum for occur and actions. Currently there is one last issue with selectrum: One of the tricks embark uses is to call a command, insert a candidate into the minibuffer and exit with it. With default completion there is no extensive computation when a completing-read session starts because the candidates are only computed on TAB. With selectrum the candidates get currently computed beforehand which introduces a noticeable delay especially for slow/large completion tables.

With this PR standard completion tables are now computed once in selectrum--minibuffer-post-command-hook. This way the general behaviour does not change but one can easily skip the computation if needed, for example with embark the delay issue is gone using this PR and the following config for embark:

(add-hook 'embark-setup-hook
          (defun embark-selectrum-handle-inject ()
            (when selectrum--active-p
              ;; use the injected candidate and make sure selectrum doesn't
              ;; update afterwards
              (setq selectrum--current-candidate-index -1)
              (setq selectrum--previous-input-string
                    (buffer-substring
                     selectrum--start-of-input-marker
                     selectrum--end-of-input-marker)))))

Another benefit of computing the table in post command hook is that we could offer an selectrum-exhibit function (analogue to icomplete-exhibit):

(defun selectrum-exhibit ()
  "Refresh completion."
  (when-let ((mini (active-minibuffer-window)))
    (with-selected-window mini
      (when minibuffer-completion-table
        ;; trigger recomputation and display update
        (setq selectrum--preprocessed-candidates nil)
        (setq selectrum--previous-input-string nil)
        (selectrum--minibuffer-post-command-hook)))))

This can for example be used for creating simple async completion tables.

If you want to checkout embark yourself here is my current setup to configure it for selectrum (at current PR), you should omit the which-key section if you don't use it.

@clemera clemera force-pushed the handle-minibuffer-completion-variables branch 4 times, most recently from 6aecf47 to 38d7a59 Compare May 28, 2020 11:27
@clemera clemera force-pushed the handle-minibuffer-completion-variables branch from 38d7a59 to 58a31e6 Compare May 28, 2020 11:30
@clemera
Copy link
Collaborator Author

clemera commented May 28, 2020

I also added support for display-sort-function metadata now. You can test with:

(completing-read "Check: "
                 (lambda (string pred action)
                   (if (eq action 'metadata)
                       `(metadata
                         (display-sort-function
                          . ,(lambda (seq) (sort seq #'string<))))
                     (complete-with-action action '("c" "b" "a") string pred))))

@clemera clemera force-pushed the handle-minibuffer-completion-variables branch 3 times, most recently from dec4daa to 370f37d Compare June 3, 2020 17:59
@clemera clemera force-pushed the handle-minibuffer-completion-variables branch from 370f37d to f08fb78 Compare June 3, 2020 18:00
@clemera clemera force-pushed the handle-minibuffer-completion-variables branch 3 times, most recently from 822aac0 to 9e4390f Compare June 4, 2020 10:51
@clemera clemera force-pushed the handle-minibuffer-completion-variables branch from 9e4390f to 2193ad0 Compare June 4, 2020 10:54
@clemera
Copy link
Collaborator Author

clemera commented Jun 4, 2020

Thanks, I finished updating the CHANGELOG and also added the mentioned selectrum-exhibit function from above.

@clemera clemera force-pushed the handle-minibuffer-completion-variables branch from 2d876b5 to 4a926b8 Compare June 4, 2020 12:35
CHANGELOG.md Outdated Show resolved Hide resolved
selectrum.el Outdated Show resolved Hide resolved
selectrum.el Outdated Show resolved Hide resolved
clemera and others added 3 commits June 12, 2020 17:49
Co-authored-by: Radon Rosborough <radon.neon@gmail.com>
Co-authored-by: Radon Rosborough <radon.neon@gmail.com>
Co-authored-by: Radon Rosborough <radon.neon@gmail.com>
@clemera
Copy link
Collaborator Author

clemera commented Jun 15, 2020

Turned out that completion-extra-properties annotations also need to be handled for completing-read (see proced-send-signal for a built-in example). I previously thought that they would only be relevant for completion-in-region.

@clemera clemera force-pushed the handle-minibuffer-completion-variables branch from 89ea467 to 47b4631 Compare June 15, 2020 12:33
@clemera clemera changed the title Handle minibuffer completion table and predicate Handle minibuffer completion table, predicate and metadata Jun 17, 2020
Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much!

@raxod502 raxod502 merged commit c793497 into radian-software:master Jun 21, 2020
@raxod502 raxod502 removed the waiting on response Needs more info or follow-up, will be closed after 90 days if no response label Jun 21, 2020
@clemera
Copy link
Collaborator Author

clemera commented Jun 21, 2020

Thanks for the feedback and review!

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

Successfully merging this pull request may close these issues.

2 participants