ivy-completion-in-region: correctly calculate the length of replacement for file category #3051
Conversation
kiennq
force-pushed
the
fix/completion-prefix-len
branch
from
7b6a90e to
4f906b7
Compare
…nt for file category
kiennq
force-pushed
the
fix/completion-prefix-len
branch
from
4f906b7 to
2ab47b0
Compare
|
Thanks! I'll try to have a closer look at this soon. In the meantime, I'm curious: does this relate at all to the issues described in #1755? |
Yes, it should address that issue. The second item is actually used to fix that.
|
Sorry, life got in the way again. |
| (t | ||
| (substring str (- len)))))) | ||
| (substring target-str (- len)))))) |
There was a problem hiding this comment.
Do you have any examples where this branch is taken and is useful?
I'm struggling to trigger it in cases other than when len = str-len, where the substring is a no-op.
There was a problem hiding this comment.
The reason I ask is because I don't understand why we would be interested in counting len characters from the end of target-str, when len represents an index from the start of (car comps).
(This is unrelated to your patch BTW - I'm trying to understand how initial is calculated in general.)
There was a problem hiding this comment.
Ah, I think it's for cases like completing ./foo where the first candidate is foobar. foobar presumably has a matching prefix of foo, so the last three characters of ./foo are deleted and replaced with the entire candidate foobar.
Even so, it strikes me as odd to take a prefix length of one string and assume it corresponds to a suffix length of a potentially unrelated string.
Perhaps we should only remove the suffix of str when it's equal to the common prefix of (car comps)?
Still not sure if this branch is taken outside of file name completion.
| (defun ivy--completion-prefix-offset (str md) | ||
| "Return the offset of the completion prefix in STR with its metadata MD." | ||
| (pcase (cdr (assoc 'category md)) | ||
| (`file (length (file-name-directory str))) | ||
| (_ 0))) |
There was a problem hiding this comment.
I imagine this doesn't handle completing e.g. ~/a/b/c to ~/aaa/bbb/ccc, right?
I think we can avoid the need for this function by instead taking advantage of the base-size already returned by completion-all-completions; stay tuned.
Thanks to Kien Nguyen <[email protected]> and Madhu <[email protected]> for suggestions. Fixes parts of #1361, #1755, #2879, #3051, #3056, and https://bugs.gnu.org/76440. Relates to https://bugs.gnu.org/71419. * ivy.el (ivy--face-list-p): New compatibility shim. (ivy-completion-common-length): Use it to handle both atom and list values for the face property, since it varies across Emacs versions. Use previous-single-property-change instead of iterating by char. Improve documentation. (ivy-completion-in-region): Translate a 'not found' result of ivy-completion-common-length into a first difference at index 0, not at the end of the string. That means, interpret a lack of completions-first-difference as no commonality. This is more consistent with both the definition of ivy-completion-common-length and completion changes in Emacs 30. * ivy-test.el (ivy-completion-common-length): Extend tests.
The latter is very problematic: it is specific to a single candidate, does not necessarily correspond to the string being completed, complicates bounds calculations, relies on only partially documented presentation details rather than the usual API, etc. When completion-all-completions returns a base-size (which in practice seems to be most if not all the time), it corresponds more closely to the substring that is being completed and will be replaced. This should address most of the remaining issues in #1361, #1755, * ivy.el (ivy-completion-common-length): Discourage use. (ivy-completion-in-region): When completion-all-completions returns a base-size, prefer that over the completions-first-difference face to determine the completion boundaries and initial-input. Fixes #1755. Fixes #2879. Closes #3051. Fixes #3056.
The latter is very problematic: it is specific to a single candidate, does not necessarily correspond to the string being completed, complicates bounds calculations, relies on only partially documented presentation details rather than the usual API, etc. When completion-all-completions returns a base-size (which in practice seems to be most if not all the time), it corresponds more closely to the substring that is being completed and will be replaced. This should address most of the remaining issues in #1361, #1755, * ivy.el (ivy-completion-common-length): Discourage use. (ivy-completion-in-region): When completion-all-completions returns a base-size, prefer that over the completions-first-difference face to determine the completion boundaries and initial-input. Fixes #1755. Fixes #2879. Closes #3051. Fixes #3056.
|
Thanks again for this PR, and apologies for the long delay in reviewing it. I think PR #3065 takes a slightly more general approach, which doesn't need to determine whether we are completing a I welcome everyone to try that PR and report back. TIA. |
The latter is very problematic: it is specific to a single candidate, does not necessarily correspond to the string being completed, complicates bounds calculations, relies on only partially documented presentation details rather than the usual API, etc. When completion-all-completions returns a base-size (which in practice seems to be most if not all the time), it corresponds more closely to the substring that is being completed and will be replaced. This should address most of the remaining issues in #1361, #1755, * ivy.el (ivy-completion-common-length): Discourage use. (ivy-completion-in-region): When completion-all-completions returns a base-size, prefer that over the completions-first-difference face to determine the completion boundaries and initial-input. Fixes #1755. Fixes #2879. Closes #3051. Fixes #3056.
The latter is very problematic: it is specific to a single candidate, does not necessarily correspond to the string being completed, complicates bounds calculations, relies on only partially documented presentation details rather than the usual API, etc. When completion-all-completions returns a base-size (which in practice seems to be most if not all the time), it corresponds more closely to the substring that is being completed and will be replaced. This should address most of the remaining issues in reports #1361, #1755, #2879, #3051, #3056, and https://bugs.gnu.org/76440. * ivy.el (ivy-completion-common-length): Discourage use. (ivy-completion-in-region): When completion-all-completions returns a base-size, prefer that over the completions-first-difference face to determine the completion boundaries and initial-input. Fixes #1755. Fixes #2879. Closes #3051. Fixes #3056.
completions-first-difference, rather than a list of them.filecategory completions, calculate the prefix offset from the file name directory, not the entire input string. This adjustment is necessary because fuzzy completion styles can lead to incorrect common length values. The prefix offset ensures that we replace only the relevant portion of the input when calculating completion items.