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

Better fontification of file names and line numbers in ivy results #399

Closed
hmelman opened this Issue Feb 24, 2016 · 9 comments

Comments

Projects
None yet
2 participants
@hmelman

hmelman commented Feb 24, 2016

This is more a feature request. I'd like the formatting in an ivy buffer to be more consistent and more like compliation-mode or grep-mode.

counsel-grep lines begin "linenumber:" and swiper lines don't have the colon. Seems like that could be unified. Then there are other lines like from counsel-git-grep or other multifile searches that are prefixed "filename:linenumber:".

I think it would be nice if filenames were in compilation-info face and line numbers were in compilation-line-number face (which are what compilation mode uses (and grep-mode defaults to). It would be great if ivy could figure this out automatically so that new commands got this too. Also the match highlighting that happens of the typed in text, should be limited to after the file name or line number. Now if the search string is in the file name, it's highlighted as well.

In writing my own counsel-zgrep using counsel--async-command I'm not quite sure where to plug this in. I think ivy does the fontification as things are added to the minibuffer, but I don't have an easy hook into that. Following compilation-mode's model I think would be to turn on font-lock in the *counsel* buffer and have it highlighted as text is added to it, then hopefully those text properties would be preserved as they're copied into the minibuffer for display.

Just an idea.

@abo-abo

This comment has been minimized.

Show comment
Hide comment
@abo-abo

abo-abo Feb 24, 2016

Owner

Commands can customize their display by let-binding ivy-format-function. See counsel-yank-pop. If you wish, implement counsel-git-grep--format-function similar to counsel--yank-pop-format-function and open a PR.

Following compilation-mode's model I think would be to turn on font-lock in the counsel buffer and have it highlighted as text is added to it, then hopefully those text properties would be preserved as they're copied into the minibuffer for display.

This will be way too slow. Using ivy-format-function means only 10 strings will be propertized in the post-command-hook, which Emacs can do very fast. Font-locking a buffer with thousands or tens of thousands of lines on the other hand...

swiper lines don't have the colon

They don't have one because they don't need one: swiper works on a single file, the file name is implied, and the content is formatted consistently into its own column.

Owner

abo-abo commented Feb 24, 2016

Commands can customize their display by let-binding ivy-format-function. See counsel-yank-pop. If you wish, implement counsel-git-grep--format-function similar to counsel--yank-pop-format-function and open a PR.

Following compilation-mode's model I think would be to turn on font-lock in the counsel buffer and have it highlighted as text is added to it, then hopefully those text properties would be preserved as they're copied into the minibuffer for display.

This will be way too slow. Using ivy-format-function means only 10 strings will be propertized in the post-command-hook, which Emacs can do very fast. Font-locking a buffer with thousands or tens of thousands of lines on the other hand...

swiper lines don't have the colon

They don't have one because they don't need one: swiper works on a single file, the file name is implied, and the content is formatted consistently into its own column.

@hmelman

This comment has been minimized.

Show comment
Hide comment
@hmelman

hmelman Feb 25, 2016

Thanks. I figured formatting all of the " counsel" buffer would be more work than necessary.

I roughly have your suggestion working. I'm let binding ivy-format-function to a new ivy-format-grep-function which adds the face properties to the the filename and line number at the start of each line.

And now I'm going to try rewriting it. I'm finding this portion of the code hard to wrap my head around. It would really help if CAND-PAIRS was documented somewhere.

Like what's the difference between the STRING and EXTRA part? It's probably because I'm coming from counsel--async-command with just a split-string on "\n" so I was keeping everything in STRING and using a blank EXTRA, but it was hard to see when the PAIRS are created and who's responsible for recombining them. I think that Fancy formatting if enabled is only applied to STRING, but I found that out later. So I think I should be using that to avoid my problem of Fancy highlighting of the search text in the filename.

Finally I'm not sure whether the CAND-PAIRS list that's passed around to the formating commands is for all the candidates or just the subset of candidates that are being displayed in the minibuffer? It was a surprise at first when I found my ivy-format-grep-function being called (with a list of CAND-PAIRS) on every C-n I do in ivy-read.

Rather than counsel-yank-pop I think counsel-M-x is a better example here because it actually has EXTRA info in the CANDS-PAIRS. It also saves the old value of ivy-format-function and calls it on each line so that the user's option of how to display the selected line is preserved. But I think I might want ivy--format to check a variable to use to break CANDS into CAND-PAIRS that I could let bind.

It feels like the abstraction levels are a little confused in this area but I suspect I feel that way just because I'm having a hard time figuring out how all these pieces fit together.

hmelman commented Feb 25, 2016

Thanks. I figured formatting all of the " counsel" buffer would be more work than necessary.

I roughly have your suggestion working. I'm let binding ivy-format-function to a new ivy-format-grep-function which adds the face properties to the the filename and line number at the start of each line.

And now I'm going to try rewriting it. I'm finding this portion of the code hard to wrap my head around. It would really help if CAND-PAIRS was documented somewhere.

Like what's the difference between the STRING and EXTRA part? It's probably because I'm coming from counsel--async-command with just a split-string on "\n" so I was keeping everything in STRING and using a blank EXTRA, but it was hard to see when the PAIRS are created and who's responsible for recombining them. I think that Fancy formatting if enabled is only applied to STRING, but I found that out later. So I think I should be using that to avoid my problem of Fancy highlighting of the search text in the filename.

Finally I'm not sure whether the CAND-PAIRS list that's passed around to the formating commands is for all the candidates or just the subset of candidates that are being displayed in the minibuffer? It was a surprise at first when I found my ivy-format-grep-function being called (with a list of CAND-PAIRS) on every C-n I do in ivy-read.

Rather than counsel-yank-pop I think counsel-M-x is a better example here because it actually has EXTRA info in the CANDS-PAIRS. It also saves the old value of ivy-format-function and calls it on each line so that the user's option of how to display the selected line is preserved. But I think I might want ivy--format to check a variable to use to break CANDS into CAND-PAIRS that I could let bind.

It feels like the abstraction levels are a little confused in this area but I suspect I feel that way just because I'm having a hard time figuring out how all these pieces fit together.

@abo-abo

This comment has been minimized.

Show comment
Hide comment
@abo-abo

abo-abo Feb 25, 2016

Owner

Like what's the difference between the STRING and EXTRA part?

The STRING part is the actual candidate. The EXTRA part is any extra information that you might want to attach. That part is almost always empty. But something like counsel-M-x might use it to display the key bindings next to command names.

It was a surprise at first when I found my ivy-format-grep-function being called (with a list of CAND-PAIRS) on every C-n I do in ivy-read.

This is standard. There is a full re-display in the post-command hook. But only the first 10 candidates are processed.

It feels like the abstraction levels are a little confused in this area but I suspect I feel that way just because I'm having a hard time figuring out how all these pieces fit together.

Yes, it's best to not think too much about it. Once there's enough things to abstract, the abstraction layers fall into place.

Owner

abo-abo commented Feb 25, 2016

Like what's the difference between the STRING and EXTRA part?

The STRING part is the actual candidate. The EXTRA part is any extra information that you might want to attach. That part is almost always empty. But something like counsel-M-x might use it to display the key bindings next to command names.

It was a surprise at first when I found my ivy-format-grep-function being called (with a list of CAND-PAIRS) on every C-n I do in ivy-read.

This is standard. There is a full re-display in the post-command hook. But only the first 10 candidates are processed.

It feels like the abstraction levels are a little confused in this area but I suspect I feel that way just because I'm having a hard time figuring out how all these pieces fit together.

Yes, it's best to not think too much about it. Once there's enough things to abstract, the abstraction layers fall into place.

@hmelman

This comment has been minimized.

Show comment
Hide comment
@hmelman

hmelman Feb 27, 2016

As I understand it, ivy--exhibit calls ivy--format which determines which cands should be shown in the minibuffer and then formats them. ivy--format converts the candidates to be displayed into cand-pairs where each pair is (string . nil) while calling ivy--format-minibuffer-line on (the whole) string which does any fancy display formatting according to ivy-display-style. It then calls the ivy-format-function on these cand-pairs. This is a user option where the user can pick a general display style for the selected line currently with a leading arrow or with highlight on the matched candidate or whole line.

The only existing functions that make use of extra is counsel-M-x which adds key binding info (so it only has to find bindings for the 10 or so shown commands, as opposed to every command in obarray) and counsel-yank-pop which explicitly eliminates it. The existing ivy-format-function-* functions all append extra to string.

I'm writing a counsel-zgrep function which calls to zgrep using the api supplied by counsel--async-command. As a result I get a list of candidate strings where each candiate string is a line of grep output. We agree that formatting this list is too computationally expensive and it's better to format this a chunk at a time as displayed in the minibuffer. The question is where to hook things in.

If my way of formatting is let binding an ivy-format-function then I think I have two issues. First is any fancy display highlighting has already been applied to the whole string, even though I only want it applied to part of it. Second, even if it wasn't too late to make use of extra for the file:line: info I didn't want fancy highlighted, all the existing ivy-format-function-* functions all append extra to string and I want it prepended (and I have to call them because it's a user option for selected line display style)

This all works for counsel-M-x by calling the user's chosen ivy-format-function on a transformation of the cand-pairs which is fine having the entire existing string fancy highlighted and adds new key binding info to extra assuming it will be appended to string by the formatting function.

So my attempt at fixing this was as follows. At the bottom of ivy--format I create the candidate pairs, call a transform function (held in a variable that defaults to 'identity) and then call ivy--format-minibuffer-line on the cars of the cand-pairs and then call ivy-format-function on them.

(defvar ivy-transform-cand-pairs 'identity)

(let* ((ivy--index index)
         (pairs1 (mapcar (lambda (cand) (cons cand nil)) cands))
         (pairs2 (mapcar ivy-transform-cand-pairs pairs1))
         (cand-pairs (mapcar (lambda (pair) 
                                              (cons (ivy--format-minibuffer-line (car pair)) (cdr pair)))
                                           pairs2))
         (res (concat "\n" (funcall ivy-format-function cand-pairs))))
    (put-text-property 0 (length res) 'read-only nil res)
    res))))

This is helpful, but the existing ivy-format-functions assume that extra should be appended to string.

I'm still thinking through what a better api for this would be, but I thought I'd get this out there. I definitely think the user option of how to format the selected line needs to be completely separate from the rest of the formatting. I also think the conversion between strings and cand-pairs needs to be more accessible to the api.

hmelman commented Feb 27, 2016

As I understand it, ivy--exhibit calls ivy--format which determines which cands should be shown in the minibuffer and then formats them. ivy--format converts the candidates to be displayed into cand-pairs where each pair is (string . nil) while calling ivy--format-minibuffer-line on (the whole) string which does any fancy display formatting according to ivy-display-style. It then calls the ivy-format-function on these cand-pairs. This is a user option where the user can pick a general display style for the selected line currently with a leading arrow or with highlight on the matched candidate or whole line.

The only existing functions that make use of extra is counsel-M-x which adds key binding info (so it only has to find bindings for the 10 or so shown commands, as opposed to every command in obarray) and counsel-yank-pop which explicitly eliminates it. The existing ivy-format-function-* functions all append extra to string.

I'm writing a counsel-zgrep function which calls to zgrep using the api supplied by counsel--async-command. As a result I get a list of candidate strings where each candiate string is a line of grep output. We agree that formatting this list is too computationally expensive and it's better to format this a chunk at a time as displayed in the minibuffer. The question is where to hook things in.

If my way of formatting is let binding an ivy-format-function then I think I have two issues. First is any fancy display highlighting has already been applied to the whole string, even though I only want it applied to part of it. Second, even if it wasn't too late to make use of extra for the file:line: info I didn't want fancy highlighted, all the existing ivy-format-function-* functions all append extra to string and I want it prepended (and I have to call them because it's a user option for selected line display style)

This all works for counsel-M-x by calling the user's chosen ivy-format-function on a transformation of the cand-pairs which is fine having the entire existing string fancy highlighted and adds new key binding info to extra assuming it will be appended to string by the formatting function.

So my attempt at fixing this was as follows. At the bottom of ivy--format I create the candidate pairs, call a transform function (held in a variable that defaults to 'identity) and then call ivy--format-minibuffer-line on the cars of the cand-pairs and then call ivy-format-function on them.

(defvar ivy-transform-cand-pairs 'identity)

(let* ((ivy--index index)
         (pairs1 (mapcar (lambda (cand) (cons cand nil)) cands))
         (pairs2 (mapcar ivy-transform-cand-pairs pairs1))
         (cand-pairs (mapcar (lambda (pair) 
                                              (cons (ivy--format-minibuffer-line (car pair)) (cdr pair)))
                                           pairs2))
         (res (concat "\n" (funcall ivy-format-function cand-pairs))))
    (put-text-property 0 (length res) 'read-only nil res)
    res))))

This is helpful, but the existing ivy-format-functions assume that extra should be appended to string.

I'm still thinking through what a better api for this would be, but I thought I'd get this out there. I definitely think the user option of how to format the selected line needs to be completely separate from the rest of the formatting. I also think the conversion between strings and cand-pairs needs to be more accessible to the api.

abo-abo added a commit that referenced this issue Feb 27, 2016

New API function ivy-set-display-transformer
* ivy.el (ivy--display-transformers-list): New defvar.
(ivy-set-display-transformer): New defun. Keys are :caller, values are
str->str lambda.
(ivy-state): New field display-transformer-fn.
(ivy-read): Set :display-transformer-fn.
(ivy--format): Apply :display-transformer-fn to each displayed
candidate, in the context of ivy-window.

* counsel.el (ivy-set-display-transformer): Set for `counsel-M-x'.
(counsel-M-x-transformer): Promote from `counsel--M-x-transformer'.
Now takes a string instead of a cons cell.
(counsel-M-x): No more messing with `ivy-format-function'.

Re #399

abo-abo added a commit that referenced this issue Feb 27, 2016

Simplify the ivy-format-function interface
* ivy.el (ivy--format-function-generic): Third arg is no longer a list
  of cons cells, but a list of strings instead.
(ivy-format-function-default):
(ivy-format-function-arrow):
(ivy-format-function-line): Take a string instead of a cons cell.
(ivy--format): Feed strings instead of cons cells to
`ivy-format-function'.

* ivy-test.el (ivy--format): Update test.

* counsel.el (counsel--yank-pop-format-function): Take a string instead
  of a cons cell.

Re #399

@abo-abo abo-abo closed this in 587526e Feb 27, 2016

@abo-abo

This comment has been minimized.

Show comment
Hide comment
@abo-abo

abo-abo Feb 27, 2016

Owner

Have a look, it's a lot simpler now.

Owner

abo-abo commented Feb 27, 2016

Have a look, it's a lot simpler now.

@hmelman

This comment has been minimized.

Show comment
Hide comment
@hmelman

hmelman Feb 27, 2016

Thanks for looking at this. This commit seems to have broken things. On an Emacs -Q, if I enable ivy-mode, then counsel-find-file (and most other things) fail with wrong number of arguments. I'm trying to track it down but figured I'd report it even without details.

hmelman commented Feb 27, 2016

Thanks for looking at this. This commit seems to have broken things. On an Emacs -Q, if I enable ivy-mode, then counsel-find-file (and most other things) fail with wrong number of arguments. I'm trying to track it down but figured I'd report it even without details.

@hmelman

This comment has been minimized.

Show comment
Hide comment
@hmelman

hmelman Feb 27, 2016

Ok, in ivy-format-function-line you didn't remove the arguments extra in the lambdas. See #406.

hmelman commented Feb 27, 2016

Ok, in ivy-format-function-line you didn't remove the arguments extra in the lambdas. See #406.

@hmelman

This comment has been minimized.

Show comment
Hide comment
@hmelman

hmelman Feb 27, 2016

Thanks for all the consideration of my ramblings, I really appreciate it. I think what you've done definitely improves things. Some suggestions:

counsel--yank-pop-format-function should have it's arg renamed from cand-pairs to cands

The doc strings in the ivy-format-function-*s should change CAND-PAIRS to CANDS

I think ivy-read should get a :display-transformer-fn keyword

Both counsel-git-grep-action and counsel-git-grep-transformer have regexps to match the file name and line number information at the start of a line, but they're different. For what it's worth, grep-regexp-alist in grep.el uses the following to try to match filenames with a colon "^(.+?)(:[ \t])([1-9][0-9])\2"

I still have the problem of my entire zgrep lines getting fancy displayed (including the file names), I'll keep thinking on that.

hmelman commented Feb 27, 2016

Thanks for all the consideration of my ramblings, I really appreciate it. I think what you've done definitely improves things. Some suggestions:

counsel--yank-pop-format-function should have it's arg renamed from cand-pairs to cands

The doc strings in the ivy-format-function-*s should change CAND-PAIRS to CANDS

I think ivy-read should get a :display-transformer-fn keyword

Both counsel-git-grep-action and counsel-git-grep-transformer have regexps to match the file name and line number information at the start of a line, but they're different. For what it's worth, grep-regexp-alist in grep.el uses the following to try to match filenames with a colon "^(.+?)(:[ \t])([1-9][0-9])\2"

I still have the problem of my entire zgrep lines getting fancy displayed (including the file names), I'll keep thinking on that.

@hmelman

This comment has been minimized.

Show comment
Hide comment
@hmelman

hmelman Feb 27, 2016

Is the only thing that ivy--format-minibuffer-line does to add fontification for fancy display? (I'm not sure about the first cond condition). If so maybe it could have a more precise name (or at least a docstring) :).

Could it get another argument, say a regexp or function, to specify the region of the STR argument to be highlighted?

hmelman commented Feb 27, 2016

Is the only thing that ivy--format-minibuffer-line does to add fontification for fancy display? (I'm not sure about the first cond condition). If so maybe it could have a more precise name (or at least a docstring) :).

Could it get another argument, say a regexp or function, to specify the region of the STR argument to be highlighted?

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