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

Expand file-name of consult-find target? #704

Open
localauthor opened this issue Mar 7, 2024 · 6 comments
Open

Expand file-name of consult-find target? #704

localauthor opened this issue Mar 7, 2024 · 6 comments

Comments

@localauthor
Copy link
Contributor

localauthor commented Mar 7, 2024

The embark target returned when using consult-find is a partial file-path. In the example below, the target is the string "subdir1/subdir2/file1.org".

Screenshot 2024-03-07 at 10 12 01

Screenshot 2024-03-07 at 10 12 23

This is a problem when using actions that do not include expand-file-name, such as embark-save-relative-path and embark-insert-relative-path. Also, when using embark-insert, only the target string is inserted, where the full file path might be expected (or desired, as in my case ;) ).

Would it be warranted to expand the target to the full file name?

@oantolin
Copy link
Owner

oantolin commented Mar 7, 2024

Huh, I thought we already did use full paths, but I guess that's just because read-file-name candidates are full paths, not because we were taking any steps to ensure it in Embark. It certainly seems suboptimal to have insert and insert-relative do the same thing...

@localauthor
Copy link
Contributor Author

localauthor commented Mar 8, 2024

Update: the problem seems specific to Vertico.

When using the default completion framework, the above-named functions work as expected, because embark-target-guess-file-at-point finds the full file-path in the Completions buffer. So embark-insert inserts the full file path.

[edit: on further investigation, the proposed solution was /not/ advisable ;)]

@localauthor
Copy link
Contributor Author

Revisiting this. The issue can be resolved by let-binding vertico--base to the default-directory whenever that variable is an empty string. (This is the case when using consult-find and consult-fd, for example.)

It's a bit convoluted, and a fair bit of code duplication, but these seem to do the trick:

  (defun embark--vertico-selected ()
    "Target the currently selected item in Vertico.
Return the category metadatum as the type of the target."
    (when vertico--input
      (if-let* ((orig-base vertico--base)
                (category (completion-metadata-get (embark--metadata) 'category))
                (fn (lambda ()
                      ;; Force candidate computation
                      (vertico--update)
                      (cons category (vertico--candidate))))
                ((equal vertico--base ""))
                ((eq 'file category))
                (vertico--base
                 (abbreviate-file-name default-directory)))
          (funcall fn)
        (setq vertico--base orig-base)
        (funcall fn))))

  (defun embark--vertico-candidates ()
    "Target the currently selected item in Vertico.
Return the category metadatum as the type of the target."
    (when vertico--input
      (if-let* ((orig-base vertico--base)
                (category (completion-metadata-get (embark--metadata) 'category))
                (fn (lambda ()
                      ;; Force candidate computation
                      (vertico--update)
                      (cons category vertico--candidates)))
                ((equal vertico--base ""))
                ((eq 'file category))
                (vertico--base
                 (abbreviate-file-name default-directory)))
          (funcall fn)
        (setq vertico--base orig-base)
        (funcall fn))))

I couldn't think of any reason for patching this in Vertico or Consult instead, but maybe @minad would want to weigh in.

@minad
Copy link
Contributor

minad commented May 7, 2024

@localauthor It doesn't look right to do this in the Vertico-specific functions. Also this looks way to complicated/hackish, and I hope we find a better way. Such transformations should better happen in some kind of transformer. But admittedly, I haven't fully understood the problem.

@localauthor
Copy link
Contributor Author

I'll try to restate the problem.

The functions consult-find and consult-fd return candidates that are partial file paths with the category file.

When using Vertico and calling embark-act on such a candidate, the target is identified as a file, but the target is only a partial file path. That is, the target is not expanded into a full file path.

For many actions in embark-file-map, the targeted file is expanded into a full path after the fact by an expand-file-name somewhere along the line. But not all actions include this expansion.

A more compact solution would be to add a blanket expand-file-path to the file transformer that is already in embark-transformer-alist, like so:

(defun embark--simplify-path (_type target)
  "Simplify and '//' or '~/' in the TARGET file path."
  (cons 'file (abbreviate-file-name
                (expand-file-name
                    (substitute-in-file-name target))))

@oantolin
Copy link
Owner

oantolin commented May 7, 2024

I like that idea. I already have that abbreviate-file-name & expand-file-name combo sprinkled all over the embark source code. I think putting in the transformer might actually allow me to remove a bunch of other places. I'll evaluate that possible change.

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

No branches or pull requests

3 participants