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

Ivy completion-in-region not working correctly (I think?) #1361

Closed
dieggsy opened this issue Dec 8, 2017 · 20 comments

Comments

@dieggsy
Copy link
Contributor

commented Dec 8, 2017

If I drop down to eval-expression with M-: and type e.g. (reg-quo TAB, I get (reg-regexp-quote, instead of the expected regexp-quote (which works just fine with emacs -q).

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 9, 2017

Can't reproduce with make plain on Emacs 26.

@dieggsy

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2017

Huh, I should mention I'm using emacs built from really recent git (specifically emacs-mirror/emacs@7367ea4), and can still reproduce with make plain. Guess you can close as unsupported, if you wish?

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 9, 2017

I can't reproduce it from make plain with Emacs <= 25, but I can with 27 (can't test 26 at the moment). Ivy doesn't (and shouldn't) specify a maximum supported version, so this issue should remain open until future versions of Emacs are supported.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 9, 2017

So was completing-read-function API broken between versions 26 and 27? I'll try to bisect.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 9, 2017

Reproducible for Emacs 27, commit 0ffd3dbce76c1a967522dbe9ec6f2dffe94ee886.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 9, 2017

I don't think it's because of an API change, but I have a feeling it may be related to the completion-common-part face being applied differently across Emacs versions by completion-all-completions.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 9, 2017

I think applying completion-common-part is part of the API.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 9, 2017

I think applying completions-common-part is part of the API.

I disagree:

  1. completions-common-part is completely undocumented in the Elisp manual.
  2. completions-common-part pertains to fontification, whereas we're talking about completion.
@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 9, 2017

Relying on a face for completion is unsound, especially given that Emacs offers multiple completion-styles which change from version to version. Having said that, I'm not 100% certain this face is to blame, but it sounds like it could be related.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 9, 2017

(defface completions-common-part '((t nil))
  "Face for the common prefix substring in completions.
The idea of this face is that you can use it to make the common parts
less visible than normal, so that the differing parts are emphasized
by contrast.
See also the face `completions-first-difference'.")

I think this face is defined not only for aesthetic reasons, but also as an output parameter of completion-all-completions. If we can't use completion-all-completions or a suitable replacement, we would have to implement a fork of minibuffer.el with the equivalent functionality. I'd like to avoid doing that.

If you can think of some better way of implementing ivy-completion-common-length, I'd like to hear it. I'm not happy with looking at face properties, but I haven't seen anything better.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 9, 2017

Found the commit: 325ef57b0e3977f9509f1049c826999e8b7c226d. It's possible to recover in ivy-completion-common-length since the info returned by completion-all-completions still has the info we need, only in a different format.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 9, 2017

I'm not happy with looking at face properties, but I haven't seen anything better.

I appreciate that, and there's no obvious alternative as far as I know. I need to familiarise myself with Ivy and Emacs in-buffer completion before looking into this, but at least it's not highest priority given Ivy still works for all released Emacs versions.

@dieggsy

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2017

at least it's not highest priority given Ivy still works for all released Emacs versions.

agreed, for what it's worth :)

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 9, 2017

It's possible to recover in ivy-completion-common-length since the info returned by completion-all-completions still has the info we need, only in a different format.

Great. I just have a quick suggestion for a simplification in ivy-completion-common-length while we're at it: let-binding char-property-alias-alist to '((face font-lock-face)) as font-lock-mode does means you only need to query for the face property, which will automatically be translated to font-lock-face if applicable. In other words, the following simplification is possible in the current version of ivy-completion-common-length:

diff --git a/ivy.el b/ivy.el
index 3538b92..2b4ea10 100644
--- a/ivy.el
+++ b/ivy.el
@@ -2029,19 +2029,14 @@ ivy-completion-common-length
   "Return the length of the first `completions-common-part' face in STR."
   (let ((pos 0)
         (len (length str))
-        face-sym)
+        (char-property-alias-alist '((face font-lock-face))))
     (while (and (<= pos len)
-                (let ((prop (or (prog1 (get-text-property
-                                        pos 'face str)
-                                  (setq face-sym 'face))
-                                (prog1 (get-text-property
-                                        pos 'font-lock-face str)
-                                  (setq face-sym 'font-lock-face)))))
+                (let ((prop (get-text-property pos 'face str)))
                   (not (eq 'completions-common-part
-                           (if (listp prop) (car prop) prop)))))
+                           (or (car-safe prop) prop)))))
       (setq pos (1+ pos)))
     (if (< pos len)
-        (or (next-single-property-change pos face-sym str) len)
+        (or (next-single-property-change pos 'face str) len)
       0)))
 
 (defun ivy-completion-in-region (start end collection &optional predicate)
abo-abo added a commit that referenced this issue Dec 9, 2017
ivy.el (ivy-completion-common-length): Simplify
Simplify the code by returning the position of the text property.
This will likely be `completions-first-difference'.

Re #1361
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 9, 2017

This fixes it for the versions that I have, as far as I can test. The approach might be too simplistic and fail elsewhere. Please test.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 9, 2017

Works for all my Emacs versions, thanks!

@dieggsy

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2017

@abo-abo Thanks, that seemed to fix the original issue, though a couple more notes:

If I type something like (s-c-t-s TAB, the completion is started with s-c-t-s as initial input, which isn't great. Those are the first letters of every word in the symbol, not the symbol prefix, so no completions are displayed. If I manually delete the initial input, I get the correct candidates (among which is shell-command-to-string, for example).

Not sure if this was introduced by the most recent change, but if in eshell I type for example (asuming some filepath parent-dir/child-dir/: cd parent-dir/ TAB or cd parent-dir/child- TAB, the trailing / is removed from parent-dir/, meaning I get cd parent-dirchild-dir rather than cd parent-dir/child-dir

@abo-abo abo-abo closed this in 3bd5f3b Dec 9, 2017

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 9, 2017

@dieggsy Thanks, your first question should now be solved.

Please open a new issue with detailed reproduction for the second item, I don't use eshell.

abo-abo added a commit that referenced this issue Dec 9, 2017
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 9, 2017

@dieggsy I just reproduced ./linux expanded to .linux in *shell*. Should be fixed now. Please check also in eshell.

@dieggsy

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2017

@abo-abo Everything seems to be working fine now, thanks!

abo-abo added a commit that referenced this issue Dec 10, 2017
ivy.el (ivy-completion-common-length): Work around inconsistency
`previous-property-change' behaves strangely, as shown by the added test.

Re #1361
abo-abo added a commit that referenced this issue Dec 10, 2017
ivy.el (ivy-completion-common-length): Try a different approach
* ivy-test.el (ivy-completion-common-length): Add test.

Re #1361
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.