|
From: | Jim Porter |
Subject: | bug#50806: 27.2; [PATCH] Optimize ansi-color.el |
Date: | Sun, 3 Oct 2021 13:16:05 -0700 |
Ok, thanks. I have actually already prepared patches for all these features: full-color in ansi-color.el and term.el and also "basic" ANSI escapes 1-8 for term.el that you mentioned. I guess there's no harm in sending them right now.
I took a brief look at these and they seem reasonable to my eyes. It'll be nice to have (mostly) complete support for reading ANSI colors in Emacs.
Thinking about it a bit more, one thing that might be nice to add for the first patch would be some additional tests to be sure that `ansi-color-context-region' and `ansi-color-context' work as expected (i.e. testing that multiple calls to `ansi-color-apply-on-region' and similar produce the correct results). That's one of the trickier bits in ansi-color.el (to me, anyway), and it'd be good to be sure all the various cases still work there. That said, it might be best to let the maintainers take a look before spending too much time on further tests.
I see you posted an updated patch that doesn't merge these vectors. I don't have an opinion here, although if we do merge them, it would probably be nice to get that into Emacs 28; other packages might conceivably want to let-bind those[1].Indeed, if we wanted to merge them we'd have to do it in Emacs 28. That's why I think its best to simply leave them un-merged.
If there's a performance benefit to merging them, I think it would be nice to do so while we have the chance. Perhaps a patch that just merges the two vectors, and nothing else, would make sense for Emacs 28. Best to ask the maintainers in this case too, though.
[Prev in Thread] | Current Thread | [Next in Thread] |