Treat popup value property when completing candidate #185

Open
wants to merge 10 commits into
from

Projects

None yet

4 participants

@tkf
Emacs Auto-Complete member

To manually test, execute this:

(defun actest-value-candidates ()
  ;; (name . value) -- see `ac-candidates-1'
  '(("foofoo" . "foo")
    ("foobar" . "bar")))

(ac-define-source actest-keywords
  '((candidates . (actest-value-candidates))))

(progn
  (pop-to-buffer (get-buffer-create "*actest*"))
  (auto-complete-mode +1)
  (erase-buffer)
  (insert "fo")
  (run-with-timer 0.1 nil #'auto-complete '(ac-source-actest-keywords)))

fixes #148

@tkf
Emacs Auto-Complete member

Wait sec, I am not sure about the last commit tkf@06acf91

@tkf
Emacs Auto-Complete member

I think this PR is ready for merge. But I want @m2ym to review it as it is a little bit large change. My concern is that the change in ac-candidates-1 (tkf@e256d26) might slows down completion as it creates a list as long as candidates every time. We can avoid this by adding "I don't use popup-item-value" flag as source attribute. Probably we should make it default.

@tjarvstrand, @jixiuf, as your issue reports (#168, #148) are fixed by this PR, feel free to try/review this.

@m2ym
Emacs Auto-Complete member

Why you use popup-item-value-or-self here? The item value can be any type of object. Maybe we should convert the item value into string for safe.

Emacs Auto-Complete member

Fixed in 6d6a8e2

@m2ym
Emacs Auto-Complete member

Matching againt the candidate values seems to be wrong, to me. Isearch will be inconsistent for example.

Emacs Auto-Complete member

This change is necessary to make the failing tests in the previous change set (85eddcf) pass. What you said is true only if the popup item is prefix of popup item value property (This is not the case in the previous change set). Otherwise, AC fails to match candidates.

@tjarvstrand

Any progress on this? I'm sorry I haven't had the time to try this out, but I'll try to get around to it soon. :)

@syohex syohex added a commit that referenced this pull request Jan 29, 2014
@syohex syohex Treat popup value property when completing candidate
This is same as #185
96b9908
@syohex

I have merged this patch. Please report us if there are any problems.

@syohex syohex closed this Feb 2, 2014
@syohex syohex reopened this Feb 8, 2014
@syohex

Sorry I revert this changes. This patch breaks auto-complete source whose candidates type
is cons such as ac-semantic-candidates. Because cdr value is set as value property
in candidate string.

This changes should apply only to popup-item object. But it is only a propertized string.
so we can't make a distinction between popup-item and normal propertized string.
Because popup-item may not have its specific property, such as popup-face.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment