Open nverno opened 4 years ago
After some experimentation, the following modifications let xterm-color
handle interpretation of the escape codes in the rg-mode
buffer, which allows interpretation of --color
flags and settings in ~/.ripgreprc.
;; Override rg's `compilation-error-regexp-alist' matching
;; to allow use with `xterm-color-filter'
(defconst rg-file-column-pattern-group
"^\\([0-9]+\\):\\([0-9]+\\):")
(defconst rg-grouped-file-regex
"^\\(?:File:\\s-*\\)?\\([^ ].*\\)$")
(defun rg-match-grouped-filename-xc ()
"Match grouped filename in compilation output."
(save-match-data
(save-excursion
(beginning-of-line)
(while (and (not (bobp))
(looking-at-p rg-file-column-pattern-group))
(forward-line -1))
(and (looking-at rg-grouped-file-regex)
(list (match-string 1))))))
(defalias 'rg-filter #'ignore) ;; no need for `rg-filter`
(defalias 'rg-match-grouped-filename 'rg-match-grouped-filename-xc)
(defun rg-use-xterm-color ()
;; (setq-local compilation-transform-file-match-alist nil)
(push 'rg-xterm-group compilation-error-regexp-alist)
(push (cons 'rg-xterm-group
(list rg-file-column-pattern-group
'rg-match-grouped-filename 1 2))
compilation-error-regexp-alist-alist))
(add-hook 'rg-mode-hook #'rg-use-xterm-color)
Thanks for the report.
- If custom colors are defined in ~/.ripgreprc, eg. --colors=path:fg:cyan etc. then font-locking and other functions in the rg-results buffer don't work.
This should work on master. Which version did you test on?
- Using xterm-color-filter also breaks rg's compilation (from xterm-color library)
I don't know how you inject xterm-color into compilation-mode but it should work with something like (add-hook 'compilation-filter-hook 'xterm-color-filter-function 'append 'local)
to allow this package to override it.
Regarding your second modification, note that some features in the results buffer will not work, like some navigation, alignment of line and color numbers, hiding of color numbers etc. I guess this could be supported in the package by making the filtering configurable, applying the necessary modifications while preserving the escape codes for xterm-color to interpret afterwards. Don't know how common this is but something in the documentation would be appropriate as you suggest. BTW, isnt't the font-locking interfering with xterm-color as well?
I'm using version 1.8, from melpa yesterday -- I think it is the same as master?
The specific --colors options that result in errors in the process-filter that I tried have to do with underline/font-weight, eg.
--colors=path:style:bold
--colors=path:style:underline
which looks like this with xterm-color-filter
The actual error is caused by compilation-error-properties
trying to match against compilation-transform-file-match-alist
when no file is found, but a match occurs in the compilation-error-regexp-alist-alist
. I think this is a bug in compilation-error-properties
. (I'm on emacs 27, and there is a note that the default compilation-transform-file-match-alist
changed in this version, so there may not be an error in earlier versions if its default is nil
)
But, regardless of the error (if I set that variable to nil, there is no error), the rg-results
isn't parsed correctly -- no font-lock, etc. -- with the additional --colors
flags.
xterm-color-filter
before other compilation-filters
on the raw string, since it's purpose is to interpret the raw escapes, but you're right, it wouldn't be a problem to apply afterward, but it would also serve no purpose at that point since the escapes are already handled.Yea, the things you mentioned don't work without rg-filter
.
I haven't run into any problems b/w font-locking and xterm-color AFAIR.
Oh, yeah, those are probably added to the color is specified on command the line. I think I would have to use the --no-config
flag to fix that. I will do that and add an option to disable it for brave souls like you that want to do advanced customisations.
The suggestion for the order of compilation-filter
was to get xterm-color out of the way when using this package. It would not give any powers to xterm-color as you want.
Having a more tight integration with xterm-color isn't high on my priority list right now but let's keep this issue open for now for visibility in case someone else think it's useful.
Sounds good, thanks!
I'm the author of xterm-color.el. I don't use rg.el or ag.el but someone filed an ag.el ANSI escape matching issue recently and I was curious to see if other packages were affected.
My answer also applies to rg.el. You should absolutely not depend on ANSI escapes when you do matching on results. Hardcoding specific ANSI SGR sequences in the code should be considered a broken design since it makes terminal assumptions that could very well not hold and is also a boundary violation. I urge you to find something else to match on.
Thanks for the input. In theory you are probably correct. This works well enough in practice though.
I think the main practical problem here is that xterm-color.el instructs its users to advice compilation-filter
instead of using compilation-filter-hook
for its modifications. This breaks any reasonable expectations downstream packages have on the input to its own filtering.
I don't agree that raw terminal ANSI sequences being part of the compilation-filter output is a reasonable expectation.
Main issue: rg.el (and ag.el) match on raw hardcoded terminal-specific ANSI sequences. As I explained, this is broken design. By advising compilation-filter xterm-color simply strips those sequences. It doesn't break any compilation-mode contracts but it does expose model brokenness and assumptions that do not generalize very well.
compilation-mode uses these variables that contain patterns to match on:
Notice that there are absolutely no terminal escapes in there. It's just matching on text output.
I want to chime in here as a mere user of both rg.el
and xterm-color
(and ag.el
, which has the same issue as rg.el
) and say that I'd really like to be able use all three of these packages, but right now I'm faced with the unenviable choice of either having rg.el
and ag.el
work, and not use xterm-color
, or use xterm-color
and have the other two packages break.
Can't we get past this impasse?
How hard can it be to have rg.el match on text output? I don't use it so I can't really say, but it doesn't sound hard. Could it be a 20 minute fix (that would also increase overall robustness) ?
@diamond-lizard Not much I can do as a downstream package.
The problem here is that the the minimal expectation a downstream package can make on its upstream counter part is that it works as it's implemented. If that is no longer true (by advising the upstream package), this fundamental requirement is broken. If xterm-color
still wants to recommend this way of integration it may do so of course but I think it should be clear that users are on their own when doing that. The setup even breaks emacs built in functionality in grep.el
for instance.
So what you can do is to hook in xterm-color
into compilation-filter-hook
instead (I hope that is supported). That would allow a downstream package to have control over its input.
@atomontage As for how hard it would be, essentially you'd had to emulate ripgrep's regexp engine to be able to highlight search matches. That is not all but should at least take more than 20 minutes for most people...
- Using xterm-color-filter also breaks rg's compilation (from xterm-color library)
I believe this should at least be fixed on emacs 28 master, where the aforementioned error in compilation-error-properties
was fixed (at least I was notified it was, although haven't tried it myself yet) bug#38081
The setup even breaks emacs built in functionality in grep.el for instance.
Let me expand on this since you didn't provide details. grep.el has an optional "highlight matches" mode, that relies on terminal escapes and performs its own interpretation and removal of said escapes. Highlighting in this fashion, taking into account the hardcoded escape sequences, is as broken as highlighting in rg.el and ag.el (broken in terms of assuming specific escapes and doing regular expression parsing rather than state machine processing). The good news is that in grep.el this is optional/configurable. The even better news is that grep.el does not conflate highlighting with semantic information extraction and does not rely on terminal escapes for the latter.
There is a separation of concerns between semantic matching and highlighting. Since xterm-color does a much better job at highlighting, it makes sense to disable that part of grep.el and have xterm-color do it. This isn't possible with rg.el since it uses terminal escapes for both matching and highlighting.
For anyone that wants rg.el to work with xterm-color, the following steps ought to work:
Don't match on terminal color escapes for semantic (non-highlight related) data. This is a must.
Make highlighting optional. Folks that use xterm-color.el can then tell rg.el to not highlight faces and have xterm-color use its much more elaborate emulation to get better results. This is optional since xterm-color will perform its own emulation/highlighting before rg.el even sees terminal escapes. If you implement it, you make it crystal clear that you support operation in non-highlighting mode and avoid possible confusion.
So you don't need to emulate ripgrep's regexp engine to highlight search matches. Assume that xterm-color will do that, and all you need to do is find a way to extract the semantic information you need without relying on terminal escapes. This by the way is exactly what grep.el (and compilation-mode) does.
EDIT: I will update xterm-color documentation and make it obvious that configuring compilation-mode in the way I recommend, breaks rg and ag.el.
Well, the thing is of course that escape codes are used for semantic data. This is the case for this package and also for built-in grep.el despite the statements above. I have no idea what ag.el does here though. And there is no sane way around that unless (as I said) you want to implement ripgrep's regexp behavior in elisp (or similar) or restricting functionality. In addition to that, regexps are not enough for perfectly matching of different parts of the output from ripgrep. Ripgrep itself has all the info it needs to do this perfectly and using that information gives the most robust solution.
So, essentially this all boils down to trade-offs (which is the usual case in all software projects). So I am not closing the door to actually supporting this but any comments claiming it's a "20 min fix" or similar is clearly missing the target. This package has been moving in the opposite direction for a while now and might even come to the point where using xterm-color doesn't make sense. I have been experimenting with using the ripgrep json putput which will solve all matching issues without using color escapes. Unfortunately the performance has not been up to par with current implementation so unsure if I will switch.
With all that said, my main concern is to avoid that this package is broken for users wanting to use xterm-color (which is a really cool package btw). Hooking xterm-color into compilation-filter-hook
should solve that so for any rg.el users that have this problem, I recommend to investigate that path.
I did some experiments with ripgrep's json output and parsing it is super fast in an Emacs built with jansson.
It took me a bit to figure out what was wrong here, and I don't think it is documented anywhere. Here are a couple issues I ran into:
If custom colors are defined in ~/.ripgreprc, eg.
--colors=path:fg:cyan
etc. then font-locking and other functions in the rg-results buffer don't work.Using
xterm-color-filter
also breaks rg's compilation (fromxterm-color
library) I usually usexterm-color-filter
in mycompilation-start-hook
to interpret shell escapes, however, since rg (like ag.el) relies on shell escapes for parsing, font-lock/other functions also break in the rg-results buffer.Quick fix is to remove xterm-color support around
rg-run
It looks like it would probably be out-of-the question to add support to
rg-results
that didn't rely on escape codes, but it might be helpful to mention incompatibilities somewhere.