Strip OSC and DCS sequences to support e.g. OSC 8 hyperlinks over tmux.#280
Strip OSC and DCS sequences to support e.g. OSC 8 hyperlinks over tmux.#280khoek wants to merge 2 commits intoconsole-rs:mainfrom
Conversation
|
Appreciate the clean commit history, but it looks like there's a fair bit of code duplication. Could that be deduplicated? (If not, why not?) |
@djc Just my editorial choice to keep things simple. Did you have something more like the following (see latest commit) in mind? It's more complex but clarifies intent. (Happy to squash if you like it, honestly I kind of do.) Either way, I think the new commit at least clearly shows why there is some subtlety due to the differences in OSC and DCS semantics/idiosyncrasies---two examples:
Let me know what you like! |
khoek
force-pushed
the
main
branch
2 times, most recently
from
3d2f8f2 to
07d3b28
Compare
|
(Sorry about the noise I was bikeshedding a function name.) |
djc
left a comment
djc
left a comment
There was a problem hiding this comment.
I definitely like it better too -- please squash.
Is there existing stuff which could leverage the same trait?
| } | ||
|
|
||
| fn consume_dcs_end_exclusive(it: &mut Peekable<CharIndices<'_>>, start: usize) -> usize { | ||
| fn consume_end_exclusive<S: EscSequence>( |
There was a problem hiding this comment.
Maybe this could/should be a provided method on the trait?
There was a problem hiding this comment.
I think reasonable, but I'm a bit averse because I guess I view consume_end_exclusive as "just another" free function parser helper just like the existing find_dfa_end_exclusive_after_entry---I'm not really imagining a case where we'd want to override the default implementation of consume_end_exclusive if it was on a trait.
src/ansi.rs
Outdated
| it.next(); | ||
| return end; | ||
| } | ||
| trait EscSequence { |
There was a problem hiding this comment.
Nit: prefer top-down ordering. That means EscSequence comes after impls for it but before EscAction.
Also, within the trait I'd prefer on_escape(), on_char(), START (renamed from START_CHAR which just duplicates its type).
Maybe EscAction could be reformulated as a ControlFlow<(), bool>?
There was a problem hiding this comment.
Ordering/naming changes made.
I didn't go with ControlFlow<(), bool> because EscAction is genuinely a "product type"---sometimes we want to "end", sometimes we want to consume next, and sometimes we want to "end" but also consume the next character. So (unless I'm missing something obvious) we'd at least need ControlFlow<bool, bool> which then seems to obscure intent to me since the bools would both mean the same thing.
Treat OSC and DCS (including tmux passthrough wrappers) as non-printing when iterating/stripping ANSI codes. Adds regression tests for OSC 8 hyperlinks (ST/BEL) and tmux-wrapped OSC 8 hyperlinks.
Ensure the complex_data test fixture is present in packaged sources.
|
If there's a natural other place the same trait fits, I can't seem to find it. I would say the DFA core is structurally a bit different. (Above are my subjective feelings about your comments and they are made with relatively little conviction, so please feel free to override if you feel more strongly.) |
2 participants
I noticed that using
indicatif(and friends) to embed a hyperlink in a console progress bar caused the wheels to fall off my app (with many newlines being spammed to the console and the auto-updating line-rewriting broken). I tracked the problem down to the OSC sequences incorrectly being interpreted as printable, and the following patch fixes the problem for me.