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

Incorrect completion case when read-file-name-completion-ignore-case is on #341

Closed
vifon opened this issue Mar 12, 2023 · 6 comments
Closed

Comments

@vifon
Copy link

vifon commented Mar 12, 2023

When read-file-name-completion-ignore-case is set to t and ~/ is used to anchor the path at the home from some other directory, accepting a completion candidate from among the ~/* files doesn't correct the case before returning it.

I've prepared a minimal example, with what I've found so far in the comments:

#!/usr/bin/env -S emacs -Q -l
;; Test scenario:
;;   - Ensure the ~/Documents/ directory exists.
;;   - Ensure there are no other files starting with ~/Docu or ~/docu.
;;   - Load this file from some subdirectory of ~/, not ~/ directly.
;;     - When running from ~/, no error occurs.
;;
;; Expected:
;;   Browse ~/Documents with `dired'.
;;
;; Actual:
;;   > Setting current directory: No such file or directory, ~/documents/

(setq read-file-name-completion-ignore-case t)

(package-install 'vertico)
(package-initialize)

;; With Vertico turned off, ~/Documents gets opened as expected.
(vertico-mode 1)

;; Simulate keypresses for easy reproduction.  Can be done manually, of course.
(execute-kbd-macro "\C-xd~/docu\C-i\C-m")

;; Further notes:
;;   - Only happens for ~/*, but not for deeper paths,
;;     such as ~/Documents/sOmEtHinG (this gets completed correctly).
;;     - Happens for both files and directories in ~/*
;;   - Does not happen for /*, tested on /TmP (gets completed as /tmp).
;;   - Does work correctly when executed from /tmp, which would imply
;;     only subdirectories of ~/ are affected (as cwd) for some reason.
@minad
Copy link
Owner

minad commented Mar 12, 2023

Thanks for the detailed report. This is an upstream bug, since invalid candidates are generated. Can you please report it? These are the steps needed:

  1. Create directory ~/Documents.
  2. emacs -Q
  3. (setq read-file-name-completion-ignore-case t)
  4. Open some directory ~/foo/.
  5. Press C-x d
  6. Enter ~/doc, the minibuffer content will now be ~/foo/~/doc
  7. Press ?
  8. Click on invalid candidate foo/~/documents.
  9. Press RET in the minibuffer, we will open a new file documents which is undesired.

Note that the problem is not pronounced in Vertico if you delete shadowed paths:

(add-hook 'rfn-eshadow-update-overlay-hook #'vertico-directory-tidy)

Generally, before reporting an issue for Vertico, please check if default completion is affected too.

@minad minad closed this as completed Mar 12, 2023
@vifon
Copy link
Author

vifon commented Mar 12, 2023

Like I mentioned, if I don't have Vertico enabled, the candidate is correctly completed with its case corrected.

@minad
Copy link
Owner

minad commented Mar 12, 2023

Like I mentioned, if I don't have Vertico enabled, the candidate is correctly completed with its case corrected.

Yes, but that's irrelevant (To be fair, you did test without Vertico, but your test wasn't precise enough to also expose the bug). If you press TAB in default completion, minibuffer-complete "fixes" the underlying issue. The problem here is that completion-all-completions produces invalid candidates, which it must not do by definition. Try the recipe I've given for default completion.

Iirc this issue got reported before in some slightly different scenario. Maybe search through the issue tracker.

@minad
Copy link
Owner

minad commented Mar 12, 2023

@vifon
Copy link
Author

vifon commented Mar 12, 2023

Oh, you're absolutely right, when I use ? for the completion instead of TAB, the problem indeed occurs. Thank you for clarifying! I'll check if it still happens in the fresh Emacs checkout, and then report. EDIT: I see your previous report should already cover it.

I think I actually prefer the behavior with the shadowed paths being deleted, thank you. I see it now that it's mentioned in the readme too.

@minad
Copy link
Owner

minad commented Mar 12, 2023

I think I actually prefer the behavior with the shadowed paths being deleted, thank you. I see it now that it's mentioned in the readme too.

Yes, vertico-directory-tidy should probably be the default behavior (and I also recommend that one uses that). It wasn't the best decision when I removed this from vertico.el and moved it to vertico-directory.el. However I also don't want to hide vanilla features in Vertico. I thought that path shadowing could be useful. Furthermore by not deleting shadowed paths automatically, bugs like these are exposed and can be reported upstream. I think it is crucial that the base infrastructure gets fixed.

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

2 participants