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.

Sign up for GitHub

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

Use internal property for directory suffix #256

Merged
merged 4 commits into from
Dec 7, 2020
Merged

Use internal property for directory suffix #256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b40ce05
Use internal property for directory suffix
clemera Dec 7, 2020
1a5222e
Rename internal property
clemera Dec 7, 2020
375fb28
Expand comment
clemera Dec 7, 2020
7daa4b5
Add changelog
clemera Dec 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ The format is based on [Keep a Changelog].
it's a list of strings. Before the list only wasn't modfied when the
function returned the alist format as specified by `selectrum-read`
([#220]).
* Annotations or usage or `selectrum-candidate-display-suffix`
property in file completions were overwritten for directories and
not displayed, which has been fixed ([#256], [#255]).

[#194]: https://github.com/raxod502/selectrum/issues/194
[#200]: https://github.com/raxod502/selectrum/pull/200
Expand All @@ -62,6 +65,8 @@ The format is based on [Keep a Changelog].
[#250]: https://github.com/raxod502/selectrum/pull/250
[#251]: https://github.com/raxod502/selectrum/pull/251
[#253]: https://github.com/raxod502/selectrum/pull/253
[#255]: https://github.com/raxod502/selectrum/issues/255
[#256]: https://github.com/raxod502/selectrum/pull/256

## 3.0 (released 2020-10-20)
### Breaking changes
Expand Down
12 changes: 10 additions & 2 deletions selectrum.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1084,6 +1084,14 @@ TABLE defaults to `minibuffer-completion-table'. PRED defaults to
(let* ((prefix (get-text-property
0 'selectrum-candidate-display-prefix
candidate))
(isuffix (get-text-property
;; Internal property to display an additional
;; suffix before the actual suffix added via
;; public API. Currently only used for
;; displaying slashes of directories in file
;; completions.
0 'selectrum--internal-candidate-display-suffix
candidate))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. This seems slightly unfortunate. It feels kind of similar to some of the things going on in Ivy/Counsel, where the public API isn't expressive enough to implement all the needed features, and so some of the Ivy functions have special cases to handle specific callers from Counsel. In an ideal world, we'd be able to support multiple suffixes natively in some way through the public API.

I don't know if that's actually achievable---maybe this is one of those cases where you just have to hack it, because the alternative is too inconvenient---but I thought I would call attention to it since I think we want to try to avoid this kind of thing suffusing the entire codebase, since then we'll end up in an Ivy/Counsel sort of situation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, hehe, I see you got rid of it in #262. That's what I get for being so behind on email!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy it could be removed because of the reasons you mentioned above, there where some refactoring issues after I switched but I hope these have all been sorted out now.

(suffix (or (get-text-property
0 'selectrum-candidate-display-suffix
candidate)
Expand All @@ -1093,7 +1101,7 @@ TABLE defaults to `minibuffer-completion-table'. PRED defaults to
candidate
'selectrum-completion-annotation))))
(displayed-candidate
(concat prefix candidate suffix))
(concat prefix candidate isuffix suffix))
(right-margin
(or (get-text-property
0 'selectrum-candidate-display-right-margin
Expand Down Expand Up @@ -1788,7 +1796,7 @@ For PROMPT, COLLECTION, PREDICATE, REQUIRE-MATCH, INITIAL-INPUT,
(setq i (substring i 0 (1- (length i))))
(put-text-property
0 (length i)
'selectrum-candidate-display-suffix
'selectrum--internal-candidate-display-suffix
"/"
i))
i)
Expand Down