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

add selectrum module #4519

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
63 changes: 63 additions & 0 deletions modules/completion/selectrum/autoload/selectrum.el
@@ -0,0 +1,63 @@
;;;###autoload
(cl-defun +selectrum-file-search (&key query in all-files (recursive t) prompt args)
"Conduct a file search using ripgrep.

:query STRING
Determines the initial input to search for.
:in PATH
Sets what directory to base the search out of. Defaults to the current project's root.
:recursive BOOL
Whether or not to search files recursively from the base directory."
(declare (indent defun))
(unless (executable-find "rg")
(user-error "Couldn't find ripgrep in your PATH"))
(require 'consult)
(setq deactivate-mark t)
(let* ((this-command 'consult--grep)
(project-root (or (doom-project-root) default-directory))
(directory (or in project-root))
(args (split-string
(string-trim
(concat (if all-files "-uu")
(unless recursive "--maxdepth 1")
"--null --line-buffered --color=always --max-columns=500 --no-heading --line-number"
" --hidden -g!.git "
(mapconcat #'shell-quote-argument args " ")))
Comment on lines +21 to +25
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a space in front of --hidden but not --maxdepth 1 or --null ?

" "))
(prompt (or prompt
(format "rg [%s]: "
(cond ((equal directory default-directory)
"./")
((equal directory project-root)
(projectile-project-name))
((file-relative-name directory project-root))))))
(query (or query
(when (doom-region-active-p)
(replace-regexp-in-string
"[! |]" (lambda (substr)
(cond ((and (string= substr " ")
(not (featurep! +fuzzy)))
" ")
((string= substr "|")
"\\\\\\\\|")
((concat "\\\\" substr))))
(rxt-quote-pcre (doom-thing-at-point-or-region))))))
(ripgrep-command `("rg" ,@args "." "-e")))
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading this correctly, you converted args into a string to concatenate more arguments to it inside the let binding, than here, you expand it to a list again in order to store it on ripgrep-command variable.

Wouldn't be more efficient to only do list operations here?

I was thinking on something like this:

(let* ((this-command 'consult-ripgrep)
         (project-root (or (doom-project-root) default-directory))
         (directory (or in project-root))
         (prompt (or prompt
                     (format "%s [%s]: "
                             (mapconcat #'identity command " ")
                             (cond ((equal directory default-directory) "./")
                                   ((equal directory project-root) (projectile-project-name))
                                   ((file-relative-name directory project-root))))))
         (query (or query
                    (when (doom-region-active-p)
                      (replace-regexp-in-string
                       "[! |]" (lambda (substr)
                                 (cond ((string= substr " ") "  ")
                                       ((string= substr "|") "\\\\\\\\|")
                                       ((concat "\\\\" substr))))
                       (rxt-quote-pcre (doom-thing-at-point-or-region))))))
         (ripgrep-command (seq-remove 'null
                                      (append (butlast consult-ripgrep-command)
                                              (list
                                               (when all-files "-uu")
                                               (unless recursive " --maxdepth 1")
                                               "--hidden"
                                               "-g!.git")
                                              args
                                              '("-e")))))
    ...
    )

(consult--grep prompt ripgrep-command directory query)))
Copy link
Contributor

Choose a reason for hiding this comment

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

The ivy counterpart of this function, sets the variable deactivate-mark before running the grep command. Shouldn't you do the same?


;;;###autoload
(defun +selectrum/project-search (&optional arg initial-query directory)
"Peforms a live project search from the project root using ripgrep.

If ARG (universal argument), include all files, even hidden or compressed ones,
in the search."
(interactive "P")
(+selectrum-file-search :query initial-query :in directory :all-files arg))

;;;###autoload
(defun +selectrum/project-search-from-cwd (&optional arg initial-query)
"Performs a live project search from the current directory.

If ARG (universal argument), include all files, even hidden or compressed ones."
(interactive "P")
(+selectrum/project-search arg initial-query default-directory))
64 changes: 64 additions & 0 deletions modules/completion/selectrum/config.el
@@ -0,0 +1,64 @@
;;; completion/selectrum/config.el -*- lexical-binding: t; -*-

(use-package! selectrum
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you manually set the refine, and highlight candidates functions if +orderless is enabled? Also, if that is the case, and the user has +prescient enabled as well, you should set the preprocesscandidates function as well:

  (setq selectrum-refine-candidates-function #'orderless-filter 
        selectrum-highlight-candidates-function #'orderless-highlight-matches 
        selectrum-preprocess-candidates-function #'selectrum-prescient--preprocess)

Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot to mention that if you want to use orderless and prescient, you should tell prescient to remember candidate selection:

  (add-hook 'selectrum-candidate-selected-hook #'selectrum-prescient--remember)
  (add-hook 'selectrum-candidate-inserted-hook #'selectrum-prescient--remember)

Copy link
Contributor

Choose a reason for hiding this comment

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

Setting separate refine and highlight functions is not necessary since Selectrum picks up the completion style, see radian-software/selectrum#330. However you should still set them in order to improve performance, since then Selectrum is applying highlighting only to the items which match the filter criterion! Note that the combination of Orderless and Prescient is a bit fragile since both packages compete about the filtering. I haven't checked what you are doing here, but selectrum-prescient-mode should be activated first, then orderless should overwrite the filter/highlighting functions. Furthermore orderless should be set as completion style.

:hook (doom-first-input . selectrum-mode)
:config
(setq selectrum-extend-current-candidate-highlight t
selectrum-fix-minibuffer-height t)
(unless (featurep! +orderless)
(setq completion-styles '(substring partial-completion))))

(when (featurep! +prescient)
jethrokuan marked this conversation as resolved.
Show resolved Hide resolved
(use-package! selectrum-prescient
:after selectrum
:hook ((selectrum-mode . selectrum-prescient-mode)
Copy link
Contributor

Choose a reason for hiding this comment

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

You should only do this if use does not have +orderless enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment above, about the order of selectrum-prescient-mode and orderless.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't enable selectrum-prescient-mode, you can use prescient sorting plus storing with orderless filter and highlights with no problem. If the user don't want this combination, then you don't need to worry as I comment, but overall, my suggestion is to get this decision path:

(cond ((and (featurep! +prescient) (featurep! +orderless)) (set-functions-manually))
       ((featurep! +prescient) (enable-selectrum-prescient-mode))
       ((featurep! +orderless) (only-enable-orderless)))

So, the use-package! for selectrum would be something like this:

(use-package! selectrum
  :init
  (when (and (featurep! +prescient) (featurep! +orderless))
    (setq selectrum-refine-candidates-function #'orderless-filter 
          selectrum-highlight-candidates-function #'orderless-highlight-matches 
          selectrum-preprocess-candidates-function #'selectrum-prescient--preprocess)

    (add-hook 'selectrum-candidate-selected-hook #'selectrum-prescient--remember)
    (add-hook 'selectrum-candidate-inserted-hook #'selectrum-prescient--remember))
  :config
  ;; other configurations...
  )

And selectrum-prescient would be:

(use-package! selectrum-prescient
    :if (featurep! +prescient) 
    :after selectrum
    : init
    (when (not (featurep! +orderless))
      (add-hook 'selectrum-mode-hook 'selectrum-prescient-mode))
    :config
    ;; other configurations
    )

(selectrum-mode . prescient-persist-mode))))

(use-package! orderless
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we lazy initialize orderless as well? If we can't, a comment explaining why would be helpful.

Copy link
Contributor

@minad minad Jan 24, 2021

Choose a reason for hiding this comment

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

It should be installed as completion-style, I recommed against loading it lazily. But I am not familiar with the doom infrastructure, maybe lazy loading after the first input would work? It could also be that it would not work since then the completion style is made available a little bit too late.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for context, this is the use-package! orderless section on my config:

(use-package! orderless
  :defer t
  :init
  (setq orderless-component-separator "[ &]"
        orderless-matching-styles '(orderless-prefixes
                                    orderless-initialism
                                    orderless-regexp))
  :custom
  (completion-styles '(orderless))
  :config
  (custom-set-faces!
    '('orderless-match-face-0 :background "#435300" :foreground "#b5bd68")
    '('orderless-match-face-1 :background "#4C2E60" :foreground "#b294bb")
    '('orderless-match-face-2 :background "#644E00" :foreground "#f0c674")
    '('orderless-match-face-3 :background "#1B2E47" :foreground "#81a2be"))

  (add-hook 'minibuffer-exit-hook #'orderless-remove-transient-configuration))

Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked, the orderless completion style and its corresponding completion style functions are indeed marked as autoload. Lazy loading works indeed 👍

:when (featurep! +orderless)
:config
(setq completion-styles '(orderless)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Here one may consider '(substring orderless) which ensures that TAB prefix completion works under more circumstances. Since substring matches a subset of orderless, this generally works well. Furthermore one could even set '(basic substring orderless), but in this case basic matches strictly less, so this can lead to unexpected behavior but ensures that matches with the current prefix are preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current recommended Selectrum+Orderless configuration is as follows, which is compatible with completion styles and optimizes orderless highlighting.

(setq orderless-skip-highlighting (lambda () selectrum-active-p))
(setq selectrum-highlight-candidates-function #'orderless-highlight-matches)


(use-package! consult
:defer t
:init
(fset 'multi-occur #'consult-multi-occur)
(define-key!
[remap apropos] #'consult-apropos
[remap goto-line] #'consult-goto-line
[remap imenu] #'consult-imenu
[remap switch-to-buffer] #'consult-buffer
[remap switch-to-buffer-other-window] #'consult-buffer-other-window
[remap switch-to-buffer-other-frame] #'consult-buffer-other-frame
[remap man] #'consult-man
[remap yank-pop] #'consult-yank-pop
[remap locate] #'consult-locate
[remap load-theme] #'consult-theme
[remap recentf-open-files] #'consult-recent-file)
:config
(setq consult-project-root-function #'projectile-project-root))
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use doom-project-root here?


(use-package! consult-flycheck
:when (featurep! :checkers syntax)
:after (consult flycheck))

(use-package! embark
jethrokuan marked this conversation as resolved.
Show resolved Hide resolved
:defer t
:init
(define-key!
"C-S-a" #'embark-act)
;; TODO need to figure out what keybindings to put here
Copy link
Contributor

Choose a reason for hiding this comment

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

Embark wiki has excellent suggestions. I particularly like the "Very large files and sudo edit".

(define-key! selectrum-minibuffer-map
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you adding these to selectrum-minibuffer-map? Embark is supposed to work without selectrum, so you can add these directly to the minibuffer-local-map, don't you?

"C-c C-o" #'embark-export
"C-c C-c" #'embark-act-noexit))

(use-package! marginalia
:after selectrum
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure you need to wait for selectrum to load here. I think just the hook is enough.

:hook (doom-first-input . marginalia-mode)
:init
(setq-default marginalia-annotators '(marginalia-annotators-heavy)))

(use-package! embark-consult
:after (embark consult)
:hook
(embark-collect-mode . embark-consult-preview-minor-mode))
14 changes: 14 additions & 0 deletions modules/completion/selectrum/packages.el
@@ -0,0 +1,14 @@
;; -*- no-byte-compile: t; -*-
;;; completion/selectrum/packages.el

(package! selectrum)
(when (featurep! +prescient)
(package! selectrum-prescient))
(when (featurep! +orderless)
(package! orderless))
(package! consult)
(when (featurep! :checkers syntax)
(package! consult-flycheck))
(package! embark)
(package! embark-consult)
(package! marginalia)
2 changes: 2 additions & 0 deletions modules/config/default/autoload/search.el
Expand Up @@ -12,6 +12,7 @@ If prefix ARG is set, prompt for a directory to search from."
(call-interactively
(cond ((featurep! :completion ivy) #'+ivy/project-search-from-cwd)
((featurep! :completion helm) #'+helm/project-search-from-cwd)
((featurep! :completion selectrum) #'+selectrum/project-search-from-cwd)
(#'rgrep)))))

;;;###autoload
Expand Down Expand Up @@ -47,6 +48,7 @@ If prefix ARG is set, include ignored/hidden files."
(call-interactively
(cond ((featurep! :completion ivy) #'+ivy/project-search)
((featurep! :completion helm) #'+helm/project-search)
((featurep! :completion selectrum) #'+selectrum/project-search)
(#'projectile-ripgrep)))))

Copy link
Contributor

Choose a reason for hiding this comment

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

You have to change the +default/yank-pop as well:

;;;###autoload
(defun +default/yank-pop (&rest _)
  "Interactively select what text to insert from the kill ring."
  (interactive "P")
  (call-interactively
   (cond ((fboundp 'counsel-yank-pop)    #'counsel-yank-pop)
         ((fboundp 'consult-yank-pop)    #'consult-yank-pop)
         ((fboundp 'helm-show-kill-ring) #'helm-show-kill-ring)
         ((error "No kill-ring search backend available. Enable ivy or helm!")))))

;;;###autoload
Expand Down