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

Text properties should be kept for default value #180

Closed
JuanGarcia345 opened this issue Aug 18, 2020 · 5 comments
Closed

Text properties should be kept for default value #180

JuanGarcia345 opened this issue Aug 18, 2020 · 5 comments

Comments

@JuanGarcia345
Copy link

Hello!

First of all, thank you for this nice piece of software!

While using xref and lsp-mode, the default value passed to completing-read has properties that should be kept, otherwise code navigation may be broken (this is the case for C++ with clangd).

In fact, keeping properties of default value seems to be the default behaviour of completing-read, for the sake of completeness:

(selectrum-mode -1)

(setq
 vanilla-output
 (completing-read "Test (hit RET to choose default): "
                  '(("foo" . 1) ("bar" . 2))
                  nil nil nil nil
                  #("bar" 0 3 (face font-lock-function-name-face fontified t identifier-at-point t))))

(selectrum-mode +1)

(setq
 selectrum-output
 (completing-read "Test (hit RET to choose default): "
                  '(("foo" . 1) ("bar" . 2))
                  nil nil nil nil
                  #("bar" 0 3 (face font-lock-function-name-face fontified t identifier-at-point t))))

(message "%s" (text-properties-at 1 vanilla-output))
(message "%s" (text-properties-at 1 selectrum-output))

This may be related to #107 and #108.

@clemera
Copy link
Collaborator

clemera commented Aug 18, 2020

Thanks for reporting. So for usual candidates returning them with text props will trigger errors and for the default you trigger errors if you strip them 🤦. The default also gets returned unchanged for completing-read, completing-read-multiple, read-buffer and read-file-name. To simplify this I think we can handle this within selectrum-read: If minibuffer-completion-table is non-nil we know it's a built-in completion function and we can strip the text properties unless the default-candidate was chosen then we return the default as it was passed.

@JuanGarcia345
Copy link
Author

Well that seems pretty right! Anyway one may say that the read family is not that much POLA compliant 😅

@clemera
Copy link
Collaborator

clemera commented Sep 9, 2020

Should be fixed by #198?

@clemera clemera added the waiting on response Needs more info or follow-up, will be closed after 90 days if no response label Sep 9, 2020
@raxod502 raxod502 closed this as completed Dec 8, 2020
@raxod502
Copy link
Member

raxod502 commented Dec 8, 2020

This thread is being closed automatically by Tidier because it is labeled with "waiting on response" and has not seen any activity for 90 days. But don't worry—if you have any information that might advance the discussion, leave a comment and I will be happy to reopen the thread :)

@JuanGarcia345
Copy link
Author

Sorry about the non-response on my side, #198 fixes the issue. Really nice work! and thank you both @raxod502 @clemera

@raxod502 raxod502 removed the waiting on response Needs more info or follow-up, will be closed after 90 days if no response label Dec 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants